Re: [Qemu-devel] [PATCH v4 2/4] ahci.c: Don't assume AHCIState's parent is AHCIPCIState

[Alistair Francis]

On Thu, Aug 27, 2015 at 5:27 PM, Peter Crosthwaite
<address@hidden> wrote:
> On Thu, Aug 27, 2015 at 5:16 PM, Alistair Francis
> <address@hidden> 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 <address@hidden>
>> ---
>> V4:
>>  - Remove unnesicary casts
>>  - Use object_dynamic_cast() instead of object_class_dynamic_cast()
>>
>>  hw/ide/ahci.c |   27 +++++++++++++++++++++------
>>  hw/ide/ahci.h |    2 ++
>>  2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 02d85fa..bab6f5c 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -121,9 +121,16 @@ 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 = NULL;
>> +    ObjectClass *ret;
>> +
>> +    /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */
>> +    ret = object_dynamic_cast(OBJECT(dev_state), TYPE_PCI_DEVICE);
>> +    if (ret) {
>> +        /* AHCIState parent is AHCIPCIState */
>> +        pci_dev = PCI_DEVICE(dev_state);
>> +    }
>>
>>      DPRINTF(0, "raise irq\n");
>>
>> @@ -136,9 +143,16 @@ 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 = NULL;
>> +    ObjectClass *ret;
>> +
>> +    /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */
>> +    ret = object_dynamic_cast(OBJECT(dev_state), TYPE_PCI_DEVICE);
>
> Is ret a correct type? object_dynamic_cast returns an object and you
> are pointer assigning to a class. I don't think it should need the
> extra variable at all. Does it work if all you do is this:
>
> -    AHCIPCIState *d = container_of(s, AHCIPCIState, ahci);
> -    PCIDevice *pci_dev =
> -        (PCIDevice *)object_dynamic_cast(OBJECT(d), TYPE_PCI_DEVICE);
> +    PCIDevice *pci_dev =
> +        (PCIDevice *)object_dynamic_cast(OBJECT(s->container),
> TYPE_PCI_DEVICE);

Good catch, that was left over from what I was doing before.

Thanks,

Alistair

>
> Regards,
> Peter
>
>> +    if (ret) {
>> +        /* AHCIState parent is AHCIPCIState */
>> +        pci_dev = PCI_DEVICE(dev_state);
>> +    }
>>
>>      DPRINTF(0, "lower irq\n");
>>
>> @@ -1436,6 +1450,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;
>> --
>> 1.7.1
>>
>