Re: [PATCH v2 6/8] x86/hvm: Enable guest access to MSR_PKRS

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 12/01/2023 1:26 pm, Jan Beulich wrote:
> On 10.01.2023 18:18, Andrew Cooper wrote:
>> @@ -2471,6 +2477,9 @@ static uint64_t cf_check vmx_get_reg(struct vcpu *v, 
>> unsigned int reg)
>>          }
>>          return val;
>>  
>> +    case MSR_PKRS:
>> +        return (v == curr) ? rdpkrs() : msrs->pkrs;
> Nothing here or ...
>
>> @@ -2514,6 +2525,12 @@ static void cf_check vmx_set_reg(struct vcpu *v, 
>> unsigned int reg, uint64_t val)
>>              domain_crash(d);
>>          }
>>          return;
>> +
>> +    case MSR_PKRS:
>> +        msrs->pkrs = val;
>> +        if ( v == curr )
>> +            wrpkrs(val);
>> +        return;
> ... here is VMX or (if we were to support it, just as a abstract
> consideration) HVM specific. Which makes me wonder why this needs
> handling in [gs]et_reg() in the first place. I guess I'm still not
> fully in sync with your longer term plans here ...

If (when) AMD implement it, the AMD form needs will be vmcb->pkrs and
not msrs->pkrs, because like all other paging controls, they'll be
swapped automatically by VMRUN.

(I don't know this for certain, but I'm happy to bet on it, given a
decade of consistency in this regard.)

> The other thing I'd like to understand (and having an answer to this
> would have been better before re-applying my R-b to this re-based
> logic) is towards the lack of feature checks here. hvm_get_reg()
> can be called from other than guest_rdmsr(), for an example see
> arch_get_info_guest().

The point is to separate auditing logic (wants to be implemented only
once) from data shuffling logic (is the value in a register, or the MSR
lists, or VMCB/VMCS or struct vcpu, etc).  It is always the caller's
responsibility to confirm that REG exists, and that VAL is suitable for REG.

arch_get_info_guest() passes MSR_SHADOW_GS_BASE which exists
unilaterally (because we don't technically do !LM correctly.)


But this is all discussed in the comment by the function prototypes. 
I'm not sure how to make that any clearer than it already is.

~Andrew