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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Feb 20, 2018 at 11:58:42AM +0000, Andrew Cooper 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 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 is a behavioural change for guests, but the semantics are rather more
> sane.  It lays groundwork for further fixes.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>  xen/arch/x86/pv/emul-priv-op.c | 35 +++++------------------------------
>  1 file changed, 5 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
> index d4d64f2..4e3641d 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -60,9 +60,6 @@ struct priv_op_ctxt {
>      } cs;
>      char *io_emul_stub;
>      unsigned int bpmatch;
> -    unsigned int tsc;
> -#define TSC_BASE 1
> -#define TSC_AUX 2
>  };
>  
>  /* I/O emulation support. Helper routines for, and type of, the stack stub. 
> */
> @@ -843,8 +840,7 @@ static inline bool is_cpufreq_controller(const struct 
> domain *d)
>  static int read_msr(unsigned int reg, uint64_t *val,
>                      struct x86_emulate_ctxt *ctxt)
>  {
> -    struct priv_op_ctxt *poc = container_of(ctxt, struct priv_op_ctxt, ctxt);
> -    const struct vcpu *curr = current;
> +    struct vcpu *curr = current;

I think you can keep the const here?

>      const struct domain *currd = curr->domain;
>      bool vpmu_msr = false;
>      int ret;
> @@ -880,20 +876,13 @@ static int read_msr(unsigned int reg, uint64_t *val,
>          *val = curr->arch.pv_vcpu.gs_base_user;
>          return X86EMUL_OKAY;
>  
> -    /*
> -     * In order to fully retain original behavior, defer calling
> -     * pv_soft_rdtsc() until after emulation. This may want/need to be
> -     * reconsidered.
> -     */
>      case MSR_IA32_TSC:
> -        poc->tsc |= TSC_BASE;
> -        goto normal;
> +        *val = currd->arch.vtsc ? pv_soft_rdtsc(curr, ctxt->regs) : rdtsc();
> +        return X86EMUL_OKAY;
>  
>      case MSR_TSC_AUX:
> -        poc->tsc |= TSC_AUX;
> -        if ( cpu_has_rdtscp )
> -            goto normal;
> -        *val = 0;
> +        *val = (uint32_t)((currd->arch.tsc_mode == TSC_MODE_PVRDTSCP)
> +                          ? currd->arch.incarnation : 0);

I wonder whether Xen should inject a #GP here if tsc_mode is not
PVRDTSCP and RDPID is not available.

Thanks, Roger.

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