sysbus failed assert for xen_sysdev

sysbus failed assert for xen_sysdev

[Jason Andryuk]

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?

Thanks,
Jason

Re: sysbus failed assert for xen_sysdev

[Mark Cave-Ayland]

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. A quick look
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?


ATB,

Mark.

Re: sysbus failed assert for xen_sysdev

[Jason Andryuk]

On Mon, Jun 22, 2020 at 5:17 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> 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. A quick look
> 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;
 }

 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

Re: sysbus failed assert for xen_sysdev

[Markus Armbruster]

Jason Andryuk <jandryuk@gmail.com> writes:

> On Mon, Jun 22, 2020 at 5:17 PM Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> 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",
>
> Is that right?
>
> I'm going to have to take a break from this now.
>
> Regards,
> Jason

RE: sysbus failed assert for xen_sysdev

[Paul Durrant]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: 23 June 2020 09:41
> To: Jason Andryuk <jandryuk@gmail.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>; Anthony PERARD <anthony.perard@citrix.com>; xen-
> devel <xen-devel@lists.xenproject.org>; Paul Durrant <paul@xen.org>; QEMU <qemu-devel@nongnu.org>
> Subject: Re: sysbus failed assert for xen_sysdev
> 
> Jason Andryuk <jandryuk@gmail.com> writes:
> 
> > On Mon, Jun 22, 2020 at 5:17 PM Mark Cave-Ayland
> > <mark.cave-ayland@ilande.co.uk> 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

Re: sysbus failed assert for xen_sysdev

[Jason Andryuk]

On Tue, Jun 23, 2020 at 7:46 AM Paul Durrant <xadimgnik@gmail.com> wrote:
>
> > -----Original Message-----
> > From: Markus Armbruster <armbru@redhat.com>
> > Sent: 23 June 2020 09:41
> > To: Jason Andryuk <jandryuk@gmail.com>
> > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>; Anthony PERARD <anthony.perard@citrix.com>; xen-
> > devel <xen-devel@lists.xenproject.org>; Paul Durrant <paul@xen.org>; QEMU <qemu-devel@nongnu.org>
> > Subject: Re: sysbus failed assert for xen_sysdev
> >
> > Jason Andryuk <jandryuk@gmail.com> writes:
> > > 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.

Yes, this is something different.  pc_pflash_create() calls
qdev_new(TYPE_PFLASH_CFI01), but it is only realized in
pc_system_flash_map()...  and pc_system_flash_map() isn't called for
Xen.

Removing the call to pc_system_flash_create() from pc_machine_initfn()
lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
there since accelerators have not been initialized yes, I guess?

Regards,
Jason

RE: sysbus failed assert for xen_sysdev

[Paul Durrant]

> -----Original Message-----
> From: Jason Andryuk <jandryuk@gmail.com>
> Sent: 24 June 2020 04:24
> To: Paul Durrant <paul@xen.org>
> Cc: Markus Armbruster <armbru@redhat.com>; Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>; Anthony
> PERARD <anthony.perard@citrix.com>; xen-devel <xen-devel@lists.xenproject.org>; QEMU <qemu-
> devel@nongnu.org>
> Subject: Re: sysbus failed assert for xen_sysdev
> 
> On Tue, Jun 23, 2020 at 7:46 AM Paul Durrant <xadimgnik@gmail.com> wrote:
> >
> > > -----Original Message-----
> > > From: Markus Armbruster <armbru@redhat.com>
> > > Sent: 23 June 2020 09:41
> > > To: Jason Andryuk <jandryuk@gmail.com>
> > > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>; Anthony PERARD <anthony.perard@citrix.com>;
> xen-
> > > devel <xen-devel@lists.xenproject.org>; Paul Durrant <paul@xen.org>; QEMU <qemu-devel@nongnu.org>
> > > Subject: Re: sysbus failed assert for xen_sysdev
> > >
> > > Jason Andryuk <jandryuk@gmail.com> writes:
> > > > 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.
> 
> Yes, this is something different.  pc_pflash_create() calls
> qdev_new(TYPE_PFLASH_CFI01), but it is only realized in
> pc_system_flash_map()...  and pc_system_flash_map() isn't called for
> Xen.
> 
> Removing the call to pc_system_flash_create() from pc_machine_initfn()
> lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
> there since accelerators have not been initialized yes, I guess?

Looks like it can be worked round by the following:

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1497d0e4ae..977d40afb8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
     if (!xen_enabled()) {
         pc_memory_init(pcms, system_memory,
                        rom_memory, &ram_memory);
-    } else if (machine->kernel_filename != NULL) {
-        /* For xen HVM direct kernel boot, load linux here */
-        xen_load_linux(pcms);
+    } else {
+        pc_system_flash_cleanup_unused(pcms);
+        if (machine->kernel_filename != NULL) {
+            /* For xen HVM direct kernel boot, load linux here */
+            xen_load_linux(pcms);
+        }
     }

     gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index ec2a3b3e7e..0ff47a4b59 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
     }
 }

