RE: sysbus failed assert for xen_sysdev

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Markus Armbruster <armbru@xxxxxxxxxx>
> Sent: 23 June 2020 09:41
> To: Jason Andryuk <jandryuk@xxxxxxxxx>
> Cc: Mark Cave-Ayland <mark.cave-ayland@xxxxxxxxxxxx>; Anthony PERARD 
> <anthony.perard@xxxxxxxxxx>; xen-
> devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Paul Durrant <paul@xxxxxxx>; QEMU 
> <qemu-devel@xxxxxxxxxx>
> Subject: Re: sysbus failed assert for xen_sysdev
> 
> Jason Andryuk <jandryuk@xxxxxxxxx> writes:
> 
> > On Mon, Jun 22, 2020 at 5:17 PM Mark Cave-Ayland
> > <mark.cave-ayland@xxxxxxxxxxxx> wrote:
> >>
> >> On 22/06/2020 21:33, Jason Andryuk wrote:
> >>
> >> > Hi,
> >> >
> >> > Running qemu devel for a Xen VM is failing an assert after the recent
> >> > "qdev: Rework how we plug into the parent bus" sysbus changes.
> >> >
> >> > qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
> >> > `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
> >> > failed.
> >> >
> >> > dc->bus_type is "xen-sysbus" and it's the
> >> > `object_dynamic_cast(OBJECT(bus), dc->bus_type)` portion that fails
> >> > the assert.  bus seems to be "main-system-bus", I think:
> >> > (gdb) p *bus
> >> > $3 = {obj = {class = 0x55555636d780, free = 0x7ffff7c40db0 <g_free>,
> >> > properties = 0x5555563f7180, ref = 3, parent = 0x5555563fe980}, parent
> >> > = 0x0, name = 0x5555563fec60 "main-system-bus", ...
> >> >
> >> > The call comes from hw/xen/xen-legacy-backend.c:706
> >> > sysbus_realize_and_unref(SYS_BUS_DEVICE(xen_sysdev), &error_fatal);
> >> >
> >> > Any pointers on what needs to be fixed?
> >>
> >> Hi Jason,
> >>
> >> My understanding is that the assert() is telling you that you're plugging a
> >> TYPE_SYS_BUS_DEVICE into a bus that isn't derived from TYPE_SYSTEM_BUS.
> >> TYPE_SYS_BUS_DEVICE into a bus that isn't derived from TYPE_SYSTEM_BUS. A 
> >> quick look
> 
> Correct.  Let's review the assertion:
> 
>     assert(dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type));
> 
> Context: we're supposted to plug @dev into @bus, and @dc is @dev's
> DeviceClass.
> 
> The assertion checks that
> 
> 1. @dev plugs into a bus: dc->bus_type
> 
> 2. @bus is an instance of the type of bus @dev plugs into:
>    object_dynamic_cast(OBJECT(bus), dc->bus_type)
> 
> >> at the file in question suggests that you could try changing the parent 
> >> class of
> >> TYPE_XENSYSBUS from TYPE_BUS to TYPE_SYSTEM_BUS to see if that helps?
> >
> > Hi, Mark.
> >
> > Thanks, but unfortunately changing xensysbus_info .parent does not
> > stop the assert.  But it kinda pointed me in the right direction.
> >
> > xen-sysdev overrode the bus_type which was breaking sysbus_realize.
> > So drop that:
> >
> > --- a/hw/xen/xen-legacy-backend.c
> > +++ b/hw/xen/xen-legacy-backend.c
> > @@ -824,7 +825,7 @@ static void xen_sysdev_class_init(ObjectClass
> > *klass, void *data)
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >
> >      device_class_set_props(dc, xen_sysdev_properties);
> > -    dc->bus_type = TYPE_XENSYSBUS;
> > +    //dc->bus_type = TYPE_XENSYSBUS;
> >  }
> 
> Uff!
> 
> Let me explain how things are supposed to work.
> 
> Say we have FOO bus (QOM type TYPE_FOO_BUS), with FOO devices plugging
> into it (abstract QOM type TYPE_FOO_DEVICE).  One of them is SOME_FOO
> (concrete QOM type TYPE_SOME_FOO).  Code ties them together like this:
> 
>     static const TypeInfo pci_bus_info = {
>         .name = TYPE_PCI_BUS,
>         .parent = TYPE_BUS,
>         ...
>     };
> 
>     static const TypeInfo foo_device_info = {
>         .name = TYPE_FOO_DEVICE,
>         .parent = TYPE_DEVICE,
>         .abstract = true,
>         .class_init = foo_device_class_init,
>         ...
>     };
> 
>     static void foo_device_class_init(ObjectClass *oc, void *data)
>     {
>         DeviceClass *dc = DEVICE_CLASS(oc);
> 
>         dc->bus_type = TYPE_FOO_BUS;
>         ...
>     }
> 
>     static const TypeInfo some_foo_info = {
>         .name = TYPE_SOME_FOO,
>         .parent = TYPE_FOO_DEVICE,
>         ...
>     };
> 
> When you plug an instance of TYPE_SOME_FOO into a bus, the assertion
> checks that the bus is an instance of TYPE_FOO_BUS.
> 
> Note that subtypes of TYPE_FOO_DEVICE do not mess with dc->bus_type!
> 
> TYPE_XENSYSDEV does mess with it:
> 
>     static void xen_sysdev_class_init(ObjectClass *klass, void *data)
>     {
>         DeviceClass *dc = DEVICE_CLASS(klass);
> 
>         device_class_set_props(dc, xen_sysdev_properties);
>         dc->bus_type = TYPE_XENSYSBUS;
>     }
> 
>     static const TypeInfo xensysdev_info = {
>         .name          = TYPE_XENSYSDEV,
>         .parent        = TYPE_SYS_BUS_DEVICE,
>         .instance_size = sizeof(SysBusDevice),
>         .class_init    = xen_sysdev_class_init,
>     };
> 
> On the one hand, xensysdev_info.parent claims TYPE_XENSYSDEV is a
> TYPE_SYS_BUS_DEVICE (and therefore should plug into a TYPE_SYSTEM_BUS).
> On the other hand, its dc->bus_type is a TYPE_XENSYSBUS, which is *not*
> a subtype of TYPE_SYSTEM_BUS:
> 
>     static const TypeInfo xensysbus_info = {
>         .name       = TYPE_XENSYSBUS,
> --->    .parent     = TYPE_BUS,
>         .class_init = xen_sysbus_class_init,
>         .interfaces = (InterfaceInfo[]) {
>             { TYPE_HOTPLUG_HANDLER },
>             { }
>         }
>     };
> 
> This is an inconsistent mess.
> 
> Are TYPE_XENSYSDEV and TYPE_XENSYSBUS related to TYPE_SYS_BUS_DEVICE and
> TYPE_SYSTEM_BUS?
> 
> If no, then xensysbus_info.parent should not be TYPE_SYS_BUS_DEVICE, and
> you must not pass instances of one kind to functions expecting the other
> kind.
> 
> If yes, how?  If the former are specializations of the latter, consider
> making the former subtypes of the latter.  Both of them.  Then a
> TYPE_XENSYSDEV device can plug into a TYPE_XENSYSBUS bus, but not into a
> TYPE_SYSTEM_BUS bus.
> 
> A TYPE_SYS_BUS_DEVICE could still plug into TYPE_XENSYSBUS, because the
> latter is also an instance of TYPE_SYSTEM_BUS.
> 
> Questions?
> 
> >
> >  static const TypeInfo xensysdev_info = {
> >
> > Then I had a different instance of the failed assert trying to attach
> > xen-console-0 to xen-sysbus.  So I made this change:
> > --- a/hw/xen/xen-legacy-backend.c
> > +++ b/hw/xen/xen-legacy-backend.c
> > @@ -789,6 +789,7 @@ static void xendev_class_init(ObjectClass *klass,
> > void *data)
> >      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> >      /* xen-backend devices can be plugged/unplugged dynamically */
> >      dc->user_creatable = true;
> > +    dc->bus_type = TYPE_XENSYSBUS;
> >  }
> >
> >  static const TypeInfo xendev_type_info = {
> >
> > Then it gets farther... until
> > qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> > Assertion `dev->realized' failed.
> >
> > dev->id is NULL. The failing device is:
> > (gdb) p *dev.parent_obj.class.type
> > $12 = {name = 0x555556207770 "cfi.pflash01",
> >

Having commented out the call to xen_be_init() entirely (and xen_bus_init() for 
good measure) I also get this assertion failure, so
I don't think is related.

  Paul