On Mon, Aug 31, 2015 at 3:38 PM, John Snow <js...@redhat.com> wrote:
>
>
> On 08/28/2015 08:04 PM, Alistair Francis wrote:
>> The AHCIState struct can either have AHCIPCIState or SysbusAHCIState
>> as a parent. The ahci_irq_lower() and ahci_irq_raise() functions
>> assume that it is always AHCIPCIState, which is not always the
>> case, which causes a seg fault. Verify what the container of AHCIState
>> is before setting the PCIDevice struct.
>>
>> Signed-off-by: Alistair Francis <alistair.fran...@xilinx.com>
>> ---
>> V5:
>> - Remove the return checks when setting PCIDevice
>> V4:
>> - Remove unnesicary casts
>> - Use object_dynamic_cast() instead of object_class_dynamic_cast()
>>
>> hw/ide/ahci.c | 13 +++++++------
>> hw/ide/ahci.h | 2 ++
>> 2 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 02d85fa..d83efa4 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -121,9 +121,9 @@ static uint32_t ahci_port_read(AHCIState *s, int port,
>> int offset)
>>
>> static void ahci_irq_raise(AHCIState *s, AHCIDevice *dev)
>> {
>> - AHCIPCIState *d = container_of(s, AHCIPCIState, ahci);
>> - PCIDevice *pci_dev =
>> - (PCIDevice *)object_dynamic_cast(OBJECT(d), TYPE_PCI_DEVICE);
>> + DeviceState *dev_state = s->container;
>> + PCIDevice *pci_dev = (PCIDevice *)
>> object_dynamic_cast(OBJECT(dev_state),
>> + TYPE_PCI_DEVICE);
>>
>> DPRINTF(0, "raise irq\n");
>>
>> @@ -136,9 +136,9 @@ static void ahci_irq_raise(AHCIState *s, AHCIDevice *dev)
>>
>> static void ahci_irq_lower(AHCIState *s, AHCIDevice *dev)
>> {
>> - AHCIPCIState *d = container_of(s, AHCIPCIState, ahci);
>> - PCIDevice *pci_dev =
>> - (PCIDevice *)object_dynamic_cast(OBJECT(d), TYPE_PCI_DEVICE);
>> + DeviceState *dev_state = s->container;
>> + PCIDevice *pci_dev = (PCIDevice *)
>> object_dynamic_cast(OBJECT(dev_state),
>> + TYPE_PCI_DEVICE);
>>
>> DPRINTF(0, "lower irq\n");
>>
>> @@ -1436,6 +1436,7 @@ void ahci_init(AHCIState *s, DeviceState *qdev,
>> AddressSpace *as, int ports)
>> s->as = as;
>> s->ports = ports;
>> s->dev = g_new0(AHCIDevice, ports);
>> + s->container = qdev;
>> ahci_reg_init(s);
>> /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now
>> */
>> memory_region_init_io(&s->mem, OBJECT(qdev), &ahci_mem_ops, s,
>> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
>> index c055d6b..c9b3805 100644
>> --- a/hw/ide/ahci.h
>> +++ b/hw/ide/ahci.h
>> @@ -287,6 +287,8 @@ struct AHCIDevice {
>> };
>>
>> typedef struct AHCIState {
>> + DeviceState *container;
>> +
>> AHCIDevice *dev;
>> AHCIControlRegs control_regs;
>> MemoryRegion mem;
>>
>
> This is kind of ugly ... but it works, and it doesn't impact migratability.
>
> If someone abstracts MSI away from AHCI in the future, this can be
> un-done and the state cleaned up again.
I agree, not ideal but I think it's the best option at the moment, without
re-working the code.
>
> Doesn't break anything, so:
> Acked-by: John Snow <js...@redhat.com>
Thanks
Alistair
>
