[PATCH v2 16/24] hw/xen: handle soft reset for primary console

David Woodhouse posted 24 patches 1 year, 1 month ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, David Woodhouse <dwmw2@infradead.org>, Jason Wang <jasowang@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>, Cleber Rosa <crosa@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Wainer dos Santos Moschetta <wainersm@redhat.com>, Beraldo Leal <bleal@redhat.com>
[PATCH v2 16/24] hw/xen: handle soft reset for primary console
Posted by David Woodhouse 1 year, 1 month ago
From: David Woodhouse <dwmw@amazon.co.uk>

On soft reset, the prinary console event channel needs to be rebound to
the backend port (in the xen-console driver). We could put that into the
xen-console driver itself, but it's slightly less ugly to keep it within
the KVM/Xen code, by stashing the backend port# on event channel reset
and then rebinding in the primary console reset when it has to recreate
the guest port anyway.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/i386/kvm/xen_evtchn.c          |  9 +++++++++
 hw/i386/kvm/xen_primary_console.c | 29 ++++++++++++++++++++++++++++-
 hw/i386/kvm/xen_primary_console.h |  1 +
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
index d72dca6591..ce4da6d37a 100644
--- a/hw/i386/kvm/xen_evtchn.c
+++ b/hw/i386/kvm/xen_evtchn.c
@@ -40,6 +40,7 @@
 #include "xen_evtchn.h"
 #include "xen_overlay.h"
 #include "xen_xenstore.h"
+#include "xen_primary_console.h"
 
 #include "sysemu/kvm.h"
 #include "sysemu/kvm_xen.h"
@@ -1098,6 +1099,7 @@ int xen_evtchn_soft_reset(void)
 {
     XenEvtchnState *s = xen_evtchn_singleton;
     bool flush_kvm_routes;
+    uint16_t con_port = xen_primary_console_get_port();
     int i;
 
     if (!s) {
@@ -1108,6 +1110,13 @@ int xen_evtchn_soft_reset(void)
 
     qemu_mutex_lock(&s->port_lock);
 
+    if (con_port) {
+        XenEvtchnPort *p = &s->port_table[con_port];
+        if (p->type == EVTCHNSTAT_interdomain) {
+            xen_primary_console_set_be_port(p->u.interdomain.port);
+        }
+    }
+
     for (i = 0; i < s->nr_ports; i++) {
         close_port(s, i, &flush_kvm_routes);
     }
diff --git a/hw/i386/kvm/xen_primary_console.c b/hw/i386/kvm/xen_primary_console.c
index 0aa1c16ad6..5e6e085ac7 100644
--- a/hw/i386/kvm/xen_primary_console.c
+++ b/hw/i386/kvm/xen_primary_console.c
@@ -112,6 +112,15 @@ uint16_t xen_primary_console_get_port(void)
     return s->guest_port;
 }
 
+void xen_primary_console_set_be_port(uint16_t port)
+{
+    XenPrimaryConsoleState *s = xen_primary_console_singleton;
+    if (s) {
+        printf("be port set to %d\n", port);
+        s->be_port = port;
+    }
+}
+
 uint64_t xen_primary_console_get_pfn(void)
 {
     XenPrimaryConsoleState *s = xen_primary_console_singleton;
@@ -142,6 +151,20 @@ static void alloc_guest_port(XenPrimaryConsoleState *s)
     }
 }
 
+static void rebind_guest_port(XenPrimaryConsoleState *s)
+{
+    struct evtchn_bind_interdomain inter = {
+        .remote_dom = DOMID_QEMU,
+        .remote_port = s->be_port,
+    };
+
+    if (!xen_evtchn_bind_interdomain_op(&inter)) {
+        s->guest_port = inter.local_port;
+    }
+
+    s->be_port = 0;
+}
+
 int xen_primary_console_reset(void)
 {
     XenPrimaryConsoleState *s = xen_primary_console_singleton;
@@ -154,7 +177,11 @@ int xen_primary_console_reset(void)
         xen_overlay_do_map_page(&s->console_page, gpa);
     }
 
-    alloc_guest_port(s);
+    if (s->be_port) {
+        rebind_guest_port(s);
+    } else {
+        alloc_guest_port(s);
+    }
 
     trace_xen_primary_console_reset(s->guest_port);
 
diff --git a/hw/i386/kvm/xen_primary_console.h b/hw/i386/kvm/xen_primary_console.h
index dd4922f3f4..7e2989ea0d 100644
--- a/hw/i386/kvm/xen_primary_console.h
+++ b/hw/i386/kvm/xen_primary_console.h
@@ -16,6 +16,7 @@ void xen_primary_console_create(void);
 int xen_primary_console_reset(void);
 
 uint16_t xen_primary_console_get_port(void);
