The xen-backend devices created by the Xen code are not supposed
to be treated as dynamic sysbus devices. This is an attempt to
change that and see what happens, but I couldn't test it because
I don't have a Xen host set up.

If this patch breaks anything, this means we have a bug in
foreach_dynamic_sysbus_device(), which is supposed to return only
devices created using -device.

The original code that sets has_dynamic_sysbus was added by
commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see
any comment explaining why it was necessary.

Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/xen/xen_backend.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index 6c21c37d68..4607d6d3ee 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -550,15 +550,6 @@ err:
     return -1;
 }
 
-static void xen_set_dynamic_sysbus(void)
-{
-    Object *machine = qdev_get_machine();
-    ObjectClass *oc = object_get_class(machine);
-    MachineClass *mc = MACHINE_CLASS(oc);
-
-    mc->has_dynamic_sysbus = true;
-}
-
 int xen_be_register(const char *type, struct XenDevOps *ops)
 {
     char path[50];
@@ -580,8 +571,6 @@ int xen_be_register(const char *type, struct XenDevOps *ops)
 
 void xen_be_register_common(void)
 {
-    xen_set_dynamic_sysbus();
-
     xen_be_register("console", &xen_console_ops);
     xen_be_register("vkbd", &xen_kbdmouse_ops);
     xen_be_register("qdisk", &xen_blkdev_ops);
-- 
2.11.0.259.g40922b1

On 23/03/17 22:28, Eduardo Habkost wrote:
> The xen-backend devices created by the Xen code are not supposed
> to be treated as dynamic sysbus devices. This is an attempt to
> change that and see what happens, but I couldn't test it because
> I don't have a Xen host set up.
> 
> If this patch breaks anything, this means we have a bug in
> foreach_dynamic_sysbus_device(), which is supposed to return only
> devices created using -device.
> 
> The original code that sets has_dynamic_sysbus was added by
> commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see
> any comment explaining why it was necessary.

xen-backend devices are created via qmp commands when attaching new
pv-devices to a domain. They can be dynamically removed, too. Setting
has_dynamic_sysbus was necessary to support this feature.

So just removing it will break Xen.

NAK as a standalone patch.


Juergen

> 
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/xen/xen_backend.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> index 6c21c37d68..4607d6d3ee 100644
> --- a/hw/xen/xen_backend.c
> +++ b/hw/xen/xen_backend.c
> @@ -550,15 +550,6 @@ err:
>      return -1;
>  }
>  
> -static void xen_set_dynamic_sysbus(void)
> -{
> -    Object *machine = qdev_get_machine();
> -    ObjectClass *oc = object_get_class(machine);
> -    MachineClass *mc = MACHINE_CLASS(oc);
> -
> -    mc->has_dynamic_sysbus = true;
> -}
> -
>  int xen_be_register(const char *type, struct XenDevOps *ops)
>  {
>      char path[50];
> @@ -580,8 +571,6 @@ int xen_be_register(const char *type, struct XenDevOps *ops)
>  
>  void xen_be_register_common(void)
>  {
> -    xen_set_dynamic_sysbus();
> -
>      xen_be_register("console", &xen_console_ops);
>      xen_be_register("vkbd", &xen_kbdmouse_ops);
>      xen_be_register("qdisk", &xen_blkdev_ops);
> 

On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
> On 23/03/17 22:28, Eduardo Habkost wrote:
>> The xen-backend devices created by the Xen code are not supposed
>> to be treated as dynamic sysbus devices. This is an attempt to
>> change that and see what happens, but I couldn't test it because
>> I don't have a Xen host set up.
>>
>> If this patch breaks anything, this means we have a bug in
>> foreach_dynamic_sysbus_device(), which is supposed to return only
>> devices created using -device.
>>
>> The original code that sets has_dynamic_sysbus was added by
>> commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see
>> any comment explaining why it was necessary.
>
> xen-backend devices are created via qmp commands when attaching new
> pv-devices to a domain. They can be dynamically removed, too. Setting
> has_dynamic_sysbus was necessary to support this feature.

This seems like it ought to be handled by marking the xen-backend
devices as being ok-to-dynamically-create somehow, not by marking
the machine as supporting dynamic-sysbus (which it doesn't).
Maybe we don't have the necessary support code to do that though?

thanks
-- PMM

On 24/03/17 11:09, Peter Maydell wrote:
> On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
>> On 23/03/17 22:28, Eduardo Habkost wrote:
>>> The xen-backend devices created by the Xen code are not supposed
>>> to be treated as dynamic sysbus devices. This is an attempt to
>>> change that and see what happens, but I couldn't test it because
>>> I don't have a Xen host set up.
>>>
>>> If this patch breaks anything, this means we have a bug in
>>> foreach_dynamic_sysbus_device(), which is supposed to return only
>>> devices created using -device.
>>>
>>> The original code that sets has_dynamic_sysbus was added by
>>> commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see
>>> any comment explaining why it was necessary.
>>
>> xen-backend devices are created via qmp commands when attaching new
>> pv-devices to a domain. They can be dynamically removed, too. Setting
>> has_dynamic_sysbus was necessary to support this feature.
> 
> This seems like it ought to be handled by marking the xen-backend
> devices as being ok-to-dynamically-create somehow, not by marking
> the machine as supporting dynamic-sysbus (which it doesn't).
> Maybe we don't have the necessary support code to do that though?

When writing the patches I couldn't find a way to do it differently.
OTOH I'm not so deep in qemu internals I'd be able to add the needed
support.

I'd be happy to test any patch, though.


Juergen

On Fri, Mar 24, 2017 at 11:24:31AM +0100, Juergen Gross wrote:
> On 24/03/17 11:09, Peter Maydell wrote:
> > On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
> >> On 23/03/17 22:28, Eduardo Habkost wrote:
> >>> The xen-backend devices created by the Xen code are not supposed
> >>> to be treated as dynamic sysbus devices. This is an attempt to
> >>> change that and see what happens, but I couldn't test it because
> >>> I don't have a Xen host set up.
> >>>
> >>> If this patch breaks anything, this means we have a bug in
> >>> foreach_dynamic_sysbus_device(), which is supposed to return only
> >>> devices created using -device.
> >>>
> >>> The original code that sets has_dynamic_sysbus was added by
> >>> commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see
> >>> any comment explaining why it was necessary.
> >>
> >> xen-backend devices are created via qmp commands when attaching new
> >> pv-devices to a domain. They can be dynamically removed, too. Setting
> >> has_dynamic_sysbus was necessary to support this feature.
> > 
> > This seems like it ought to be handled by marking the xen-backend
> > devices as being ok-to-dynamically-create somehow, not by marking
> > the machine as supporting dynamic-sysbus (which it doesn't).
> > Maybe we don't have the necessary support code to do that though?
> 
> When writing the patches I couldn't find a way to do it differently.
> OTOH I'm not so deep in qemu internals I'd be able to add the needed
> support.
> 
> I'd be happy to test any patch, though.

If xen-backend devices are created via QMP commands, then
has_dynamic_sysbus is (currently) really needed, although I would
have preferred to set it on all x86 machines instead of changing
MachineClass::has_dynamic_sysbus outside class_init.

But with the new whitelist implemented by this series, we could
simply include xen-backend in the whitelist for the machines that
can be used with Xen, and get rid of xen_set_dynamic_sysbus().

I assume plugging/unplugging xen-backend devices apply to both
xen{pv,fv} and pc,accel=xen, right? Do we need to make it work
with "-machine none,accel=xen" and "-machine isapc" too?

-- 
Eduardo

On 24/03/17 12:10, Eduardo Habkost wrote:
> On Fri, Mar 24, 2017 at 11:24:31AM +0100, Juergen Gross wrote:
>> On 24/03/17 11:09, Peter Maydell wrote:
>>> On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
>>>> On 23/03/17 22:28, Eduardo Habkost wrote:
>>>>> The xen-backend devices created by the Xen code are not supposed
>>>>> to be treated as dynamic sysbus devices. This is an attempt to
>>>>> change that and see what happens, but I couldn't test it because
>>>>> I don't have a Xen host set up.
>>>>>
>>>>> If this patch breaks anything, this means we have a bug in
>>>>> foreach_dynamic_sysbus_device(), which is supposed to return only
>>>>> devices created using -device.
>>>>>
>>>>> The original code that sets has_dynamic_sysbus was added by
>>>>> commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see
>>>>> any comment explaining why it was necessary.
>>>>
>>>> xen-backend devices are created via qmp commands when attaching new
>>>> pv-devices to a domain. They can be dynamically removed, too. Setting
>>>> has_dynamic_sysbus was necessary to support this feature.
>>>
>>> This seems like it ought to be handled by marking the xen-backend
>>> devices as being ok-to-dynamically-create somehow, not by marking
>>> the machine as supporting dynamic-sysbus (which it doesn't).
>>> Maybe we don't have the necessary support code to do that though?
>>
>> When writing the patches I couldn't find a way to do it differently.
>> OTOH I'm not so deep in qemu internals I'd be able to add the needed
>> support.
>>
>> I'd be happy to test any patch, though.
> 
> If xen-backend devices are created via QMP commands, then
> has_dynamic_sysbus is (currently) really needed, although I would
> have preferred to set it on all x86 machines instead of changing
> MachineClass::has_dynamic_sysbus outside class_init.
> 
> But with the new whitelist implemented by this series, we could
> simply include xen-backend in the whitelist for the machines that
> can be used with Xen, and get rid of xen_set_dynamic_sysbus().
> 
> I assume plugging/unplugging xen-backend devices apply to both
> xen{pv,fv} and pc,accel=xen, right? Do we need to make it work
> with "-machine none,accel=xen" and "-machine isapc" too?

AFAIK -xenpv, -xenfv and -pc,accel=xen are the only machine types
to support. Wouldn't it make sense to do the whitelisting in
xen_be_register_common() in spite of setting has_dynamic_sysbus?


Juergen

On Fri, Mar 24, 2017 at 01:27:31PM +0100, Juergen Gross wrote:
> On 24/03/17 12:10, Eduardo Habkost wrote:
> > On Fri, Mar 24, 2017 at 11:24:31AM +0100, Juergen Gross wrote:
> >> On 24/03/17 11:09, Peter Maydell wrote:
> >>> On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
> >>>> On 23/03/17 22:28, Eduardo Habkost wrote:
> >>>>> The xen-backend devices created by the Xen code are not supposed
> >>>>> to be treated as dynamic sysbus devices. This is an attempt to
> >>>>> change that and see what happens, but I couldn't test it because
> >>>>> I don't have a Xen host set up.
> >>>>>
> >>>>> If this patch breaks anything, this means we have a bug in
> >>>>> foreach_dynamic_sysbus_device(), which is supposed to return only
> >>>>> devices created using -device.
> >>>>>
> >>>>> The original code that sets has_dynamic_sysbus was added by
> >>>>> commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see
> >>>>> any comment explaining why it was necessary.
> >>>>
> >>>> xen-backend devices are created via qmp commands when attaching new
> >>>> pv-devices to a domain. They can be dynamically removed, too. Setting
> >>>> has_dynamic_sysbus was necessary to support this feature.
> >>>
> >>> This seems like it ought to be handled by marking the xen-backend
> >>> devices as being ok-to-dynamically-create somehow, not by marking
> >>> the machine as supporting dynamic-sysbus (which it doesn't).
> >>> Maybe we don't have the necessary support code to do that though?
> >>
> >> When writing the patches I couldn't find a way to do it differently.
> >> OTOH I'm not so deep in qemu internals I'd be able to add the needed
> >> support.
> >>
> >> I'd be happy to test any patch, though.
> > 
> > If xen-backend devices are created via QMP commands, then
> > has_dynamic_sysbus is (currently) really needed, although I would
> > have preferred to set it on all x86 machines instead of changing
> > MachineClass::has_dynamic_sysbus outside class_init.
> > 
> > But with the new whitelist implemented by this series, we could
> > simply include xen-backend in the whitelist for the machines that
> > can be used with Xen, and get rid of xen_set_dynamic_sysbus().
> > 
> > I assume plugging/unplugging xen-backend devices apply to both
> > xen{pv,fv} and pc,accel=xen, right? Do we need to make it work
> > with "-machine none,accel=xen" and "-machine isapc" too?
> 
> AFAIK -xenpv, -xenfv and -pc,accel=xen are the only machine types
> to support. Wouldn't it make sense to do the whitelisting in
> xen_be_register_common() in spite of setting has_dynamic_sysbus?

It would, but that would mean we would make the whitelisting
mechanism more complex: in addition to the static
per-machine-class whitelist, we would need a runtime whitelist.

This would make the interface for querying available/supported
device types more complex and messier, and I would like to avoid
that.

-- 
Eduardo

On 24/03/17 14:50, Eduardo Habkost wrote:
> On Fri, Mar 24, 2017 at 01:27:31PM +0100, Juergen Gross wrote:
>> On 24/03/17 12:10, Eduardo Habkost wrote:
>>> On Fri, Mar 24, 2017 at 11:24:31AM +0100, Juergen Gross wrote:
>>>> On 24/03/17 11:09, Peter Maydell wrote:
>>>>> On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
>>>>>> On 23/03/17 22:28, Eduardo Habkost wrote:
>>>>>>> The xen-backend devices created by the Xen code are not supposed
>>>>>>> to be treated as dynamic sysbus devices. This is an attempt to
>>>>>>> change that and see what happens, but I couldn't test it because
>>>>>>> I don't have a Xen host set up.
>>>>>>>
>>>>>>> If this patch breaks anything, this means we have a bug in
>>>>>>> foreach_dynamic_sysbus_device(), which is supposed to return only
>>>>>>> devices created using -device.
>>>>>>>
>>>>>>> The original code that sets has_dynamic_sysbus was added by
>>>>>>> commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see
>>>>>>> any comment explaining why it was necessary.
>>>>>>
>>>>>> xen-backend devices are created via qmp commands when attaching new
>>>>>> pv-devices to a domain. They can be dynamically removed, too. Setting
>>>>>> has_dynamic_sysbus was necessary to support this feature.
>>>>>
>>>>> This seems like it ought to be handled by marking the xen-backend
>>>>> devices as being ok-to-dynamically-create somehow, not by marking
>>>>> the machine as supporting dynamic-sysbus (which it doesn't).
>>>>> Maybe we don't have the necessary support code to do that though?
>>>>
>>>> When writing the patches I couldn't find a way to do it differently.
>>>> OTOH I'm not so deep in qemu internals I'd be able to add the needed
>>>> support.
>>>>
>>>> I'd be happy to test any patch, though.
>>>
>>> If xen-backend devices are created via QMP commands, then
>>> has_dynamic_sysbus is (currently) really needed, although I would
>>> have preferred to set it on all x86 machines instead of changing
>>> MachineClass::has_dynamic_sysbus outside class_init.
>>>
>>> But with the new whitelist implemented by this series, we could
>>> simply include xen-backend in the whitelist for the machines that
>>> can be used with Xen, and get rid of xen_set_dynamic_sysbus().
>>>
>>> I assume plugging/unplugging xen-backend devices apply to both
>>> xen{pv,fv} and pc,accel=xen, right? Do we need to make it work
>>> with "-machine none,accel=xen" and "-machine isapc" too?
>>
>> AFAIK -xenpv, -xenfv and -pc,accel=xen are the only machine types
>> to support. Wouldn't it make sense to do the whitelisting in
>> xen_be_register_common() in spite of setting has_dynamic_sysbus?
> 
> It would, but that would mean we would make the whitelisting
> mechanism more complex: in addition to the static
> per-machine-class whitelist, we would need a runtime whitelist.
> 
> This would make the interface for querying available/supported
> device types more complex and messier, and I would like to avoid
> that.
> 

Okay, understood. And I suppose you don't want to add the Xen
devices to the per-machine-class whitelist (after all my patch
did the same with the has_dynamic_sysbus flag) on demand.

Either way is fine with me.


Juergen

On 24.03.2017 11:09, Peter Maydell wrote:
> On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
>> On 23/03/17 22:28, Eduardo Habkost wrote:
>>> The xen-backend devices created by the Xen code are not supposed
>>> to be treated as dynamic sysbus devices. This is an attempt to
>>> change that and see what happens, but I couldn't test it because
>>> I don't have a Xen host set up.
>>>
>>> If this patch breaks anything, this means we have a bug in
>>> foreach_dynamic_sysbus_device(), which is supposed to return only
>>> devices created using -device.
>>>
>>> The original code that sets has_dynamic_sysbus was added by
>>> commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see
>>> any comment explaining why it was necessary.
>>
>> xen-backend devices are created via qmp commands when attaching new
>> pv-devices to a domain. They can be dynamically removed, too. Setting
>> has_dynamic_sysbus was necessary to support this feature.
> 
> This seems like it ought to be handled by marking the xen-backend
> devices as being ok-to-dynamically-create somehow, not by marking
> the machine as supporting dynamic-sysbus (which it doesn't).
> Maybe we don't have the necessary support code to do that though?

Do the xen devices have to be sysbus devices? If no, I think you could
change them to be of type TYPE_DEVICE nowadays - TYPE_DEVICE can be
instantiated with "-device" nowadays, too. That's what we do with the
spapr-rng device for example (see hw/ppc/spapr_rng.c).

 Thomas