* [Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist @ 2017-03-23 21:28 Eduardo Habkost 2017-03-23 21:28 ` [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class Eduardo Habkost ` (4 more replies) 0 siblings, 5 replies; 14+ messages in thread From: Eduardo Habkost @ 2017-03-23 21:28 UTC (permalink / raw) To: qemu-devel Cc: jgross, Thomas Huth, sstabellini, Markus Armbruster, Laszlo Ersek, Marcel Apfelbaum Summary ------- This series replaces the existing has_dynamic_sysbus flag (that makes the machine accept every single sysbus device type on the command-line) with a short whitelist. This will be helpful when implementing the new query-device-slots command, because each machine type will include only the sysbus devices it really supports, instead of including a catch-all TYPE_SYS_BUS_DEVICE "slot". Most of the machines already have an internal whitelist implemented using foreach_dynamic_sysbus_device(), so we just encode the existing behavior inside the new MachineClass field. Caveat: q35 ----------- The only problematic case is q35, that accepts all sysbus device types. In this case, instead of adding TYPE_SYS_BUS_DEVICE to the whitelist, I'm adding the existing list of existing sysbus device types to keep compatibility. After that, we can gradually and carefully remove unnecessary entries from the q35 whitelist. Issue: xen_set_dynamic_sysbus() hack ------------------------------------ We still have an obstacle to get around: the callers of qdev_set_id() outside qdev_device_add() are breaking foreach_dynamic_sysbus_device(), and xen_set_dynamic_sysbus() is a workaround for that bug. We now need to fix that bug to be able to remove the xen_set_dynamic_sysbus() hack. This means the first patch of this series will probably break Xen, until we fix that bug. Eduardo Habkost (4): [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class machine: Replace has_dynamic_sysbus with a whitelist q35: Remove ioapic devices from sysbus whitelist q35: Remove fw_cfg* from sysbus whitelist include/hw/arm/sysbus-fdt.h | 7 +++++++ include/hw/boards.h | 3 ++- hw/arm/sysbus-fdt.c | 10 ++++++++++ hw/arm/virt.c | 4 +++- hw/core/machine.c | 43 +++++++++++++++++++++++++++++-------------- hw/i386/pc_q35.c | 25 ++++++++++++++++++++++++- hw/ppc/e500plat.c | 4 +++- hw/ppc/spapr.c | 2 +- hw/xen/xen_backend.c | 11 ----------- 9 files changed, 79 insertions(+), 30 deletions(-) -- 2.11.0.259.g40922b1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class 2017-03-23 21:28 [Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist Eduardo Habkost @ 2017-03-23 21:28 ` Eduardo Habkost 2017-03-24 8:23 ` Juergen Gross 2017-03-23 21:28 ` [Qemu-devel] [RFC 2/4] machine: Replace has_dynamic_sysbus with a whitelist Eduardo Habkost ` (3 subsequent siblings) 4 siblings, 1 reply; 14+ messages in thread From: Eduardo Habkost @ 2017-03-23 21:28 UTC (permalink / raw) To: qemu-devel Cc: jgross, Thomas Huth, sstabellini, Markus Armbruster, Laszlo Ersek, Marcel Apfelbaum 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class 2017-03-23 21:28 ` [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class Eduardo Habkost @ 2017-03-24 8:23 ` Juergen Gross 2017-03-24 10:09 ` Peter Maydell 0 siblings, 1 reply; 14+ messages in thread From: Juergen Gross @ 2017-03-24 8:23 UTC (permalink / raw) To: Eduardo Habkost, qemu-devel Cc: Thomas Huth, sstabellini, Markus Armbruster, Laszlo Ersek, Marcel Apfelbaum 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); > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class 2017-03-24 8:23 ` Juergen Gross @ 2017-03-24 10:09 ` Peter Maydell 2017-03-24 10:24 ` Juergen Gross 2017-03-24 10:32 ` Thomas Huth 0 siblings, 2 replies; 14+ messages in thread From: Peter Maydell @ 2017-03-24 10:09 UTC (permalink / raw) To: Juergen Gross Cc: Eduardo Habkost, QEMU Developers, Marcel Apfelbaum, Thomas Huth, Stefano Stabellini, Laszlo Ersek, Markus Armbruster 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class 2017-03-24 10:09 ` Peter Maydell @ 2017-03-24 10:24 ` Juergen Gross 2017-03-24 11:10 ` Eduardo Habkost 2017-03-24 10:32 ` Thomas Huth 1 sibling, 1 reply; 14+ messages in thread From: Juergen Gross @ 2017-03-24 10:24 UTC (permalink / raw) To: Peter Maydell Cc: Eduardo Habkost, QEMU Developers, Marcel Apfelbaum, Thomas Huth, Stefano Stabellini, Laszlo Ersek, Markus Armbruster 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class 2017-03-24 10:24 ` Juergen Gross @ 2017-03-24 11:10 ` Eduardo Habkost 2017-03-24 12:27 ` Juergen Gross 0 siblings, 1 reply; 14+ messages in thread From: Eduardo Habkost @ 2017-03-24 11:10 UTC (permalink / raw) To: Juergen Gross Cc: Peter Maydell, QEMU Developers, Marcel Apfelbaum, Thomas Huth, Stefano Stabellini, Laszlo Ersek, Markus Armbruster 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class 2017-03-24 11:10 ` Eduardo Habkost @ 2017-03-24 12:27 ` Juergen Gross 2017-03-24 13:50 ` Eduardo Habkost 0 siblings, 1 reply; 14+ messages in thread From: Juergen Gross @ 2017-03-24 12:27 UTC (permalink / raw) To: Eduardo Habkost Cc: Peter Maydell, QEMU Developers, Marcel Apfelbaum, Thomas Huth, Stefano Stabellini, Laszlo Ersek, Markus Armbruster 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class 2017-03-24 12:27 ` Juergen Gross @ 2017-03-24 13:50 ` Eduardo Habkost 2017-03-24 13:59 ` Juergen Gross 0 siblings, 1 reply; 14+ messages in thread From: Eduardo Habkost @ 2017-03-24 13:50 UTC (permalink / raw) To: Juergen Gross Cc: Peter Maydell, QEMU Developers, Marcel Apfelbaum, Thomas Huth, Stefano Stabellini, Laszlo Ersek, Markus Armbruster 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class 2017-03-24 13:50 ` Eduardo Habkost @ 2017-03-24 13:59 ` Juergen Gross 0 siblings, 0 replies; 14+ messages in thread From: Juergen Gross @ 2017-03-24 13:59 UTC (permalink / raw) To: Eduardo Habkost Cc: Peter Maydell, QEMU Developers, Marcel Apfelbaum, Thomas Huth, Stefano Stabellini, Laszlo Ersek, Markus Armbruster 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class 2017-03-24 10:09 ` Peter Maydell 2017-03-24 10:24 ` Juergen Gross @ 2017-03-24 10:32 ` Thomas Huth 1 sibling, 0 replies; 14+ messages in thread From: Thomas Huth @ 2017-03-24 10:32 UTC (permalink / raw) To: Peter Maydell, Juergen Gross Cc: Eduardo Habkost, QEMU Developers, Marcel Apfelbaum, Stefano Stabellini, Laszlo Ersek, Markus Armbruster 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [RFC 2/4] machine: Replace has_dynamic_sysbus with a whitelist 2017-03-23 21:28 [Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist Eduardo Habkost 2017-03-23 21:28 ` [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class Eduardo Habkost @ 2017-03-23 21:28 ` Eduardo Habkost 2017-03-23 21:28 ` [Qemu-devel] [RFC 3/4] q35: Remove ioapic devices from sysbus whitelist Eduardo Habkost ` (2 subsequent siblings) 4 siblings, 0 replies; 14+ messages in thread From: Eduardo Habkost @ 2017-03-23 21:28 UTC (permalink / raw) To: qemu-devel Cc: jgross, Thomas Huth, sstabellini, Markus Armbruster, Laszlo Ersek, Marcel Apfelbaum Replace the existing has_dynamic_sysbus flag, that makes the machine accept every single sysbus device type on the command-line, with a short whitelist. Most of the machines already have an internal whitelist implemented using foreach_dynamic_sysbus_device(), so we just encode the existing behavior inside the new MachineClass field. The only problematic case is q35, that accepts all sysbus device types. In this case, instad of adding TYPE_SYS_BUS_DEVICE to the whitelist, add the existing list of existing sysbus device types to keep compatibility. After that, we can gradually and carefully remove unnecessary entries from the q35 whitelist. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- include/hw/arm/sysbus-fdt.h | 7 +++++++ include/hw/boards.h | 3 ++- hw/arm/sysbus-fdt.c | 10 ++++++++++ hw/arm/virt.c | 4 +++- hw/core/machine.c | 43 +++++++++++++++++++++++++++++-------------- hw/i386/pc_q35.c | 29 ++++++++++++++++++++++++++++- hw/ppc/e500plat.c | 4 +++- hw/ppc/spapr.c | 2 +- 8 files changed, 83 insertions(+), 19 deletions(-) diff --git a/include/hw/arm/sysbus-fdt.h b/include/hw/arm/sysbus-fdt.h index e15bb81807..071124e13d 100644 --- a/include/hw/arm/sysbus-fdt.h +++ b/include/hw/arm/sysbus-fdt.h @@ -57,4 +57,11 @@ typedef struct { */ void arm_register_platform_bus_fdt_creator(ARMPlatformBusFDTParams *fdt_params); +/** + * arm_register_fdt_sysbus_whitelist - register supported sysbus device types + * + * Add supported dynamic sysbus device types to machine class. + */ +void arm_register_fdt_sysbus_whitelist(MachineClass *mc); + #endif diff --git a/include/hw/boards.h b/include/hw/boards.h index 269d0ba399..6576f6a5b0 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -42,6 +42,7 @@ bool machine_dump_guest_core(MachineState *machine); bool machine_mem_merge(MachineState *machine); void machine_register_compat_props(MachineState *machine); HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine); +void machine_class_add_sysbus_whitelist(MachineClass *mc, const char *type); /** * CPUArchId: @@ -121,7 +122,6 @@ struct MachineClass { no_floppy:1, no_cdrom:1, no_sdcard:1, - has_dynamic_sysbus:1, pci_allow_0_address:1, legacy_fw_cfg_order:1; int is_default; @@ -135,6 +135,7 @@ struct MachineClass { bool rom_file_has_mr; int minimum_page_bits; bool has_hotpluggable_cpus; + strList *dynamic_sysbus_whitelist; HotplugHandler *(*get_hotplug_handler)(MachineState *machine, DeviceState *dev); diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c index d68e3dcdbd..29573a4412 100644 --- a/hw/arm/sysbus-fdt.c +++ b/hw/arm/sysbus-fdt.c @@ -29,6 +29,7 @@ #include <linux/vfio.h> #endif #include "hw/arm/sysbus-fdt.h" +#include "hw/boards.h" #include "qemu/error-report.h" #include "sysemu/device_tree.h" #include "hw/platform-bus.h" @@ -424,6 +425,15 @@ static const NodeCreationPair add_fdt_node_functions[] = { {"", NULL}, /* last element */ }; +void arm_register_fdt_sysbus_whitelist(MachineClass *mc) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(add_fdt_node_functions); i++) { + machine_class_add_sysbus_whitelist(mc, add_fdt_node_functions[i].typename); + } +} + /* Generic Code */ /** diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 5f62a0321e..293710a6ea 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -52,6 +52,8 @@ #include "hw/arm/fdt.h" #include "hw/intc/arm_gic.h" #include "hw/intc/arm_gicv3_common.h" +#include "hw/vfio/vfio-calxeda-xgmac.h" +#include "hw/vfio/vfio-amd-xgbe.h" #include "kvm_arm.h" #include "hw/smbios/smbios.h" #include "qapi/visitor.h" @@ -1528,7 +1530,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data) * configuration of the particular instance. */ mc->max_cpus = 255; - mc->has_dynamic_sysbus = true; + arm_register_fdt_sysbus_whitelist(mc); mc->block_default_type = IF_VIRTIO; mc->no_cdrom = 1; mc->pci_allow_0_address = true; diff --git a/hw/core/machine.c b/hw/core/machine.c index 0d92672203..606edffc38 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -332,29 +332,44 @@ static bool machine_get_enforce_config_section(Object *obj, Error **errp) return ms->enforce_config_section; } -static void error_on_sysbus_device(SysBusDevice *sbdev, void *opaque) +void machine_class_add_sysbus_whitelist(MachineClass *mc, const char *type) { - error_report("Option '-device %s' cannot be handled by this machine", - object_class_get_name(object_get_class(OBJECT(sbdev)))); - exit(1); + strList *item = g_new0(strList, 1); + + item->value = g_strdup(type); + item->next = mc->dynamic_sysbus_whitelist; + mc->dynamic_sysbus_whitelist = item; } -static void machine_init_notify(Notifier *notifier, void *data) +static void validate_sysbus_device(SysBusDevice *sbdev, void *opaque) { - Object *machine = qdev_get_machine(); - ObjectClass *oc = object_get_class(machine); - MachineClass *mc = MACHINE_CLASS(oc); + MachineState *machine = opaque; + MachineClass *mc = MACHINE_GET_CLASS(machine); + bool whitelisted = false; + strList *wl; - if (mc->has_dynamic_sysbus) { - /* Our machine can handle dynamic sysbus devices, we're all good */ - return; + for (wl = mc->dynamic_sysbus_whitelist; + !whitelisted && wl; + wl = wl->next) { + whitelisted |= !!object_dynamic_cast(OBJECT(sbdev), wl->value); } + if (!whitelisted) { + error_report("Option '-device %s' cannot be handled by this machine", + object_class_get_name(object_get_class(OBJECT(sbdev)))); + exit(1); + } +} + +static void machine_init_notify(Notifier *notifier, void *data) +{ + MachineState *machine = MACHINE(qdev_get_machine()); + /* - * Loop through all dynamically created devices and check whether there - * are sysbus devices among them. If there are, error out. + * Loop through all dynamically created sysbus devices and check if they are + * all whitelisted. If there are non-whitelisted devices, error out. */ - foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL); + foreach_dynamic_sysbus_device(validate_sysbus_device, machine); } HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine) diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index dd792a8547..ea039540d8 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -297,7 +297,34 @@ static void pc_q35_machine_options(MachineClass *m) m->default_machine_opts = "firmware=bios-256k.bin"; m->default_display = "std"; m->no_floppy = 1; - m->has_dynamic_sysbus = true; + /* The following whitelist is based on the set of available + * sysbus devices when has_dynamic_sysbus was replaced with + * a whitelist. Not every single device in this list is really + * supposed to be supported, but they are kept here for compatibility. + * We can gradually remove unnecessary devices from the whitelist. + */ + machine_class_add_sysbus_whitelist(m, "allwinner-ahci"); + machine_class_add_sysbus_whitelist(m, "amd-iommu"); + machine_class_add_sysbus_whitelist(m, "cfi.pflash01"); + machine_class_add_sysbus_whitelist(m, "esp"); + machine_class_add_sysbus_whitelist(m, "fw_cfg_io"); + machine_class_add_sysbus_whitelist(m, "fw_cfg_mem"); + machine_class_add_sysbus_whitelist(m, "generic-sdhci"); + machine_class_add_sysbus_whitelist(m, "hpet"); + machine_class_add_sysbus_whitelist(m, "intel-iommu"); + machine_class_add_sysbus_whitelist(m, "ioapic"); + machine_class_add_sysbus_whitelist(m, "isabus-bridge"); + machine_class_add_sysbus_whitelist(m, "kvm-ioapic"); + machine_class_add_sysbus_whitelist(m, "kvmclock"); + machine_class_add_sysbus_whitelist(m, "kvmvapic"); + machine_class_add_sysbus_whitelist(m, "SUNW,fdtwo"); + machine_class_add_sysbus_whitelist(m, "sysbus-ahci"); + machine_class_add_sysbus_whitelist(m, "sysbus-fdc"); + machine_class_add_sysbus_whitelist(m, "sysbus-ohci"); + machine_class_add_sysbus_whitelist(m, "unimplemented-device"); + machine_class_add_sysbus_whitelist(m, "virtio-mmio"); + machine_class_add_sysbus_whitelist(m, "xen-backend"); + machine_class_add_sysbus_whitelist(m, "xen-sysdev"); m->max_cpus = 288; } diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c index 94b454551f..3abed05add 100644 --- a/hw/ppc/e500plat.c +++ b/hw/ppc/e500plat.c @@ -12,7 +12,9 @@ #include "qemu/osdep.h" #include "qemu-common.h" #include "e500.h" +#include "hw/net/fsl_etsec/etsec.h" #include "hw/boards.h" +#include "hw/sysbus.h" #include "sysemu/device_tree.h" #include "sysemu/kvm.h" #include "hw/pci/pci.h" @@ -63,7 +65,7 @@ static void e500plat_machine_init(MachineClass *mc) mc->desc = "generic paravirt e500 platform"; mc->init = e500plat_init; mc->max_cpus = 32; - mc->has_dynamic_sysbus = true; + machine_class_add_sysbus_whitelist(mc, TYPE_ETSEC_COMMON); } DEFINE_MACHINE("ppce500", e500plat_machine_init) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 6ee566d658..db56d0a653 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3070,7 +3070,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) mc->default_boot_order = ""; mc->default_ram_size = 512 * M_BYTE; mc->kvm_type = spapr_kvm_type; - mc->has_dynamic_sysbus = true; + machine_class_add_sysbus_whitelist(mc, TYPE_SPAPR_PCI_HOST_BRIDGE); mc->pci_allow_0_address = true; mc->get_hotplug_handler = spapr_get_hotplug_handler; hc->pre_plug = spapr_machine_device_pre_plug; -- 2.11.0.259.g40922b1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [RFC 3/4] q35: Remove ioapic devices from sysbus whitelist 2017-03-23 21:28 [Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist Eduardo Habkost 2017-03-23 21:28 ` [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class Eduardo Habkost 2017-03-23 21:28 ` [Qemu-devel] [RFC 2/4] machine: Replace has_dynamic_sysbus with a whitelist Eduardo Habkost @ 2017-03-23 21:28 ` Eduardo Habkost 2017-03-23 21:28 ` [Qemu-devel] [RFC 4/4] q35: Remove fw_cfg* " Eduardo Habkost 2017-03-24 8:27 ` [Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist Juergen Gross 4 siblings, 0 replies; 14+ messages in thread From: Eduardo Habkost @ 2017-03-23 21:28 UTC (permalink / raw) To: qemu-devel Cc: jgross, Thomas Huth, sstabellini, Markus Armbruster, Laszlo Ersek, Marcel Apfelbaum An ioapic device is always created by the q35 machine, so "-device ioapic" and "-device kvm-ioapic" never worked before. Remove it from the sysbus whitelist. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/i386/pc_q35.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index ea039540d8..94a3069e16 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -312,9 +312,7 @@ static void pc_q35_machine_options(MachineClass *m) machine_class_add_sysbus_whitelist(m, "generic-sdhci"); machine_class_add_sysbus_whitelist(m, "hpet"); machine_class_add_sysbus_whitelist(m, "intel-iommu"); - machine_class_add_sysbus_whitelist(m, "ioapic"); machine_class_add_sysbus_whitelist(m, "isabus-bridge"); - machine_class_add_sysbus_whitelist(m, "kvm-ioapic"); machine_class_add_sysbus_whitelist(m, "kvmclock"); machine_class_add_sysbus_whitelist(m, "kvmvapic"); machine_class_add_sysbus_whitelist(m, "SUNW,fdtwo"); -- 2.11.0.259.g40922b1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [RFC 4/4] q35: Remove fw_cfg* from sysbus whitelist 2017-03-23 21:28 [Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist Eduardo Habkost ` (2 preceding siblings ...) 2017-03-23 21:28 ` [Qemu-devel] [RFC 3/4] q35: Remove ioapic devices from sysbus whitelist Eduardo Habkost @ 2017-03-23 21:28 ` Eduardo Habkost 2017-03-24 8:27 ` [Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist Juergen Gross 4 siblings, 0 replies; 14+ messages in thread From: Eduardo Habkost @ 2017-03-23 21:28 UTC (permalink / raw) To: qemu-devel Cc: jgross, Thomas Huth, sstabellini, Markus Armbruster, Laszlo Ersek, Marcel Apfelbaum fw_cfg devices are supposed to be created by the q35 board code, and not manually using -device. Remove them from the whitelist. Suggested-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/i386/pc_q35.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 94a3069e16..91d7f67e48 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -307,8 +307,6 @@ static void pc_q35_machine_options(MachineClass *m) machine_class_add_sysbus_whitelist(m, "amd-iommu"); machine_class_add_sysbus_whitelist(m, "cfi.pflash01"); machine_class_add_sysbus_whitelist(m, "esp"); - machine_class_add_sysbus_whitelist(m, "fw_cfg_io"); - machine_class_add_sysbus_whitelist(m, "fw_cfg_mem"); machine_class_add_sysbus_whitelist(m, "generic-sdhci"); machine_class_add_sysbus_whitelist(m, "hpet"); machine_class_add_sysbus_whitelist(m, "intel-iommu"); -- 2.11.0.259.g40922b1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist 2017-03-23 21:28 [Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist Eduardo Habkost ` (3 preceding siblings ...) 2017-03-23 21:28 ` [Qemu-devel] [RFC 4/4] q35: Remove fw_cfg* " Eduardo Habkost @ 2017-03-24 8:27 ` Juergen Gross 4 siblings, 0 replies; 14+ messages in thread From: Juergen Gross @ 2017-03-24 8:27 UTC (permalink / raw) To: Eduardo Habkost, qemu-devel Cc: Thomas Huth, sstabellini, Markus Armbruster, Laszlo Ersek, Marcel Apfelbaum On 23/03/17 22:28, Eduardo Habkost wrote: > Summary > ------- > > This series replaces the existing has_dynamic_sysbus flag (that > makes the machine accept every single sysbus device type on the > command-line) with a short whitelist. > > This will be helpful when implementing the new query-device-slots > command, because each machine type will include only the sysbus > devices it really supports, instead of including a catch-all > TYPE_SYS_BUS_DEVICE "slot". > > Most of the machines already have an internal whitelist > implemented using foreach_dynamic_sysbus_device(), so we just > encode the existing behavior inside the new MachineClass field. > > Caveat: q35 > ----------- > > The only problematic case is q35, that accepts all sysbus device > types. In this case, instead of adding TYPE_SYS_BUS_DEVICE to the > whitelist, I'm adding the existing list of existing sysbus device > types to keep compatibility. After that, we can gradually and > carefully remove unnecessary entries from the q35 whitelist. > > Issue: xen_set_dynamic_sysbus() hack > ------------------------------------ > > We still have an obstacle to get around: the callers of > qdev_set_id() outside qdev_device_add() are breaking > foreach_dynamic_sysbus_device(), and xen_set_dynamic_sysbus() is > a workaround for that bug. We now need to fix that bug to be able > to remove the xen_set_dynamic_sysbus() hack. This means the first > patch of this series will probably break Xen, until we fix that > bug. > > Eduardo Habkost (4): > [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class > machine: Replace has_dynamic_sysbus with a whitelist > q35: Remove ioapic devices from sysbus whitelist > q35: Remove fw_cfg* from sysbus whitelist > > include/hw/arm/sysbus-fdt.h | 7 +++++++ > include/hw/boards.h | 3 ++- > hw/arm/sysbus-fdt.c | 10 ++++++++++ > hw/arm/virt.c | 4 +++- > hw/core/machine.c | 43 +++++++++++++++++++++++++++++-------------- > hw/i386/pc_q35.c | 25 ++++++++++++++++++++++++- > hw/ppc/e500plat.c | 4 +++- > hw/ppc/spapr.c | 2 +- > hw/xen/xen_backend.c | 11 ----------- > 9 files changed, 79 insertions(+), 30 deletions(-) > This seems to break Xen. I got: qemu-system-i386: Option '-device xen-backend' cannot be handled by this machine when using qemu as Xen backend driver. Juergen ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-03-24 13:59 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-23 21:28 [Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist Eduardo Habkost 2017-03-23 21:28 ` [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class Eduardo Habkost 2017-03-24 8:23 ` Juergen Gross 2017-03-24 10:09 ` Peter Maydell 2017-03-24 10:24 ` Juergen Gross 2017-03-24 11:10 ` Eduardo Habkost 2017-03-24 12:27 ` Juergen Gross 2017-03-24 13:50 ` Eduardo Habkost 2017-03-24 13:59 ` Juergen Gross 2017-03-24 10:32 ` Thomas Huth 2017-03-23 21:28 ` [Qemu-devel] [RFC 2/4] machine: Replace has_dynamic_sysbus with a whitelist Eduardo Habkost 2017-03-23 21:28 ` [Qemu-devel] [RFC 3/4] q35: Remove ioapic devices from sysbus whitelist Eduardo Habkost 2017-03-23 21:28 ` [Qemu-devel] [RFC 4/4] q35: Remove fw_cfg* " Eduardo Habkost 2017-03-24 8:27 ` [Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist Juergen Gross
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.