+void xen_primary_console_set_be_port(uint16_t port);
 uint64_t xen_primary_console_get_pfn(void);
 void *xen_primary_console_get_map(void);
 
-- 
2.40.1
Re: [PATCH v2 16/24] hw/xen: handle soft reset for primary console
Posted by Paul Durrant 1 year, 1 month ago
On 19/10/2023 16:40, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> On soft reset, the prinary console event channel needs to be rebound to
> the backend port (in the xen-console driver). We could put that into the
> xen-console driver itself, but it's slightly less ugly to keep it within
> the KVM/Xen code, by stashing the backend port# on event channel reset
> and then rebinding in the primary console reset when it has to recreate
> the guest port anyway.

Does Xen re-bind the primary console on EVTCHNOP_reset? That's news to 
me. I go check.

   Paul

> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/i386/kvm/xen_evtchn.c          |  9 +++++++++
>   hw/i386/kvm/xen_primary_console.c | 29 ++++++++++++++++++++++++++++-
>   hw/i386/kvm/xen_primary_console.h |  1 +
>   3 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
> index d72dca6591..ce4da6d37a 100644
> --- a/hw/i386/kvm/xen_evtchn.c
> +++ b/hw/i386/kvm/xen_evtchn.c
> @@ -40,6 +40,7 @@
>   #include "xen_evtchn.h"
>   #include "xen_overlay.h"
>   #include "xen_xenstore.h"
> +#include "xen_primary_console.h"
>   
>   #include "sysemu/kvm.h"
>   #include "sysemu/kvm_xen.h"
> @@ -1098,6 +1099,7 @@ int xen_evtchn_soft_reset(void)
>   {
>       XenEvtchnState *s = xen_evtchn_singleton;
>       bool flush_kvm_routes;
> +    uint16_t con_port = xen_primary_console_get_port();
>       int i;
>   
>       if (!s) {
> @@ -1108,6 +1110,13 @@ int xen_evtchn_soft_reset(void)
>   
>       qemu_mutex_lock(&s->port_lock);
>   
> +    if (con_port) {
> +        XenEvtchnPort *p = &s->port_table[con_port];
> +        if (p->type == EVTCHNSTAT_interdomain) {
> +            xen_primary_console_set_be_port(p->u.interdomain.port);
> +        }
> +    }
> +
>       for (i = 0; i < s->nr_ports; i++) {
>           close_port(s, i, &flush_kvm_routes);
>       }
> diff --git a/hw/i386/kvm/xen_primary_console.c b/hw/i386/kvm/xen_primary_console.c
> index 0aa1c16ad6..5e6e085ac7 100644
> --- a/hw/i386/kvm/xen_primary_console.c
> +++ b/hw/i386/kvm/xen_primary_console.c
> @@ -112,6 +112,15 @@ uint16_t xen_primary_console_get_port(void)
>       return s->guest_port;
>   }
>   
> +void xen_primary_console_set_be_port(uint16_t port)
> +{
> +    XenPrimaryConsoleState *s = xen_primary_console_singleton;
> +    if (s) {
> +        printf("be port set to %d\n", port);
> +        s->be_port = port;
> +    }
> +}
> +
>   uint64_t xen_primary_console_get_pfn(void)
>   {
>       XenPrimaryConsoleState *s = xen_primary_console_singleton;
> @@ -142,6 +151,20 @@ static void alloc_guest_port(XenPrimaryConsoleState *s)
>       }
>   }
>   
> +static void rebind_guest_port(XenPrimaryConsoleState *s)
> +{
> +    struct evtchn_bind_interdomain inter = {
> +        .remote_dom = DOMID_QEMU,
> +        .remote_port = s->be_port,
> +    };
> +
> +    if (!xen_evtchn_bind_interdomain_op(&inter)) {
> +        s->guest_port = inter.local_port;
> +    }
> +
> +    s->be_port = 0;
> +}
> +
>   int xen_primary_console_reset(void)
>   {
>       XenPrimaryConsoleState *s = xen_primary_console_singleton;
> @@ -154,7 +177,11 @@ int xen_primary_console_reset(void)
>           xen_overlay_do_map_page(&s->console_page, gpa);
>       }
>   
> -    alloc_guest_port(s);
> +    if (s->be_port) {
> +        rebind_guest_port(s);
> +    } else {
> +        alloc_guest_port(s);
> +    }
>   
>       trace_xen_primary_console_reset(s->guest_port);
>   
> diff --git a/hw/i386/kvm/xen_primary_console.h b/hw/i386/kvm/xen_primary_console.h
> index dd4922f3f4..7e2989ea0d 100644
> --- a/hw/i386/kvm/xen_primary_console.h
> +++ b/hw/i386/kvm/xen_primary_console.h
> @@ -16,6 +16,7 @@ void xen_primary_console_create(void);
>   int xen_primary_console_reset(void);
>   
>   uint16_t xen_primary_console_get_port(void);
> +void xen_primary_console_set_be_port(uint16_t port);
>   uint64_t xen_primary_console_get_pfn(void);
>   void *xen_primary_console_get_map(void);
>
Re: [PATCH v2 16/24] hw/xen: handle soft reset for primary console
Posted by David Woodhouse 1 year, 1 month ago
On Tue, 2023-10-24 at 16:44 +0100, Paul Durrant wrote:
> On 19/10/2023 16:40, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > On soft reset, the prinary console event channel needs to be rebound to
> > the backend port (in the xen-console driver). We could put that into the
> > xen-console driver itself, but it's slightly less ugly to keep it within
> > the KVM/Xen code, by stashing the backend port# on event channel reset
> > and then rebinding in the primary console reset when it has to recreate
> > the guest port anyway.
> 
> Does Xen re-bind the primary console on EVTCHNOP_reset? That's news to 
> me. I go check.

