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

[Alistair Francis]

()

On Thu, Aug 27, 2015 at 3:36 PM, Peter Crosthwaite
<address@hidden> wrote:
> On Thu, Aug 27, 2015 at 3:06 PM, Alistair Francis
> <address@hidden> wrote:
>> On Thu, Aug 27, 2015 at 2:28 PM, Peter Crosthwaite
>> <address@hidden> wrote:
>>> On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis
>>> <address@hidden> wrote:
>>>> The AHCIState strucut can either have AHCIPCIState or SysbusAHCIState
>>>
>>> "struct"
>>
>> Good catch, will fix
>>
>>>
>>>> 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>
>>>> ---
>>>>
>>>>  hw/ide/ahci.c |   29 +++++++++++++++++++++++------
>>>>  hw/ide/ahci.h |    2 ++
>>>>  2 files changed, 25 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>>>> index 02d85fa..b45b21d 100644
>>>> --- a/hw/ide/ahci.c
>>>> +++ b/hw/ide/ahci.c
>>>> @@ -121,9 +121,17 @@ 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);
>>>
>>> Why can't you just convert this to use use s->container? ...
>>>
>>>> +    DeviceState *dev_state = DEVICE(s->container);
>>>
>>> Unneeded cast. s->container is already DeviceState, but I'm not sure
>>> you need this local variable at all.
>>
>> I'll remove the cast. The variable is just included to save line space
>> (and complexity).
>>
>>>
>>>> +    PCIDevice *pci_dev = NULL;
>>>> +    ObjectClass *ret;
>>>> +
>>>> +    /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */
>>>> +    ret = object_class_dynamic_cast(object_get_class(OBJECT(dev_state)),
>>>> +                                    TYPE_PCI_DEVICE);
>>>
>>> ... I don't understand why this needs to be done as a class cast, does
>>> the original object cast approach not work?
>>
>> The problem with the standard ones is that they contain asserts. So you can't
>> do a test to see if the cast is possible, which is what I want to do.
>>
>
> But the original code uses object_dynamic_cast which should be a
> non-asserting cast attempt. What was the assertion you were getting?

Oh! I see what you mean. I can use the object_dynamic_cast() instead.
I thought you meant to use the macros that are defined.

If there are no other comments I will re-send the series.

Thanks,

Alistair

>
> Regards,
> Peter
>
>> Thanks,
>>
>> Alistair
>>
>>>
>>> Regards,
>>> Peter
>>>
>>>> +    if (ret) {
>>>> +        /* AHCIState parent is AHCIPCIState */
>>>> +        pci_dev = PCI_DEVICE(dev_state);
>>>> +    }
>>>>
>>>>      DPRINTF(0, "raise irq\n");
>>>>
>>>> @@ -136,9 +144,17 @@ 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 = DEVICE(s->container);
>>>> +    PCIDevice *pci_dev = NULL;
>>>> +    ObjectClass *ret;
>>>> +
>>>> +    /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */
>>>> +    ret = object_class_dynamic_cast(object_get_class(OBJECT(dev_state)),
>>>> +                                    TYPE_PCI_DEVICE);
>>>> +    if (ret) {
>>>> +        /* AHCIState parent is AHCIPCIState */
>>>> +        pci_dev = PCI_DEVICE(dev_state);
>>>> +    }
>>>>
>>>>      DPRINTF(0, "lower irq\n");
>>>>
>>>> @@ -1436,6 +1452,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
>>>>
>>>
>