Re: [RFC PATCH v2 03/22] i386/xen: Add xen-version machine property and init KVM Xen support

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3553 bytes --]

On Mon, 2022-12-12 at 18:30 +0100, Paolo Bonzini wrote:
> On 12/9/22 10:55, David Woodhouse wrote:
> > -    m->default_machine_opts = "accel=xen,suppress-vmdesc=on";
> > +    if (xen_enabled())
> > +            m->default_machine_opts = "accel=xen,suppress-vmdesc=on";
> > +    else
> > +            m->default_machine_opts = "accel=kvm,xen-version=0x30001";
> 
> Please do not modify pc_xen_hvm_init().
> 
> "-M xenfv" should be the same as "-M pc-i440fx-...,suppress-vmdesc=on
> -accel xen -device xen-platform".  It must work *without* "-accel xen", 
> while here you're basically requiring it.  For now, please make 
> KVM-emulated Xen use "-M pc -device xen-platform". 

I did that (well, you also need -accel kvm,xen_version=xxx).

>  We can figure out "-M xenfv" later.

I don't know that we care about doing this. Sure, I could change the if
(xen_enabled()) to something which isn't recursive, and works out which
of Xen or KVM mode is *possible* right now.

But having moved the xen-version from a machine property to a kvm-accel
property, using "accel=kvm,xen-version=xxx" doesn't even work; not
without the hackery to 'forward' that from the machine to the
accelerator, like we do in qemu_apply_legacy_machine_options() for
properties like kvm-shadow-mem. I don't think we want that.

So I think I'm most inclined to rename the CONFIG_XENFV_MACHINE option
to CONFIG_XEN_BUS, since that's basically what it ended up being once
the rest of the patches took shape on top of the basic platform
support. And drop the idea of making '-M xenfv' work altogether.

I'll add some documentation on how to launch it using
-accel kvm,xen-version=.

> You can instead have:
> 
> - a check in xen_init() that checks that xen_mode is XEN_ATTACH.  If 
> not, fail.
> 
> - an extra enum value for xen_mode, XEN_DISABLED, which is the default 
> instead of XEN_EMULATE;
> 
> - an accelerator property "-accel kvm,xen-version=...", added in 
> kvm_accel_class_init() instead of the machine property.  The property, 
> when set to a nonzero value, flips xen_mode from XEN_DISABLED to 
> XEN_EMULATE.
> 
> The Xen overlay device can be created using the mc->kvm_type function
> (which you can set in DEFINE_PC_MACHINE); at that point, xen_mode has
> switched from XEN_DISABLED to XEN_EMULATE.  Those xen_enabled() checks 
> that apply to KVM then become xen_mode != XEN_DISABLED, as long as they 
> run during mc->kvm_type or afterwards.
> 
> The platform device can be created either in mc->kvm_type or manually
> (not sure if it makes sense to have a "XenVMMXenVMM" CPUID + emulated
> hypercalls but no platform device---would it still use pvclock for 
> example?).

Yeah, I think fairly much everything *can* work without the platform
device. It's only really the hook for the legacy event channel
interrupt, and the unplug protocol. Linux does use its BAR to map grant
tables over, but that's just a crutch to find a free bit of guest
physical address space.

But still, I think it makes sense to add it unconditionally as you
suggest. In mc->kvm_type, pcms->bus isn't set yet but I can do it like
this:

--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1313,6 +1313,9 @@ void pc_basic_device_init(struct PCMachineState
*pcms,
 #ifdef CONFIG_XEN_EMU
     if (xen_mode == XEN_EMULATE) {
         xen_evtchn_connect_gsis(gsi);
+        if (pcms->bus) {
+            pci_create_simple(pcms->bus, -1, "xen-platform");
+        }
     }
 #endif
 



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]