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

[Peter Crosthwaite]

On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis
<address@hidden> 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 <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.

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