>>> On 20.02.18 at 12:58, <andrew.coop...@citrix.com> 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 Xenemail@example.com https://lists.xenproject.org/mailman/listinfo/xen-devel