Re: [PATCH] x86/svm: Fold nsvm_{wr,rd}msr() into svm_msr_{read,write}_intercept()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/07/2020 10:16, Jan Beulich wrote:
> On 21.07.2020 19:22, Andrew Cooper wrote:
>> ... to simplify the default cases.
>>
>> There are multiple errors with the handling of these three MSRs, but they are
>> deliberately not addressed this point.
>>
>> This removes the dance converting -1/0/1 into X86EMUL_*, allowing for the
>> removal of the 'ret' variable.
>>
>> While cleaning this up, drop the gdprintk()'s for #GP conditions, and the
>> 'result' variable from svm_msr_write_intercept() is it is never modified.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> However, ...
>
>> @@ -2085,6 +2091,22 @@ static int svm_msr_write_intercept(unsigned int msr, 
>> uint64_t msr_content)
>>              goto gpf;
>>          break;
>>  
>> +    case MSR_K8_VM_CR:
>> +        /* ignore write. handle all bits as read-only. */
>> +        break;
>> +
>> +    case MSR_K8_VM_HSAVE_PA:
>> +        if ( (msr_content & ~PAGE_MASK) || msr_content > 0xfd00000000ULL )
> ... while you're moving this code here, wouldn't it be worthwhile
> to at least fix the > to be >= ?

I'd prefer not to, to avoid breaking the "No Functional Change" aspect.

In reality, this needs to be a path which takes an extra ref on the
nominated frame and globally maps it, seeing as we memcpy to/from it on
every virtual vmentry/exit.  The check against the HT range is quite bogus.

~Andrew