| Message ID | 1b264632e6c61b5bf6645395571d5b738c669e2a.1440719554.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 27, 2015 at 5:16 PM, Alistair Francis <alistair.francis@xilinx.com> wrote: > The AHCIState struct 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> > --- > V4: > - Remove unnesicary casts > - Use object_dynamic_cast() instead of object_class_dynamic_cast() > > hw/ide/ahci.c | 27 +++++++++++++++++++++------ > hw/ide/ahci.h | 2 ++ > 2 files changed, 23 insertions(+), 6 deletions(-) > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index 02d85fa..bab6f5c 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -121,9 +121,16 @@ 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 = s->container; > + PCIDevice *pci_dev = NULL; > + ObjectClass *ret; > + > + /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */ > + ret = object_dynamic_cast(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 +143,16 @@ 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 = s->container; > + PCIDevice *pci_dev = NULL; > + ObjectClass *ret; > + > + /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */ > + ret = object_dynamic_cast(OBJECT(dev_state), TYPE_PCI_DEVICE); Is ret a correct type? object_dynamic_cast returns an object and you are pointer assigning to a class. I don't think it should need the extra variable at all. Does it work if all you do is this: - AHCIPCIState *d = container_of(s, AHCIPCIState, ahci); - PCIDevice *pci_dev = - (PCIDevice *)object_dynamic_cast(OBJECT(d), TYPE_PCI_DEVICE); + PCIDevice *pci_dev = + (PCIDevice *)object_dynamic_cast(OBJECT(s->container), TYPE_PCI_DEVICE); Regards, Peter > + if (ret) { > + /* AHCIState parent is AHCIPCIState */ > + pci_dev = PCI_DEVICE(dev_state); > + } > > DPRINTF(0, "lower irq\n"); > > @@ -1436,6 +1450,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 >
On Thu, Aug 27, 2015 at 5:27 PM, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote: > On Thu, Aug 27, 2015 at 5:16 PM, Alistair Francis > <alistair.francis@xilinx.com> wrote: >> The AHCIState struct 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> >> --- >> V4: >> - Remove unnesicary casts >> - Use object_dynamic_cast() instead of object_class_dynamic_cast() >> >> hw/ide/ahci.c | 27 +++++++++++++++++++++------ >> hw/ide/ahci.h | 2 ++ >> 2 files changed, 23 insertions(+), 6 deletions(-) >> >> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c >> index 02d85fa..bab6f5c 100644 >> --- a/hw/ide/ahci.c >> +++ b/hw/ide/ahci.c >> @@ -121,9 +121,16 @@ 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 = s->container; >> + PCIDevice *pci_dev = NULL; >> + ObjectClass *ret; >> + >> + /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */ >> + ret = object_dynamic_cast(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 +143,16 @@ 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 = s->container; >> + PCIDevice *pci_dev = NULL; >> + ObjectClass *ret; >> + >> + /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */ >> + ret = object_dynamic_cast(OBJECT(dev_state), TYPE_PCI_DEVICE); > > Is ret a correct type? object_dynamic_cast returns an object and you > are pointer assigning to a class. I don't think it should need the > extra variable at all. Does it work if all you do is this: > > - AHCIPCIState *d = container_of(s, AHCIPCIState, ahci); > - PCIDevice *pci_dev = > - (PCIDevice *)object_dynamic_cast(OBJECT(d), TYPE_PCI_DEVICE); > + PCIDevice *pci_dev = > + (PCIDevice *)object_dynamic_cast(OBJECT(s->container), > TYPE_PCI_DEVICE); Good catch, that was left over from what I was doing before. Thanks, Alistair > > Regards, > Peter > >> + if (ret) { >> + /* AHCIState parent is AHCIPCIState */ >> + pci_dev = PCI_DEVICE(dev_state); >> + } >> >> DPRINTF(0, "lower irq\n"); >> >> @@ -1436,6 +1450,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 --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 02d85fa..bab6f5c 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -121,9 +121,16 @@ 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 = s->container; + PCIDevice *pci_dev = NULL; + ObjectClass *ret; + + /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */ + ret = object_dynamic_cast(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 +143,16 @@ 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 = s->container; + PCIDevice *pci_dev = NULL; + ObjectClass *ret; + + /* Check is AHCIState's parent is SysbusAHCIState or AHCIPCIState */ + ret = object_dynamic_cast(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 +1450,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;
The AHCIState struct 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> --- V4: - Remove unnesicary casts - Use object_dynamic_cast() instead of object_class_dynamic_cast() hw/ide/ahci.c | 27 +++++++++++++++++++++------ hw/ide/ahci.h | 2 ++ 2 files changed, 23 insertions(+), 6 deletions(-)