Re: [Xen-devel] [PATCH v3 7/8] x86emul: support RDPRU

[Jan Beulich <jbeulich@suse.com>]

On 03.09.2019 14:34, Andrew Cooper wrote:
> On 03/09/2019 10:41, Jan Beulich wrote:
>> While the PM doesn't say so, this assumes that the MPERF value read this
>> way gets scaled similarly to its reading through RDMSR.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> This wants the following hunks merged, to at least keep the
> intercept/exit codes up to date.  This is from my alternative series
> which intercepted and terminated RDPRU with #UD.
> 
> diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h
> b/xen/include/asm-x86/hvm/svm/vmcb.h
> index 5c710286f7..2bf0d50f6d 100644
> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
> @@ -76,7 +76,8 @@ enum GenericIntercept2bits
>      GENERAL2_INTERCEPT_MONITOR = 1 << 10,
>      GENERAL2_INTERCEPT_MWAIT   = 1 << 11,
>      GENERAL2_INTERCEPT_MWAIT_CONDITIONAL = 1 << 12,
> -    GENERAL2_INTERCEPT_XSETBV  = 1 << 13
> +    GENERAL2_INTERCEPT_XSETBV  = 1 << 13,
> +    GENERAL2_INTERCEPT_RDPRU   = 1 << 14,
>  };
> 
> 
> @@ -300,6 +301,7 @@ enum VMEXIT_EXITCODE
>      VMEXIT_MWAIT            = 139, /* 0x8b */
>      VMEXIT_MWAIT_CONDITIONAL= 140, /* 0x8c */
>      VMEXIT_XSETBV           = 141, /* 0x8d */
> +    VMEXIT_RDPRU            = 142, /* 0x8e */
>      VMEXIT_NPF              = 1024, /* 0x400, nested paging fault */
>      VMEXIT_INVALID          =  -1
>  };

I wouldn't think this belongs here, but since you ask for it, I
can fold it in.

>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -545,6 +545,11 @@ void recalculate_cpuid_policy(struct dom
>>  
>>      p->extd.maxlinaddr = p->extd.lm ? 48 : 32;
>>  
>> +    if ( p->extd.rdpru )
>> +        p->extd.rdpru_max = min(p->extd.rdpru_max, max->extd.rdpru_max);
>> +    else
>> +        p->extd.rdpru_max = 0;
>> +
>>      recalculate_xstate(p);
>>      recalculate_misc(p);
> 
> The CPUID logic needs quite a bit more than this, and to be safe on
> migrate.  For one, recalculate_xstate() unilaterally clobbers this to 0.

Oh, recalculate_misc() - yes, I see this now. And I have to admit
I don't see the migration-unsafety, so ...

> Shall I do a prep patch getting the CPUID side of things in order?

... yes, I'd appreciate you doing so.

>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -5670,6 +5671,52 @@ x86_emulate(
>>                  limit -= sizeof(zero);
>>              }
>>              break;
>> +
>> +        case 0xfd: /* rdpru */
>> +            vcpu_must_have(rdpru);
>> +
>> +            if ( !mode_ring0() )
>> +            {
>> +                fail_if(!ops->read_cr);
>> +                if ( (rc = ops->read_cr(4, &cr4, ctxt)) != X86EMUL_OKAY )
>> +                    goto done;
>> +                generate_exception_if(cr4 & X86_CR4_TSD, EXC_UD);
>> +            }
>> +
>> +            switch ( _regs.ecx )
>> +            {
>> +            case 0:  n = MSR_IA32_MPERF; break;
>> +            case 1:  n = MSR_IA32_APERF; break;
>> +            default: n = 0; break;
>> +            }
>> +            if ( _regs.ecx > ctxt->cpuid->extd.rdpru_max )
>> +                n = 0;
> 
> This can be folded into the switch statement.  Something like (
> _regs.ecx | -(_regs.ecx > ctxt->cpuid->extd.rdpru_max) )

Will do.

> Also, the sentinel might better be -1, which is not in a defined MSR
> block.  MSR 0 is a P5-compat MCE MSR, even on AMD hardware.

I did consider this, but decided there's a vanishingly small risk
for them to expose this MSR (and if they did we could still change
the code along the lines of what you say). A sentinel of zero is
slightly cheaper to have, after all.

Jan

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