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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.07.2020 12:10, Andrew Cooper wrote:
> 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.

Well, so be it then.

> 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.

I agree; I merely found the > so very obviously off-by-one that I
thought I'd at least inquire.

Jan