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

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

On Tue, Jul 21, 2020 at 06:22:08PM +0100, 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.
                            ^ at
> 
> 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.
                                                   ^ extra is
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I've got one question (not really patch related).

> @@ -1956,10 +1962,10 @@ static int svm_msr_read_intercept(unsigned int msr, 
> uint64_t *msr_content)
>  
>  static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>  {
> -    int ret, result = X86EMUL_OKAY;
>      struct vcpu *v = current;
>      struct domain *d = v->domain;
>      struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
> +    struct nestedsvm *nsvm = &vcpu_nestedsvm(v);
>  
>      switch ( msr )
>      {
> @@ -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 )

Regarding the address check, the PM states "the maximum supported
physical address for this implementation", but I don't seem to be able
to find where is this actually announced.

Roger.