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

[David Woodhouse]

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.

smime.p7s
Description: S/MIME cryptographic signature