On Thu, Aug 27, 2015 at 2:28 PM, Peter Crosthwaite <[email protected]> wrote: > On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis > <[email protected]> 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 <[email protected]> >> --- >> >> 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 >> >
