| 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 |
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 > } > } >
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 --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); + } } }