| Message ID | 58381B15020000780012226B@prv-mh.provo.novell.com |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 25 November 2016 10:06 > To: qemu-devel@nongnu.org > Cc: Anthony Perard <anthony.perard@citrix.com>; Paul Durrant > <Paul.Durrant@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen- > devel <xen-devel@lists.xenproject.org> > Subject: [PATCH v2 1/3] xen: fix quad word bufioreq handling > > We should not consume the second slot if it didn't get written yet. > Normal writers - i.e. Xen - would not update write_pointer between the > two writes, but the page may get fiddled with by the guest itself, and > we're better off avoiding to enter an infinite loop in that case. > > Reported-by: yanghongke <yanghongke@huawei.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Bail (using hw_error()) instead of just breaking the loop. LGTM. Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > > --- a/xen-hvm.c > +++ b/xen-hvm.c > @@ -1021,6 +1021,9 @@ static int handle_buffered_iopage(XenIOS > xen_rmb(); > qw = (req.size == 8); > if (qw) { > + if (rdptr + 1 == wrptr) { > + hw_error("Incomplete quad word buffered ioreq"); > + } > buf_req = &buf_page->buf_ioreq[(rdptr + 1) % > IOREQ_BUFFER_SLOT_NUM]; > req.data |= ((uint64_t)buf_req->data) << 32; > >
On Fri, 25 Nov 2016, Jan Beulich wrote: > We should not consume the second slot if it didn't get written yet. > Normal writers - i.e. Xen - would not update write_pointer between the > two writes, but the page may get fiddled with by the guest itself, and > we're better off avoiding to enter an infinite loop in that case. > > Reported-by: yanghongke <yanghongke@huawei.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > v2: Bail (using hw_error()) instead of just breaking the loop. > > --- a/xen-hvm.c > +++ b/xen-hvm.c > @@ -1021,6 +1021,9 @@ static int handle_buffered_iopage(XenIOS > xen_rmb(); > qw = (req.size == 8); > if (qw) { > + if (rdptr + 1 == wrptr) { > + hw_error("Incomplete quad word buffered ioreq"); > + } > buf_req = &buf_page->buf_ioreq[(rdptr + 1) % > IOREQ_BUFFER_SLOT_NUM]; > req.data |= ((uint64_t)buf_req->data) << 32; > > >
--- a/xen-hvm.c +++ b/xen-hvm.c @@ -1021,6 +1021,9 @@ static int handle_buffered_iopage(XenIOS xen_rmb(); qw = (req.size == 8); if (qw) { + if (rdptr + 1 == wrptr) { + hw_error("Incomplete quad word buffered ioreq"); + } buf_req = &buf_page->buf_ioreq[(rdptr + 1) % IOREQ_BUFFER_SLOT_NUM]; req.data |= ((uint64_t)buf_req->data) << 32;
We should not consume the second slot if it didn't get written yet. Normal writers - i.e. Xen - would not update write_pointer between the two writes, but the page may get fiddled with by the guest itself, and we're better off avoiding to enter an infinite loop in that case. Reported-by: yanghongke <yanghongke@huawei.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Bail (using hw_error()) instead of just breaking the loop.