RE: sysbus failed assert for xen_sysdev

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jason Andryuk <jandryuk@xxxxxxxxx>
> Sent: 23 June 2020 13:57
> To: Markus Armbruster <armbru@xxxxxxxxxx>
> 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
> 
> On Tue, Jun 23, 2020 at 4:41 AM Markus Armbruster <armbru@xxxxxxxxxx> wrote:
> >
> > 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.
> 
> Thanks for your response, Markus.
> 
> I didn't write it, but my understanding is as follows.  TYPE_XENSYSDEV
> is a device on the system bus that provides the TYPE_XENSYSBUS bus.
> TYPE_XENBACKEND devices can then attach to TYPE_XENSYSBUS.
> 
> That would make the qom-tree something like:
>   /TYPE_XENSYSDEV
>     /TYPE_XENSYSBUX
>       /TYPE_XENBACKEND
> 
> (I think today the TYPE_XENBACKEND devices ends up attached to the System 
> bus.)
> 
> I think TYPE_XENSYSDEV is correct - it is a device on the system bus.
> static const TypeInfo xensysdev_info = {
> .name = TYPE_XENSYSDEV,
> .parent = TYPE_SYS_BUS_DEVICE,
> ...
> }
> 
> TYPE_XENSYSBUS is the xen-specific bus - provided by TYPE_XENSYSDEV -
> for attaching xendev.
> static const TypeInfo xensysbus_info = {
> .name = TYPE_XENSYSBUS,
> .parent = TYPE_BUS,
> ...
> }
> 
> TYPE_XENBACKEND is a generic Xen device and it plugs into
> TYPE_XENSYSBUS.  Maybe the .parent here is wrong and it should just be
> TYPE_DEVICE?

Yes, I think that is the problem leading to the assert. See the equivalent 
(non-legacy) code in xen-bus.c. 

  Paul

> static const TypeInfo xendev_type_info = {
> .name = TYPE_XENBACKEND,
> .parent = TYPE_XENSYSDEV,
> ...
> }
> 
> So removing `bus_type = TYPE_XENSYSBUS` from TYPE_XENSYSDEV class_init
> and adding it to TYPE_XENBACKEND seems correct to me.
> 
> Regards,
> Jason
> 
> > 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",
> > >
> > > Is that right?
> > >
> > > I'm going to have to take a break from this now.
> > >
> > > Regards,
> > > Jason
> >