Re: [Xen-devel] [PATCH] x86/HVM: avoid pointer wraparound in bufioreq handling

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2015-06-16 at 09:37 +0100, Jan Beulich wrote:
> >>> On 16.06.15 at 10:20, <ian.campbell@xxxxxxxxxx> wrote:
> > On Tue, 2015-06-16 at 07:44 +0100, Jan Beulich wrote:
> >> >>> On 15.06.15 at 16:30, <JBeulich@xxxxxxxx> wrote:
> >> > The number of slots per page being 511 (i.e. not a power of two) means
> >> > that the (32-bit) read and write indexes going beyond 2^32 will likely
> >> > disturb operation. Extend I/O req server creation so the caller can
> >> > indicate that it is using suitable atomic accesses where needed (not
> >> > all accesses to the two pointers really need to be atomic), allowing
> >> > the hypervisor to atomically canonicalize both pointers when both have
> >> > gone through at least one cycle.
> >> > 
> >> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> 
> >> No matter that it's just a single line change, I realized that I
> >> forgot to Cc the tools maintainers. While a v2 will be needed (see
> >> the reply just sent to Andrew) I'd still appreciate input (if any) to
> >> limit the number of revisions needed.
> > 
> > For such a simple toolstack side change which just reflects the
> > underlying hcall interface I have no real opinion so far as the tools
> > side goes, but it would be good to update the comments in xenctrl.h too.
> > With that done for the tools change:
> >         Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 
> Thanks. The request for feedback went beyond the request for
> an ack though, namely
> 
> TBD: Do we need to be worried about non-libxc users of the changed
>      (tools only) interface?

It's (currently at least) a declared non-stable API, so in principal no.
It would be polite to give a heads up to the expected potential users
though, which you've done by CCing the QEMU maintainers I think. Adding
Paul D for completeness though.

>      Do we also need a way for default servers to flag atomicity?

Not doing so leaves them open to the issue which you are fixing here, I
think?

Andy indicated that qemu-trad was the only default-server these days, so
perhaps we can just decree that it is so while fixing qemu-trad as
necessary?

In fact, are ioreq servers new enough and with few enough users that can
we decree that even they are always atomic, perhaps after having audited
the current users to ensure they behave correctly?

> > For the hypercall interface level, I wonder if handle_bufioreq is still
> > an appropriate name given its no longer treated as a boolean flag?
> > bufioreq_type or something perhaps?
> 
> I think the name is fine as is (it doesn't really imply boolean-ness
> in my reading), but if others agree with you that it should be
> renamed I also wouldn't object.

I read it as "should handle buf ioreqs", with should being a boolean
type predicate thing, but that might just be me, I suppose it could also
be read as "kind of buf ioreqs to handle".


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel