Re: [PATCH 3/4] x86: Allow non-faulting accesses to non-emulated MSRs if policy permits this

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2021 21:34, Boris Ostrovsky wrote:
> Starting with commit 84e848fd7a16 ("x86/hvm: disallow access to unknown MSRs")
> accesses to unhandled MSRs result in #GP sent to the guest. This caused a
> regression for Solaris who tries to acccess MSR_RAPL_POWER_UNIT and (unlike,

Nit: One c too many.

> for example, Linux) does not catch exceptions when accessing MSRs that
> potentially may not be present.

Just to re-raise the question raised by Andrew already earlier
on: Has Solaris been fixed in the meantime, or is this at least
firmly planned to happen?

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1965,8 +1965,8 @@ static int svm_msr_read_intercept(unsigned int msr, 
> uint64_t *msr_content)
>          break;
>  
>      default:
> -        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr);
> -        goto gpf;
> +        if ( guest_unhandled_msr(v, msr, msr_content, false) )
> +            goto gpf;
>      }
>  
>      HVM_DBG_LOG(DBG_LEVEL_MSR, "returns: ecx=%x, msr_value=%"PRIx64,
> @@ -2151,10 +2151,8 @@ static int svm_msr_write_intercept(unsigned int msr, 
> uint64_t msr_content)
>          break;
>  
>      default:
> -        gdprintk(XENLOG_WARNING,
> -                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
> -                 msr, msr_content);
> -        goto gpf;
> +        if ( guest_unhandled_msr(v, msr, &msr_content, true) )
> +            goto gpf;
>      }
>  
>      return X86EMUL_OKAY;
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3017,8 +3017,8 @@ static int vmx_msr_read_intercept(unsigned int msr, 
> uint64_t *msr_content)
>              break;
>          }
>  
> -        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr);
> -        goto gp_fault;
> +        if ( guest_unhandled_msr(curr, msr, msr_content, false) )
> +            goto gp_fault;
>      }
>  
>  done:
> @@ -3319,10 +3319,8 @@ static int vmx_msr_write_intercept(unsigned int msr, 
> uint64_t msr_content)
>               is_last_branch_msr(msr) )
>              break;
>  
> -        gdprintk(XENLOG_WARNING,
> -                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
> -                 msr, msr_content);
> -        goto gp_fault;
> +        if ( guest_unhandled_msr(v, msr, &msr_content, true) )
> +            goto gp_fault;
>      }
>  
>      return X86EMUL_OKAY;

These functions also get used from the insn emulator, when it
needs to fetch an MSR value (not necessarily in the context of
emulating RDMSR or WRMSR). I wonder whether applying this
behavior in that case is actually correct. It would only be if
we would settle on it being a requirement that any such MSRs
have to have emulation present in one of the handlers.

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -164,6 +164,26 @@ int init_vcpu_msr_policy(struct vcpu *v)
>      return 0;
>  }
>  
> +/* Returns true if policy requires #GP to the guest. */
> +bool guest_unhandled_msr(const struct vcpu *v, uint32_t msr,
> +                         uint64_t *val, bool is_write)
> +{
> +    const struct msr_policy *mp = v->domain->arch.msr;
> +
> +    if ( unlikely(mp->ignore_msrs != MSR_UNHANDLED_NEVER) && !is_write )
> +        *val = 0;

I'd recommend to drop the left side of the && - there's no harm
in storing zero in this extra case.

> +    if ( likely(mp->ignore_msrs != MSR_UNHANDLED_SILENT) ) {

Nit: Brace on its own line please.

> --- a/xen/include/asm-x86/msr.h
> +++ b/xen/include/asm-x86/msr.h
> @@ -345,5 +345,6 @@ int init_vcpu_msr_policy(struct vcpu *v);
>   */
>  int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val);
>  int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val);
> -
> +bool guest_unhandled_msr(const struct vcpu *v, uint32_t msr,
> +                         uint64_t *val, bool is_write);
>  #endif /* __ASM_MSR_H */

Please retain the blank line that was there.

Jan