Re: [Qemu-devel] [PATCH] ahci: fix sysbus support

[Peter Maydell]

On 10 February 2014 23:02, Rob Herring <address@hidden> wrote:
> From: Rob Herring <address@hidden>
>
> Non-PCI AHCI support is broken due to assertion failures when trying
> to convert AHCIState to a PCIDevice pointer as AHCIState can have
> different container structs. Fix this by using the non-asserting object
> cast and checking the returned pointer is not NULL.
>
> The AddressSpace pointer is also being initialized to NULL and causing
> dma_memory_map call to fail. Fix this by initializing to
> address_space_memory for sysbus instances.
>
> Also correct AHCI_VMSTATE to use the correct container SysbusAHCIState
> for sysbus instances.
>
> Signed-off-by: Rob Herring <address@hidden>
> ---
>  hw/ide/ahci.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index fbea9e8..55f984e 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -118,11 +118,11 @@ 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 = PCI_DEVICE(d);
> +    PCIDevice *pci_dev = (PCIDevice *)object_dynamic_cast(OBJECT(d), 
> TYPE_PCI_DEVICE);
>
>      DPRINTF(0, "raise irq\n");
>
> -    if (msi_enabled(pci_dev)) {
> +    if (pci_dev && msi_enabled(pci_dev)) {
>          msi_notify(pci_dev, 0);
>      } else {
>          qemu_irq_raise(s->irq);
> @@ -132,10 +132,11 @@ 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);
>
>      DPRINTF(0, "lower irq\n");
>
> -    if (!msi_enabled(PCI_DEVICE(d))) {
> +    if (!pci_dev || !msi_enabled(pci_dev)) {
>          qemu_irq_lower(s->irq);
>      }
>  }

Cc'ing Andreas in case he knows a neater way of doing those casts.

> @@ -1311,7 +1312,7 @@ static const VMStateDescription vmstate_sysbus_ahci = {
>      .name = "sysbus-ahci",
>      .unmigratable = 1, /* Still buggy under I/O load */
>      .fields = (VMStateField []) {
> -        VMSTATE_AHCI(ahci, AHCIPCIState),
> +        VMSTATE_AHCI(ahci, SysbusAHCIState),
>          VMSTATE_END_OF_LIST()
>      },

...I wonder if that fixes the "buggy under I/O load" issue the
comment is talking about. Jason, do you still have whatever
testcase you were using when you added that comment?

thanks
-- PMM