| Message ID | 76f0a328e53daa4a2303fe17d3bbe157d7b40d03.1437699224.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jul 27, 2015 at 11:37 AM, Alistair Francis <alistair.francis@xilinx.com> wrote: > If the ObjectClass has no type return NULL instead of trying to compare > the type name. > What was the issue? Regards, Peter > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > Reviewed-by: Sai Pavan Boddu <saipava@xilinx.com> > --- > qom/object.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/qom/object.c b/qom/object.c > index eea8edf..2d6d715 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -603,7 +603,7 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class, > TypeImpl *target_type; > TypeImpl *type; > > - if (!class) { > + if (!class || !class->type) { > return NULL; > } > > -- > 1.7.1 > >
On Sat, Aug 15, 2015 at 2:22 PM, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote: > On Mon, Jul 27, 2015 at 11:37 AM, Alistair Francis > <alistair.francis@xilinx.com> wrote: >> If the ObjectClass has no type return NULL instead of trying to compare >> the type name. >> > > What was the issue? There is a seg fault in object_class_dynamic_cast() because there is no type in the ObjectClass struct. It happens when it is trying to cast the "pci-device", which is called from the ahci_irq_lower() function. The function is testing if the device is a pci device, so it should return NULL if it isn't valid. Thanks, Alistair > > Regards, > Peter > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> Reviewed-by: Sai Pavan Boddu <saipava@xilinx.com> >> --- >> qom/object.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/qom/object.c b/qom/object.c >> index eea8edf..2d6d715 100644 >> --- a/qom/object.c >> +++ b/qom/object.c >> @@ -603,7 +603,7 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class, >> TypeImpl *target_type; >> TypeImpl *type; >> >> - if (!class) { >> + if (!class || !class->type) { >> return NULL; >> } >> >> -- >> 1.7.1 >> >> >
Am 18.08.2015 um 00:24 schrieb Alistair Francis: > On Sat, Aug 15, 2015 at 2:22 PM, Peter Crosthwaite > <crosthwaitepeter@gmail.com> wrote: >> On Mon, Jul 27, 2015 at 11:37 AM, Alistair Francis >> <alistair.francis@xilinx.com> wrote: >>> If the ObjectClass has no type return NULL instead of trying to compare >>> the type name. >>> >> >> What was the issue? > > There is a seg fault in object_class_dynamic_cast() because there is > no type in the ObjectClass struct. That should never happen, ever since TYPE_OBJECT is no longer NULL. > It happens when it is trying to cast the "pci-device", which is called > from the ahci_irq_lower() function. The function is testing if the > device is a pci device, so it should return NULL if it isn't valid. It rather sounds as if some build-time dependency is wrong, which we used to run into for the Container type before Paolo macrofied this. Please try again with a clean build - if it still occurs, we'll need a reproducible test case to investigate what is going on rather than papering over a latent bug. Thanks, Andreas
On Mon, Aug 17, 2015 at 3:33 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 18.08.2015 um 00:24 schrieb Alistair Francis: >> On Sat, Aug 15, 2015 at 2:22 PM, Peter Crosthwaite >> <crosthwaitepeter@gmail.com> wrote: >>> On Mon, Jul 27, 2015 at 11:37 AM, Alistair Francis >>> <alistair.francis@xilinx.com> wrote: >>>> If the ObjectClass has no type return NULL instead of trying to compare >>>> the type name. >>>> >>> >>> What was the issue? >> >> There is a seg fault in object_class_dynamic_cast() because there is >> no type in the ObjectClass struct. > > That should never happen, ever since TYPE_OBJECT is no longer NULL. > >> It happens when it is trying to cast the "pci-device", which is called >> from the ahci_irq_lower() function. The function is testing if the >> device is a pci device, so it should return NULL if it isn't valid. Yes so I vaguely remember this now. It is about MSI interrupts which have nothing to do with sysbus implementation. My solution was to rip that PCI specific stuff out of AHCI completely in my branch. Should sysbus and PCI AHCI classes install their own separate logic for this part via a virtualised hook? On the topic though, I notice many PCI devices have this MSI specific logic in them. Is it possible for devs to just treat interrupts as pins and the PCI layers do the MSI vs non-MSI logic switch in core layers? If Andreas' idea don't work this is still a core QOM bug though. I think object_dynamic_cast should not have this segfault when passed a non implementing object. Regards, Peter > > It rather sounds as if some build-time dependency is wrong, which we > used to run into for the Container type before Paolo macrofied this. > > Please try again with a clean build - if it still occurs, we'll need a > reproducible test case to investigate what is going on rather than > papering over a latent bug. > > Thanks, > Andreas > > -- > SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
On Mon, Aug 17, 2015 at 4:37 PM, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote: > On Mon, Aug 17, 2015 at 3:33 PM, Andreas Färber <afaerber@suse.de> wrote: >> Am 18.08.2015 um 00:24 schrieb Alistair Francis: >>> On Sat, Aug 15, 2015 at 2:22 PM, Peter Crosthwaite >>> <crosthwaitepeter@gmail.com> wrote: >>>> On Mon, Jul 27, 2015 at 11:37 AM, Alistair Francis >>>> <alistair.francis@xilinx.com> wrote: >>>>> If the ObjectClass has no type return NULL instead of trying to compare >>>>> the type name. >>>>> >>>> >>>> What was the issue? >>> >>> There is a seg fault in object_class_dynamic_cast() because there is >>> no type in the ObjectClass struct. >> >> That should never happen, ever since TYPE_OBJECT is no longer NULL. >> >>> It happens when it is trying to cast the "pci-device", which is called >>> from the ahci_irq_lower() function. The function is testing if the >>> device is a pci device, so it should return NULL if it isn't valid. > > Yes so I vaguely remember this now. It is about MSI interrupts which > have nothing to do with sysbus implementation. My solution was to rip > that PCI specific stuff out of AHCI completely in my branch. Should > sysbus and PCI AHCI classes install their own separate logic for this > part via a virtualised hook? > > On the topic though, I notice many PCI devices have this MSI specific > logic in them. Is it possible for devs to just treat interrupts as > pins and the PCI layers do the MSI vs non-MSI logic switch in core > layers? > > If Andreas' idea don't work this is still a core QOM bug though. I > think object_dynamic_cast should not have this segfault when passed a > non implementing object. > > Regards, > Peter > >> >> It rather sounds as if some build-time dependency is wrong, which we >> used to run into for the Container type before Paolo macrofied this. >> >> Please try again with a clean build - if it still occurs, we'll need a >> reproducible test case to investigate what is going on rather than >> papering over a latent bug. Hey, Sorry abut the delay, but I didn't get a chance to look at this last week. I tried with a clean setup and still see the seg fault. I will try to look into it more this week, but if anyone is interested here are the steps to reproduce: On the latest mainline QEMU, with my 2nd and 3rd patches applied $ ./configure --target-list="aarch64-softmmu,microblazeel-softmmu" --disable-pie --disable-sdl --disable-werror # This is what is required at work $ ./aarch64-softmmu/qemu-system-aarch64 -M xlnx-ep108 -display none -kernel ./u-boot.elf -m 8000000 -nographic -serial mon:stdio # Boot u-boot on QEMU The image I'm using is available at: http://1drv.ms/1NxDXLo Thanks, Alistair >> >> Thanks, >> Andreas >> >> -- >> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany >> GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg) >
On Mon, Aug 24, 2015 at 4:36 PM, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Mon, Aug 17, 2015 at 4:37 PM, Peter Crosthwaite > <crosthwaitepeter@gmail.com> wrote: >> On Mon, Aug 17, 2015 at 3:33 PM, Andreas Färber <afaerber@suse.de> wrote: >>> Am 18.08.2015 um 00:24 schrieb Alistair Francis: >>>> On Sat, Aug 15, 2015 at 2:22 PM, Peter Crosthwaite >>>> <crosthwaitepeter@gmail.com> wrote: >>>>> On Mon, Jul 27, 2015 at 11:37 AM, Alistair Francis >>>>> <alistair.francis@xilinx.com> wrote: >>>>>> If the ObjectClass has no type return NULL instead of trying to compare >>>>>> the type name. >>>>>> >>>>> >>>>> What was the issue? >>>> >>>> There is a seg fault in object_class_dynamic_cast() because there is >>>> no type in the ObjectClass struct. >>> >>> That should never happen, ever since TYPE_OBJECT is no longer NULL. >>> >>>> It happens when it is trying to cast the "pci-device", which is called >>>> from the ahci_irq_lower() function. The function is testing if the >>>> device is a pci device, so it should return NULL if it isn't valid. >> >> Yes so I vaguely remember this now. It is about MSI interrupts which >> have nothing to do with sysbus implementation. My solution was to rip >> that PCI specific stuff out of AHCI completely in my branch. Should >> sysbus and PCI AHCI classes install their own separate logic for this >> part via a virtualised hook? >> >> On the topic though, I notice many PCI devices have this MSI specific >> logic in them. Is it possible for devs to just treat interrupts as >> pins and the PCI layers do the MSI vs non-MSI logic switch in core >> layers? >> >> If Andreas' idea don't work this is still a core QOM bug though. I >> think object_dynamic_cast should not have this segfault when passed a >> non implementing object. >> >> Regards, >> Peter >> >>> >>> It rather sounds as if some build-time dependency is wrong, which we >>> used to run into for the Container type before Paolo macrofied this. >>> >>> Please try again with a clean build - if it still occurs, we'll need a >>> reproducible test case to investigate what is going on rather than >>> papering over a latent bug. > > Hey, > > Sorry abut the delay, but I didn't get a chance to look at this last > week. I tried with a clean setup and still see the seg fault. > > I will try to look into it more this week, but if anyone is interested > here are the steps to reproduce: > > On the latest mainline QEMU, with my 2nd and 3rd patches applied > $ ./configure --target-list="aarch64-softmmu,microblazeel-softmmu" > --disable-pie --disable-sdl --disable-werror # This is what is > required at work > $ ./aarch64-softmmu/qemu-system-aarch64 -M xlnx-ep108 -display none > -kernel ./u-boot.elf -m 8000000 -nographic -serial mon:stdio # Boot > u-boot on QEMU > > The image I'm using is available at: http://1drv.ms/1NxDXLo > So it's not a core bug. That container_of in ahci_lower_irq is incorrectly assuming that the passed AHCIState * is always for a PCI, which it is not in the sysbus case. So it's incorrectly getting the offset of QOM the object and the QOM cast is treating some invalid offset into the (or past) object as a QOM object base address. The simplest solution is a back pointer in AHCIState to the encapsulating device (would be a DeviceState *). The container_of is replaced with a nav of this pointer and then the conditional PCI cast can work. Regards, Peter > Thanks, > > Alistair > >>> >>> Thanks, >>> Andreas >>> >>> -- >>> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany >>> GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg) >>
diff --git a/qom/object.c b/qom/object.c index eea8edf..2d6d715 100644 --- a/qom/object.c +++ b/qom/object.c @@ -603,7 +603,7 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class, TypeImpl *target_type; TypeImpl *type; - if (!class) { + if (!class || !class->type) { return NULL; }