| Message ID | 1392073373-3295-1-git-send-email-robherring2@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 10 February 2014 23:02, Rob Herring <robherring2@gmail.com> wrote: > From: Rob Herring <rob.herring@linaro.org> > > 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 <rob.herring@linaro.org> > --- > 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
On Mon, Feb 10, 2014 at 6:32 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 10 February 2014 23:02, Rob Herring <robherring2@gmail.com> wrote: >> From: Rob Herring <rob.herring@linaro.org> >> >> 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 <rob.herring@linaro.org> >> --- >> 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. Any comments on this? If not, can someone apply this? >> @@ -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
Il 11/02/2014 01:32, Peter Maydell ha scritto: >> > @@ -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? No, because PCI AHCI is also buggy under I/O load. Paolo
On 10 February 2014 23:02, Rob Herring <robherring2@gmail.com> wrote: > From: Rob Herring <rob.herring@linaro.org> > > 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 <rob.herring@linaro.org> Andreas just confirmed on IRC that the cast syntax you've used is fine, so I'll take this into target-arm.next (since Highbank is the only user of sysbus-ahci). I expect to get this into rc1. Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
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); } } @@ -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() }, }; @@ -1328,7 +1329,7 @@ static void sysbus_ahci_realize(DeviceState *dev, Error **errp) SysBusDevice *sbd = SYS_BUS_DEVICE(dev); SysbusAHCIState *s = SYSBUS_AHCI(dev); - ahci_init(&s->ahci, dev, NULL, s->num_ports); + ahci_init(&s->ahci, dev, &address_space_memory, s->num_ports); sysbus_init_mmio(sbd, &s->ahci.mem); sysbus_init_irq(sbd, &s->ahci.irq);