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@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
> > 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.
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
