Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class

[Eduardo Habkost <ehabkost@redhat.com>]

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