diff mbox series

[v2,05/24] hw/xen: fix XenStore watch delivery to guest

Message ID 20231019154020.99080-6-dwmw2@infradead.org (mailing list archive)
State New, archived
Headers show
Series Get Xen PV shim running in Qemu, add net & console | expand

Commit Message

David Woodhouse Oct. 19, 2023, 3:40 p.m. UTC
From: David Woodhouse <dwmw@amazon.co.uk>

When fire_watch_cb() found the response buffer empty, it would call
deliver_watch() to generate the XS_WATCH_EVENT message in the response
buffer and send an event channel notification to the guest… without
actually *copying* the response buffer into the ring. So there was
nothing for the guest to see. The pending response didn't actually get
processed into the ring until the guest next triggered some activity
from its side.

Add the missing call to put_rsp().

It might have been slightly nicer to call xen_xenstore_event() here,
which would *almost* have worked. Except for the fact that it calls
xen_be_evtchn_pending() to check that it really does have an event
pending (and clear the eventfd for next time). And under Xen it's
defined that setting that fd to O_NONBLOCK isn't guaranteed to work,
so the emu implementation follows suit.

This fixes Xen device hot-unplug.

Fixes: 0254c4d19df ("hw/xen: Add xenstore wire implementation and implementation stubs")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/i386/kvm/xen_xenstore.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Paul Durrant Oct. 24, 2023, 3:19 p.m. UTC | #1
On 19/10/2023 16:40, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> When fire_watch_cb() found the response buffer empty, it would call
> deliver_watch() to generate the XS_WATCH_EVENT message in the response
> buffer and send an event channel notification to the guest… without
> actually *copying* the response buffer into the ring. So there was
> nothing for the guest to see. The pending response didn't actually get
> processed into the ring until the guest next triggered some activity
> from its side.
> 
> Add the missing call to put_rsp().
> 
> It might have been slightly nicer to call xen_xenstore_event() here,
> which would *almost* have worked. Except for the fact that it calls
> xen_be_evtchn_pending() to check that it really does have an event
> pending (and clear the eventfd for next time). And under Xen it's
> defined that setting that fd to O_NONBLOCK isn't guaranteed to work,
> so the emu implementation follows suit.
> 
> This fixes Xen device hot-unplug.
> 
> Fixes: 0254c4d19df ("hw/xen: Add xenstore wire implementation and implementation stubs")
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/i386/kvm/xen_xenstore.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
> index 660d0b72f9..82a215058a 100644
> --- a/hw/i386/kvm/xen_xenstore.c
> +++ b/hw/i386/kvm/xen_xenstore.c
> @@ -1357,10 +1357,12 @@ static void fire_watch_cb(void *opaque, const char *path, const char *token)
>       } else {
>           deliver_watch(s, path, token);
>           /*
> -         * If the message was queued because there was already ring activity,
> -         * no need to wake the guest. But if not, we need to send the evtchn.
> +         * Attempt to queue the message into the actual ring, and send
> +         * the event channel notification if any bytes are copied.
>            */
> -        xen_be_evtchn_notify(s->eh, s->be_port);
> +        if (put_rsp(s) > 0) {
> +            xen_be_evtchn_notify(s->eh, s->be_port);
> +        }

There's actually the potential for an assertion failure there; if the 
guest has specified an oversize token then deliver_watch() will not set 
rsp_pending... then put_rsp() will fail its assertion that the flag is true.

   Paul

>       }
>   }
>
David Woodhouse Oct. 24, 2023, 3:27 p.m. UTC | #2
On Tue, 2023-10-24 at 16:19 +0100, Paul Durrant wrote:
> On 19/10/2023 16:40, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > When fire_watch_cb() found the response buffer empty, it would call
> > deliver_watch() to generate the XS_WATCH_EVENT message in the response
> > buffer and send an event channel notification to the guest… without
> > actually *copying* the response buffer into the ring. So there was
> > nothing for the guest to see. The pending response didn't actually get
> > processed into the ring until the guest next triggered some activity
> > from its side.
> > 
> > Add the missing call to put_rsp().
> > 
> > It might have been slightly nicer to call xen_xenstore_event() here,
> > which would *almost* have worked. Except for the fact that it calls
> > xen_be_evtchn_pending() to check that it really does have an event
> > pending (and clear the eventfd for next time). And under Xen it's
> > defined that setting that fd to O_NONBLOCK isn't guaranteed to work,
> > so the emu implementation follows suit.
> > 
> > This fixes Xen device hot-unplug.
> > 
> > Fixes: 0254c4d19df ("hw/xen: Add xenstore wire implementation and implementation stubs")
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> >   hw/i386/kvm/xen_xenstore.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
> > index 660d0b72f9..82a215058a 100644
> > --- a/hw/i386/kvm/xen_xenstore.c
> > +++ b/hw/i386/kvm/xen_xenstore.c
> > @@ -1357,10 +1357,12 @@ static void fire_watch_cb(void *opaque, const char *path, const char *token)
> >       } else {
> >           deliver_watch(s, path, token);
> >           /*
> > -         * If the message was queued because there was already ring activity,
> > -         * no need to wake the guest. But if not, we need to send the evtchn.
> > +         * Attempt to queue the message into the actual ring, and send
> > +         * the event channel notification if any bytes are copied.
> >            */
> > -        xen_be_evtchn_notify(s->eh, s->be_port);
> > +        if (put_rsp(s) > 0) {
> > +            xen_be_evtchn_notify(s->eh, s->be_port);
> > +        }
> 
> There's actually the potential for an assertion failure there; if the
> guest has specified an oversize token then deliver_watch() will not set 
> rsp_pending... then put_rsp() will fail its assertion that the flag is true.
> 


Meh, I *already* had a whole paragraph in the commit message about how
it would have been nicer to just call xen_xenstore_event() :)

Thanks for spotting that; will fix it to check s->rsp_pending.
diff mbox series

Patch

diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
index 660d0b72f9..82a215058a 100644
--- a/hw/i386/kvm/xen_xenstore.c
+++ b/hw/i386/kvm/xen_xenstore.c
@@ -1357,10 +1357,12 @@  static void fire_watch_cb(void *opaque, const char *path, const char *token)
     } else {
         deliver_watch(s, path, token);
         /*
-         * If the message was queued because there was already ring activity,
-         * no need to wake the guest. But if not, we need to send the evtchn.
+         * Attempt to queue the message into the actual ring, and send
+         * the event channel notification if any bytes are copied.
          */
-        xen_be_evtchn_notify(s->eh, s->be_port);
+        if (put_rsp(s) > 0) {
+            xen_be_evtchn_notify(s->eh, s->be_port);
+        }
     }
 }