Re: [Xen-devel] [PATCH 4/5] x86/pv: Remove deferred RDTSC{, P} handling in pv_emulate_privileged_op()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.02.18 at 12:58, <andrew.cooper3@xxxxxxxxxx> wrote:
> The handling of RDTSCP for PV guests has been broken (AFAICT forever).
> 
> To start with, RDTSCP is hidden from PV guests so the MSR_TSC_AUX path should
> be unreachable.  However, this appears to be a "feature" of TSC_MODE_PVRDTSCP,
> and the emulator doesn't perform appropriate feature checking.  (Conversely,
> we unilaterally advertise RDPID which uses the same path, but it should never
> trap on #GP to arrive here in the first place).
> 
> A PV guest typically can see RDTSCP in native CPUID, so userspace will
> probably end up using it.  On a capable pipeline (without TSD, see below), it
> will execute normally and return non-virtualised data.
> 
> When a virtual TSC mode is not specified for the domain, CR4.TSD is left
> clear, so executing RDTSCP will execute without trapping.  However, a guest
> kernel may set TSD itself, at which point the emulator should not suddenly
> switch to virtualised TSC mode and start handing out differently-scaled
> values.
> 
> Drop all the deferral logic, and return scaled or raw TSC values depending
> only on currd->arch.vtsc.  This changes the exact moment at which the
> timestamp is taken, but that doesn't matter from the guests point of view, and
> is consistent with the HVM side of things.  It also means that RDTSC and
> RDTSCP are now consistent WRT handing out native or virtualised timestamps.

The reason I didn't want to drop that deferral logic back when I
converted this code to use the main emulator was that
pv_soft_rdtsc() updates guest state beyond register values.
That is if the TSC_AUX access fails (and hence instruction
emulation as a whole fails), we would still have updated that
other state. The stats part of this is quite likely irrelevant in this
regard, but the update to d->arch.vtsc_last may yield
unintended guest observable change in behavior.

I don't mean to say this is a no-go, but it is a change that goes
beyond what you describe, and I'd like it to (a) be spelled out
and (b) given an explanation of why this is okay.

> The MSR_TSC_AUX case unconditionally returns the migration incarnation or
> zero, depending on TSC_MODE_PVRDTSCP, which is faster than re-reading it out
> of hardware.

This, iiuc, is correct solely because we don't currently permit PV
guests to write TSC_AUX. Which renders wrong exposing RDPID
to such guests. But without having checked yet, I could imagine
patch 5 to take care of this.

The changes themselves look fine to me, provided the wider
behavioral change is both intended and acceptable.

Jan


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