Re: [Xen-devel] [PATCH v5 10/11] viridian: add implementation of synthetic timers

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of 
> Paul Durrant
> Sent: 13 March 2019 14:37
> To: 'Jan Beulich' <JBeulich@xxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu 
> <wei.liu2@xxxxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Tim 
> (Xen.org) <tim@xxxxxxx>;
> George Dunlap <George.Dunlap@xxxxxxxxxx>; Julien Grall 
> <julien.grall@xxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Roger Pau 
> Monne
> <roger.pau@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v5 10/11] viridian: add implementation of 
> synthetic timers
> 
[snip]
> 
> > As to safety of this, I have two concerns:
> >
> > 1) TscSequence gets updated as a result of a guest action (an MSR
> > write). This makes it non-obvious that the loop above will get
> > exited in due course.
> >
> 
> True. The domain could try to DoS this call. This could be avoided by doing a 
> domain_pause() if we
> test continuously fails for a number of iterations, or maybe just one 
> iteration.
> 
> > 2) The way update_reference_tsc() deals with the two "invalid"
> > values suggests ~0 and 0 should be special cased in general. I
> > _think_ this is not necessary here, but it also seems to me as if
> > the VM ever having a way to observe either of those two values
> > would be wrong too. Shouldn't the function avoid to ever store
> > ~0 into that field, i.e. increment into a local variable, update
> > that local variable to skip the two "invalid" values, and only then
> > store into the field?
> >
> > Otoh, making it into that function being a result of an MSR write,
> > it may welll be that the spec precludes the guest from reading
> > the reference page while an update was invoked from one of its
> > vCPU-s. If this was the case, then I also wouldn't have to
> > wonder any longer how this entire mechanism can be race free
> > in the first place (without a double increment like we do in the
> > pv-clock protocol).
> 
> From observation, it looks like Windows initializes the reference tsc page 
> before it brings secondary
> CPUs online and then doesn't touch the MSR again, so we should probably only 
> tolerate one mismatch in
> time_now() before doing domain_pause().

Actually it occurred to me last night that I'm being completely thick by coding 
it this way. The viridian code sets TscScale, not the guest, so we don't even 
need to reference the HV_REFERENCE_TSC_PAGE struct. Looking again, I'm also 
concerned that there's a small TOCTOU race in testing whether the reference tsc 
page is valid where the guest could unmap it on another CPU and cause a NULL 
pointer deref in time_now(), so I'll re-work this entirely.

  Paul

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel