[v3] Xen HVM support under KVM

[RFC PATCH v3 03/38] xen: Add XEN_DISABLED mode and make it default

Posted by David Woodhouse 3 years, 1 month ago

From: David Woodhouse <dwmw@amazon.co.uk>

Also check for XEN_ATTACH mode in xen_init()

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 accel/xen/xen-all.c  | 4 ++++
 include/hw/xen/xen.h | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 69aa7d018b..109d2e84bc 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -158,6 +158,10 @@ static int xen_init(MachineState *ms)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
 
+    if (xen_mode != XEN_ATTACH) {
+        xen_pv_printf(NULL, 0, "xen requires --xen-attach mode\n");
+        return -1;
+    }
     xen_xc = xc_interface_open(0, 0, 0);
     if (xen_xc == NULL) {
         xen_pv_printf(NULL, 0, "can't open xen interface\n");
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index afdf9c436a..82347e76a4 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -12,8 +12,9 @@
 
 /* xen-machine.c */
 enum xen_mode {
-    XEN_EMULATE = 0,  // xen emulation, using xenner (default)
-    XEN_ATTACH        // attach to xen domain created by libxl
+    XEN_DISABLED = 0, // xen support disabled (default)
+    XEN_ATTACH,       // attach to xen domain created by libxl
+    XEN_EMULATE,
 };
 
 extern uint32_t xen_domid;
-- 
2.35.3

Re: [RFC PATCH v3 03/38] xen: Add XEN_DISABLED mode and make it default

Posted by Paul Durrant 3 years, 1 month ago

On 16/12/2022 00:40, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Also check for XEN_ATTACH mode in xen_init()
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   accel/xen/xen-all.c  | 4 ++++
>   include/hw/xen/xen.h | 5 +++--
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 69aa7d018b..109d2e84bc 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -158,6 +158,10 @@ static int xen_init(MachineState *ms)
>   {
>       MachineClass *mc = MACHINE_GET_CLASS(ms);
>   
> +    if (xen_mode != XEN_ATTACH) {
> +        xen_pv_printf(NULL, 0, "xen requires --xen-attach mode\n");
> +        return -1;
> +    }

This is new requirement, isn't it? Libxl only passes --xen-attach
for PV domains so AFAICT this will break Xen HVM domains.

   Paul

>       xen_xc = xc_interface_open(0, 0, 0);
>       if (xen_xc == NULL) {
>           xen_pv_printf(NULL, 0, "can't open xen interface\n");
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index afdf9c436a..82347e76a4 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -12,8 +12,9 @@
>   
>   /* xen-machine.c */
>   enum xen_mode {
> -    XEN_EMULATE = 0,  // xen emulation, using xenner (default)
> -    XEN_ATTACH        // attach to xen domain created by libxl
> +    XEN_DISABLED = 0, // xen support disabled (default)
> +    XEN_ATTACH,       // attach to xen domain created by libxl
> +    XEN_EMULATE,
>   };
>   
>   extern uint32_t xen_domid;

Re: [RFC PATCH v3 03/38] xen: Add XEN_DISABLED mode and make it default

Posted by David Woodhouse 3 years, 1 month ago

On Tue, 2022-12-20 at 14:39 +0000, Paul Durrant wrote:
> On 16/12/2022 00:40, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > Also check for XEN_ATTACH mode in xen_init()
> > 
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> >    accel/xen/xen-all.c  | 4 ++++
> >    include/hw/xen/xen.h | 5 +++--
> >    2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> > index 69aa7d018b..109d2e84bc 100644
> > --- a/accel/xen/xen-all.c
> > +++ b/accel/xen/xen-all.c
> > @@ -158,6 +158,10 @@ static int xen_init(MachineState *ms)
> >    {
> >        MachineClass *mc = MACHINE_GET_CLASS(ms);
> >    
> > +    if (xen_mode != XEN_ATTACH) {
> > +        xen_pv_printf(NULL, 0, "xen requires --xen-attach mode\n");
> > +        return -1;
> > +    }
> 
> This is new requirement, isn't it? Libxl only passes --xen-attach
> for PV domains so AFAICT this will break Xen HVM domains.

So HVM domains are currently running with xen_mode == XEN_EMULATE and
work because nobody ever actually looks at that variable except in
xen_init_pv().

What was the reason for ever needing it to be explicitly set on the
command line; couldn't it have been implicit?

Seems like the right thing to do is to make it implicit in xen_init()
above, and *set* it to XEN_ATTACH instead of requiring it to have been
set on the command line?

Re: [RFC PATCH v3 03/38] xen: Add XEN_DISABLED mode and make it default

Posted by Paul Durrant 3 years, 1 month ago

On 20/12/2022 22:59, David Woodhouse wrote:
> On Tue, 2022-12-20 at 14:39 +0000, Paul Durrant wrote:
>> On 16/12/2022 00:40, David Woodhouse wrote:
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> Also check for XEN_ATTACH mode in xen_init()
>>>
>>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>>> ---
>>>     accel/xen/xen-all.c  | 4 ++++
>>>     include/hw/xen/xen.h | 5 +++--
>>>     2 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
>>> index 69aa7d018b..109d2e84bc 100644
>>> --- a/accel/xen/xen-all.c
>>> +++ b/accel/xen/xen-all.c
>>> @@ -158,6 +158,10 @@ static int xen_init(MachineState *ms)
>>>     {
>>>         MachineClass *mc = MACHINE_GET_CLASS(ms);
>>>     
>>> +    if (xen_mode != XEN_ATTACH) {
>>> +        xen_pv_printf(NULL, 0, "xen requires --xen-attach mode\n");
>>> +        return -1;
>>> +    }
>>
>> This is new requirement, isn't it? Libxl only passes --xen-attach
>> for PV domains so AFAICT this will break Xen HVM domains.
> 
> So HVM domains are currently running with xen_mode == XEN_EMULATE and
> work because nobody ever actually looks at that variable except in
> xen_init_pv().
> 
> What was the reason for ever needing it to be explicitly set on the
> command line; couldn't it have been implicit?
> 

Judging by the comments on the enum, it dates back to xenner. I guess it 
was needed to disambiguate use of xenner from attaching to a real Xen 
instance.
The only cases I see it being used in libxl are for starting up a qdisk 
backend when spawning a PV domU, or retrospectively for dom0.

As you say, the only place it is checked is in xen_init_pv() and even 
then it has no effect. The comment in the code says "nothing to do, 
libxl handles everything".

> Seems like the right thing to do is to make it implicit in xen_init()
> above, and *set* it to XEN_ATTACH instead of requiring it to have been
> set on the command line?

I guess, now that your reviving Xen emulation in QEMU, the mode does 
become relevant again so making XEN_ATTACH implicit when QEMU is 
launched by libxl, in all cases, is the appropriate thing to do.

   Paul