Message ID | e5ab65c34f7e8a6d097711eeae7b2dbae6ce5496.1440705929.git.alistair.francis@xilinx.com |
---|---|
State | New |
Headers | show |
On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com> > --- > > 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 >
On Thu, Aug 27, 2015 at 2:28 PM, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote: > On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis > <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com> >> --- >> >> 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 >> >
On Thu, Aug 27, 2015 at 3:06 PM, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Thu, Aug 27, 2015 at 2:28 PM, Peter Crosthwaite > <crosthwaitepeter@gmail.com> wrote: >> On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis >> <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com> >>> --- >>> >>> 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. > But the original code uses object_dynamic_cast which should be a non-asserting cast attempt. What was the assertion you were getting? Regards, Peter > 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 >>> >>
() On Thu, Aug 27, 2015 at 3:36 PM, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote: > On Thu, Aug 27, 2015 at 3:06 PM, Alistair Francis > <alistair.francis@xilinx.com> wrote: >> On Thu, Aug 27, 2015 at 2:28 PM, Peter Crosthwaite >> <crosthwaitepeter@gmail.com> wrote: >>> On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis >>> <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com> >>>> --- >>>> >>>> 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. >> > > But the original code uses object_dynamic_cast which should be a > non-asserting cast attempt. What was the assertion you were getting? Oh! I see what you mean. I can use the object_dynamic_cast() instead. I thought you meant to use the macros that are defined. If there are no other comments I will re-send the series. Thanks, Alistair > > Regards, > Peter > >> 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 >>>> >>> >
On Thu, Aug 27, 2015 at 3:58 PM, Alistair Francis <alistair.francis@xilinx.com> wrote: > () > > On Thu, Aug 27, 2015 at 3:36 PM, Peter Crosthwaite > <crosthwaitepeter@gmail.com> wrote: >> On Thu, Aug 27, 2015 at 3:06 PM, Alistair Francis >> <alistair.francis@xilinx.com> wrote: >>> On Thu, Aug 27, 2015 at 2:28 PM, Peter Crosthwaite >>> <crosthwaitepeter@gmail.com> wrote: >>>> On Thu, Aug 27, 2015 at 1:47 PM, Alistair Francis >>>> <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com> >>>>> --- >>>>> >>>>> 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. >>> >> >> But the original code uses object_dynamic_cast which should be a >> non-asserting cast attempt. What was the assertion you were getting? > > Oh! I see what you mean. I can use the object_dynamic_cast() instead. > I thought you meant to use the macros that are defined. > > If there are no other comments I will re-send the series. > Nothing more from me :) Regards, Peter > Thanks, > > Alistair > >> >> Regards, >> Peter >> >>> 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 >>>>> >>>> >>
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); + 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, "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;
The AHCIState strucut 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> --- hw/ide/ahci.c | 29 +++++++++++++++++++++++------ hw/ide/ahci.h | 2 ++ 2 files changed, 25 insertions(+), 6 deletions(-)