I spent an unhapp hour trying to work out how Xen actually does any of
this :)

In the short term I'm more interested in having soft reset work, than
an explicit EVTCHNOP_reset. And I can't work out *how*, but we do seem
to have console again after a kexec in real Xen.
Re: [PATCH v2 16/24] hw/xen: handle soft reset for primary console
Posted by Paul Durrant 1 year, 1 month ago
On 24/10/2023 16:48, David Woodhouse wrote:
> On Tue, 2023-10-24 at 16:44 +0100, Paul Durrant wrote:
>> On 19/10/2023 16:40, David Woodhouse wrote:
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> On soft reset, the prinary console event channel needs to be rebound to
>>> the backend port (in the xen-console driver). We could put that into the
>>> xen-console driver itself, but it's slightly less ugly to keep it within
>>> the KVM/Xen code, by stashing the backend port# on event channel reset
>>> and then rebinding in the primary console reset when it has to recreate
>>> the guest port anyway.
>>
>> Does Xen re-bind the primary console on EVTCHNOP_reset? That's news to
>> me. I go check.
> 
> I spent an unhapp hour trying to work out how Xen actually does any of
> this :)
> 
> In the short term I'm more interested in having soft reset work, than
> an explicit EVTCHNOP_reset. And I can't work out *how*, but we do seem
> to have console again after a kexec in real Xen.

*Soft* reset may do it, but not the EVTCHNOP_reset hypercall itself, 
because there's a bunch of impenetrable toolstack magic involved the 
former. Perhaps you could just push the re-bind code up a layer into
kvm_xen_soft_reset().

   Paul
Re: [PATCH v2 16/24] hw/xen: handle soft reset for primary console
Posted by David Woodhouse 1 year, 1 month ago
On Tue, 2023-10-24 at 17:22 +0100, Paul Durrant wrote:
> On 24/10/2023 16:48, David Woodhouse wrote:
> > On Tue, 2023-10-24 at 16:44 +0100, Paul Durrant wrote:
> > > On 19/10/2023 16:40, David Woodhouse wrote:
> > > > From: David Woodhouse <dwmw@amazon.co.uk>
> > > > 
> > > > On soft reset, the prinary console event channel needs to be rebound to
> > > > the backend port (in the xen-console driver). We could put that into the
> > > > xen-console driver itself, but it's slightly less ugly to keep it within
> > > > the KVM/Xen code, by stashing the backend port# on event channel reset
> > > > and then rebinding in the primary console reset when it has to recreate
> > > > the guest port anyway.
> > > 
> > > Does Xen re-bind the primary console on EVTCHNOP_reset? That's news to
> > > me. I go check.
> > 
> > I spent an unhapp hour trying to work out how Xen actually does any of
> > this :)
> > 
> > In the short term I'm more interested in having soft reset work, than
> > an explicit EVTCHNOP_reset. And I can't work out *how*, but we do seem
> > to have console again after a kexec in real Xen.
> 
> *Soft* reset may do it, but not the EVTCHNOP_reset hypercall itself, 
> because there's a bunch of impenetrable toolstack magic involved the 
> former. Perhaps you could just push the re-bind code up a layer into
> kvm_xen_soft_reset().

Actually, all we're doing here is *storing* the be_port so that the
*console* reset code can later connect to it. So the actual reconnect 
is already called separately from a layer up in kvm_xen_soft_reset().

If the guest just calls EVTCHNOP_reset then it'll just get the event
channels reset and the console *won't* reconnect.

Actually.. if the guest just calls EVTCHNOP_reset I think they'll just
hit the assert(qemu_mutex_iothread_locked()) at line 1109. We need a  
QEMU_IOTHREAD_LOCK_GUARD() in the penultimate line of
xen_evtchn_reset_op(). I'll fix that separately.