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

From: Paul Durrant <Paul.Durrant@citrix.com>
To: Paul Durrant <Paul.Durrant@citrix.com>,
	'Jan Beulich' <JBeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>,
	"Tim (Xen.org)" <tim@xen.org>,
	George Dunlap <George.Dunlap@citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Ian Jackson <Ian.Jackson@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v5 10/11] viridian: add implementation of synthetic timers
Date: Thu, 14 Mar 2019 09:58:03 +0000	[thread overview]
Message-ID: <505c75a6681d499e83066d620fbf7f89@AMSPEX02CL02.citrite.net> (raw)
In-Reply-To: <5ea47ce70ae54813b9e5189db2f423b3@AMSPEX02CL02.citrite.net>

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of Paul Durrant
> Sent: 13 March 2019 14:37
> To: 'Jan Beulich' <JBeulich@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Tim (Xen.org) <tim@xen.org>;
> George Dunlap <George.Dunlap@citrix.com>; Julien Grall <julien.grall@arm.com>; xen-devel <xen-
> devel@lists.xenproject.org>; Ian Jackson <Ian.Jackson@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel