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

[Juergen Gross]

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 <address@hidden> 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