-static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
+void pc_system_flash_cleanup_unused(PCMachineState *pcms)
 {
     char *prop_name;
     int i;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index e6135c34d6..497f2b7ab7 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);

 /* pc_sysfw.c */
 void pc_system_flash_create(PCMachineState *pcms);
+void pc_system_flash_cleanup_unused(PCMachineState *pcms);
 void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);

 /* acpi-build.c */


> 
> Regards,
> Jason

Re: sysbus failed assert for xen_sysdev

[Jason Andryuk]

On Wed, Jun 24, 2020 at 6:29 AM Paul Durrant <xadimgnik@gmail.com> wrote:
>
> > -----Original Message-----
> > From: Jason Andryuk <jandryuk@gmail.com>
> > Sent: 24 June 2020 04:24
> > To: Paul Durrant <paul@xen.org>
> > Cc: Markus Armbruster <armbru@redhat.com>; Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>; Anthony
> > PERARD <anthony.perard@citrix.com>; xen-devel <xen-devel@lists.xenproject.org>; QEMU <qemu-
> > devel@nongnu.org>
> > Subject: Re: sysbus failed assert for xen_sysdev
> >
> > On Tue, Jun 23, 2020 at 7:46 AM Paul Durrant <xadimgnik@gmail.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Markus Armbruster <armbru@redhat.com>
> > > > Sent: 23 June 2020 09:41
> > > > To: Jason Andryuk <jandryuk@gmail.com>
> > > > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>; Anthony PERARD <anthony.perard@citrix.com>;
> > xen-
> > > > devel <xen-devel@lists.xenproject.org>; Paul Durrant <paul@xen.org>; QEMU <qemu-devel@nongnu.org>
> > > > Subject: Re: sysbus failed assert for xen_sysdev
> > > >
> > > > Jason Andryuk <jandryuk@gmail.com> writes:
> > > > > 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.
> >
> > Yes, this is something different.  pc_pflash_create() calls
> > qdev_new(TYPE_PFLASH_CFI01), but it is only realized in
> > pc_system_flash_map()...  and pc_system_flash_map() isn't called for
> > Xen.
> >
> > Removing the call to pc_system_flash_create() from pc_machine_initfn()
> > lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
> > there since accelerators have not been initialized yes, I guess?
>
> Looks like it can be worked round by the following:

Yes, thank you.

Tested-by: Jason Andryuk <jandryuk@gmail.com>

> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 1497d0e4ae..977d40afb8 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
>      if (!xen_enabled()) {
>          pc_memory_init(pcms, system_memory,
>                         rom_memory, &ram_memory);
> -    } else if (machine->kernel_filename != NULL) {
> -        /* For xen HVM direct kernel boot, load linux here */
> -        xen_load_linux(pcms);
> +    } else {
> +        pc_system_flash_cleanup_unused(pcms);
> +        if (machine->kernel_filename != NULL) {
> +            /* For xen HVM direct kernel boot, load linux here */
> +            xen_load_linux(pcms);
> +        }
>      }
>
>      gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index ec2a3b3e7e..0ff47a4b59 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
>      }
>  }
>
> -static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> +void pc_system_flash_cleanup_unused(PCMachineState *pcms)
>  {
>      char *prop_name;
>      int i;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index e6135c34d6..497f2b7ab7 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
>
>  /* pc_sysfw.c */
>  void pc_system_flash_create(PCMachineState *pcms);
> +void pc_system_flash_cleanup_unused(PCMachineState *pcms);
>  void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>
>  /* acpi-build.c */
>
>
> >
> > Regards,
> > Jason
>

RE: sysbus failed assert for xen_sysdev

[Paul Durrant]

> -----Original Message-----
> From: Jason Andryuk <jandryuk@gmail.com>
> Sent: 24 June 2020 13:15
> To: Paul Durrant <paul@xen.org>
> Cc: Markus Armbruster <armbru@redhat.com>; Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>; Anthony
> PERARD <anthony.perard@citrix.com>; xen-devel <xen-devel@lists.xenproject.org>; QEMU <qemu-
> devel@nongnu.org>
> Subject: Re: sysbus failed assert for xen_sysdev
> 
> On Wed, Jun 24, 2020 at 6:29 AM Paul Durrant <xadimgnik@gmail.com> wrote:
> >
> > > -----Original Message-----
> > > From: Jason Andryuk <jandryuk@gmail.com>
> > > Sent: 24 June 2020 04:24
> > > To: Paul Durrant <paul@xen.org>
> > > Cc: Markus Armbruster <armbru@redhat.com>; Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>;
> Anthony
> > > PERARD <anthony.perard@citrix.com>; xen-devel <xen-devel@lists.xenproject.org>; QEMU <qemu-
> > > devel@nongnu.org>
> > > Subject: Re: sysbus failed assert for xen_sysdev
> > >
> > > On Tue, Jun 23, 2020 at 7:46 AM Paul Durrant <xadimgnik@gmail.com> wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Markus Armbruster <armbru@redhat.com>
> > > > > Sent: 23 June 2020 09:41
> > > > > To: Jason Andryuk <jandryuk@gmail.com>
> > > > > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>; Anthony PERARD
> <anthony.perard@citrix.com>;
> > > xen-
> > > > > devel <xen-devel@lists.xenproject.org>; Paul Durrant <paul@xen.org>; QEMU <qemu-
> devel@nongnu.org>
> > > > > Subject: Re: sysbus failed assert for xen_sysdev
> > > > >
> > > > > Jason Andryuk <jandryuk@gmail.com> writes:
> > > > > > 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.
> > >
> > > Yes, this is something different.  pc_pflash_create() calls
> > > qdev_new(TYPE_PFLASH_CFI01), but it is only realized in
> > > pc_system_flash_map()...  and pc_system_flash_map() isn't called for
> > > Xen.
> > >
> > > Removing the call to pc_system_flash_create() from pc_machine_initfn()
> > > lets QEMU startup and run a Xen HVM again.  xen_enabled() doesn't work
> > > there since accelerators have not been initialized yes, I guess?
> >
> > Looks like it can be worked round by the following:
> 
> Yes, thank you.
> 
> Tested-by: Jason Andryuk <jandryuk@gmail.com>

Thanks.

  Paul

> 
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 1497d0e4ae..977d40afb8 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
> >      if (!xen_enabled()) {
> >          pc_memory_init(pcms, system_memory,
> >                         rom_memory, &ram_memory);
> > -    } else if (machine->kernel_filename != NULL) {
> > -        /* For xen HVM direct kernel boot, load linux here */
> > -        xen_load_linux(pcms);
> > +    } else {
> > +        pc_system_flash_cleanup_unused(pcms);
> > +        if (machine->kernel_filename != NULL) {
> > +            /* For xen HVM direct kernel boot, load linux here */
> > +            xen_load_linux(pcms);
> > +        }
> >      }
> >
> >      gsi_state = pc_gsi_create(&x86ms->gsi, pcmc->pci_enabled);
> > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> > index ec2a3b3e7e..0ff47a4b59 100644
> > --- a/hw/i386/pc_sysfw.c
> > +++ b/hw/i386/pc_sysfw.c
> > @@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
> >      }
> >  }
> >
> > -static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> > +void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> >  {
> >      char *prop_name;
> >      int i;
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index e6135c34d6..497f2b7ab7 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
> >
> >  /* pc_sysfw.c */
> >  void pc_system_flash_create(PCMachineState *pcms);
> > +void pc_system_flash_cleanup_unused(PCMachineState *pcms);
> >  void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
> >
> >  /* acpi-build.c */
> >
> >
> > >
> > > Regards,
> > > Jason
> >

Re: sysbus failed assert for xen_sysdev

[Jason Andryuk]

On Tue, Jun 23, 2020 at 4:41 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Jason Andryuk <jandryuk@gmail.com> writes:
>
> > On Mon, Jun 22, 2020 at 5:17 PM Mark Cave-Ayland
> > <mark.cave-ayland@ilande.co.uk> 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?
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
>

RE: sysbus failed assert for xen_sysdev

[Paul Durrant]

> -----Original Message-----
> From: Jason Andryuk <jandryuk@gmail.com>
> Sent: 23 June 2020 13:57
> To: Markus Armbruster <armbru@redhat.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>; Anthony PERARD <anthony.perard@citrix.com>; xen-
> devel <xen-devel@lists.xenproject.org>; Paul Durrant <paul@xen.org>; QEMU <qemu-devel@nongnu.org>
> Subject: Re: sysbus failed assert for xen_sysdev
> 
> On Tue, Jun 23, 2020 at 4:41 AM Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Jason Andryuk <jandryuk@gmail.com> writes:
> >
> > > On Mon, Jun 22, 2020 at 5:17 PM Mark Cave-Ayland
> > > <mark.cave-ayland@ilande.co.uk> 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
> >

Re: sysbus failed assert for xen_sysdev

[Jason Andryuk]

On Tue, Jun 23, 2020 at 9:22 AM Paul Durrant <xadimgnik@gmail.com> wrote:
>
> > -----Original Message-----
> > From: Jason Andryuk <jandryuk@gmail.com>
> > Sent: 23 June 2020 13:57
> > To: Markus Armbruster <armbru@redhat.com>
> > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>; Anthony PERARD <anthony.perard@citrix.com>; xen-
> > devel <xen-devel@lists.xenproject.org>; Paul Durrant <paul@xen.org>; QEMU <qemu-devel@nongnu.org>
> > Subject: Re: sysbus failed assert for xen_sysdev

> >
> > 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.

Yes, xen-bus.c looks correct, but the important change seems to be
removing `dc->bus_type = TYPE_XENSYSBUS;` from xen_sysdev_class_init()
and adding it to xendev_class_init().  That let QEMU get to the
cfi.pflash01 assertion failure.

Regards,
Jason

>   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.