| Message ID | 20221205173137.607044-10-dwmw2@infradead.org |
|---|---|
| State | New |
| Headers | show |
| Series | Xen HVM support under KVM | expand |
On 5/12/22 18:31, David Woodhouse wrote: > From: Joao Martins <joao.m.martins@oracle.com> > > This allows -machine xenfv to work with Xen emulated guests. > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > --- > hw/i386/pc_piix.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 3dcac2f4b6..d1127adde0 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -404,8 +404,8 @@ static void pc_xen_hvm_init(MachineState *machine) > { > PCMachineState *pcms = PC_MACHINE(machine); > > - if (!xen_enabled()) { > - error_report("xenfv machine requires the xen accelerator"); > + if (!xen_enabled() && (xen_mode != XEN_EMULATE)) { > + error_report("xenfv machine requires the xen or kvm accelerator"); > exit(1); > } What about the XEN_EMULATE case? Shouldn't this be: if (!xen_enabled()) { if (xen_mode == XEN_EMULATE) { error_report("xenfv machine requires the xen accelerator"); } else { error_report("xenfv machine requires the xen or kvm accelerator"); } exit(1); } ?
On Mon, 2022-12-05 at 23:06 +0100, Philippe Mathieu-Daudé wrote: > On 5/12/22 18:31, David Woodhouse wrote: > > From: Joao Martins <joao.m.martins@oracle.com> > > > > This allows -machine xenfv to work with Xen emulated guests. > > > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > > --- > > hw/i386/pc_piix.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > > index 3dcac2f4b6..d1127adde0 100644 > > --- a/hw/i386/pc_piix.c > > +++ b/hw/i386/pc_piix.c > > @@ -404,8 +404,8 @@ static void pc_xen_hvm_init(MachineState *machine) > > { > > PCMachineState *pcms = PC_MACHINE(machine); > > > > - if (!xen_enabled()) { > > - error_report("xenfv machine requires the xen accelerator"); > > + if (!xen_enabled() && (xen_mode != XEN_EMULATE)) { > > + error_report("xenfv machine requires the xen or kvm accelerator"); > > exit(1); > > } > > What about the XEN_EMULATE case? Shouldn't this be: > > if (!xen_enabled()) { > if (xen_mode == XEN_EMULATE) { > error_report("xenfv machine requires the xen accelerator"); > } else { > error_report("xenfv machine requires the xen or kvm accelerator"); > } > exit(1); > } > > ? Erm... that one I cherry-picked directly from the original and I confess I haven't yet done much thinking about it. There are two sane cases. If xen_mode == XEN_ATTACH, then xen_enabled() should be true. If xen_mode == XEN_EMULATED, then we'd better be using KVM with the Xen support (which could *theoretically* be added to TCG if anyone really wanted to). So this check is working because it's allowing *either* xen_enabled() *or* xen_mode==XEN_ATTACH to satisfy it. But it's too lax. I think it should *require* KVM in the case of XEN_EMULATE. That ought to be sufficient since it's going to set the xen-version machine property, and that would cause KVM itself to bail out if the required support isn't present in the kernel. (I'm assuming the existing XEN_EMULATE mode is long dead along with Xenner? Even on true Xen we run PV guests in a shim these days, and that shim works without modification under KVM and will eventually be one of my test cases as I get this all working under qemu)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 3dcac2f4b6..d1127adde0 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -404,8 +404,8 @@ static void pc_xen_hvm_init(MachineState *machine) { PCMachineState *pcms = PC_MACHINE(machine); - if (!xen_enabled()) { - error_report("xenfv machine requires the xen accelerator"); + if (!xen_enabled() && (xen_mode != XEN_EMULATE)) { + error_report("xenfv machine requires the xen or kvm accelerator"); exit(1); }