diff mbox

[v3,2/4] ahci.c: Don't assume AHCIState's parent is AHCIPCIState

Message ID e5ab65c34f7e8a6d097711eeae7b2dbae6ce5496.1440705929.git.alistair.francis@xilinx.com
State New
Headers show

Commit Message

Alistair Francis Aug. 27, 2015, 8:47 p.m. UTC
The AHCIState strucut 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.francis@xilinx.com>
---

 hw/ide/ahci.c |   29 +++++++++++++++++++++++------
 hw/ide/ahci.h |    2 ++
 2 files changed, 25 insertions(+), 6 deletions(-)

Comments

Peter Crosthwaite Aug. 27, 2015, 9:28 p.m. UTC | #1
On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> The AHCIState strucut can either have AHCIPCIState or SysbusAHCIState

"struct"

> 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.francis@xilinx.com>
> ---
>
>  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.

> +    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?

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
>
Alistair Francis Aug. 27, 2015, 10:06 p.m. UTC | #2
On Thu, Aug 27, 2015 at 2:28 PM, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis
> <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com>
>> ---
>>
>>  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.

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
>>
>
Peter Crosthwaite Aug. 27, 2015, 10:36 p.m. UTC | #3
On Thu, Aug 27, 2015 at 3:06 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Thu, Aug 27, 2015 at 2:28 PM, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis
>> <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com>
>>> ---
>>>
>>>  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?

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
>>>
>>
Alistair Francis Aug. 27, 2015, 10:58 p.m. UTC | #4
()

On Thu, Aug 27, 2015 at 3:36 PM, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Thu, Aug 27, 2015 at 3:06 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Thu, Aug 27, 2015 at 2:28 PM, Peter Crosthwaite
>> <crosthwaitepeter@gmail.com> wrote:
>>> On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis
>>> <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com>
>>>> ---
>>>>
>>>>  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
>>>>
>>>
>
Peter Crosthwaite Aug. 27, 2015, 11:25 p.m. UTC | #5
On Thu, Aug 27, 2015 at 3:58 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> ()
>
> On Thu, Aug 27, 2015 at 3:36 PM, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> On Thu, Aug 27, 2015 at 3:06 PM, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> On Thu, Aug 27, 2015 at 2:28 PM, Peter Crosthwaite
>>> <crosthwaitepeter@gmail.com> wrote:
>>>> On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis
>>>> <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com>
>>>>> ---
>>>>>
>>>>>  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.
>

Nothing more from me :)

Regards,
Peter

> 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
>>>>>
>>>>
>>
diff mbox

Patch

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);
+    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, "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;