Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.03.2021 17:18, Andrew Cooper wrote:
> In hindsight, this was a poor move.  Some of these MSRs require probing for,
> causing unhelpful spew into xl dmesg, as well as spew from unit tests
> explicitly checking behaviour.

I can indeed see your point for MSRs that require probing. But what about
the others (which, as it seems, is the majority)? And perhaps specifically
what about the entire WRMSR side, which won't be related to probing? I'm
not opposed to the change, but I'd like to understand the reasoning for
every one of the MSRs, not just a subset.

Of course such ever-growing lists of case labels aren't very nice - this
going away was one of the things I particularly liked about the original
change.

Jan

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -175,6 +175,30 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t 
> *val)
>  
>      switch ( msr )
>      {
> +        /* Write-only */
> +    case MSR_AMD_PATCHLOADER:
> +    case MSR_IA32_UCODE_WRITE:
> +    case MSR_PRED_CMD:
> +    case MSR_FLUSH_CMD:
> +
> +        /* Not offered to guests. */
> +    case MSR_TEST_CTRL:
> +    case MSR_CORE_CAPABILITIES:
> +    case MSR_TSX_FORCE_ABORT:
> +    case MSR_TSX_CTRL:
> +    case MSR_MCU_OPT_CTRL:
> +    case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
> +    case MSR_U_CET:
> +    case MSR_S_CET:
> +    case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
> +    case MSR_AMD64_LWP_CFG:
> +    case MSR_AMD64_LWP_CBADDR:
> +    case MSR_PPIN_CTL:
> +    case MSR_PPIN:
> +    case MSR_AMD_PPIN_CTL:
> +    case MSR_AMD_PPIN:
> +        goto gp_fault;
> +
>      case MSR_IA32_FEATURE_CONTROL:
>          /*
>           * Architecturally, availability of this MSR is enumerated by the
> @@ -382,6 +406,30 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
> val)
>      {
>          uint64_t rsvd;
>  
> +        /* Read-only */
> +    case MSR_IA32_PLATFORM_ID:
> +    case MSR_CORE_CAPABILITIES:
> +    case MSR_INTEL_CORE_THREAD_COUNT:
> +    case MSR_INTEL_PLATFORM_INFO:
> +    case MSR_ARCH_CAPABILITIES:
> +
> +        /* Not offered to guests. */
> +    case MSR_TEST_CTRL:
> +    case MSR_TSX_FORCE_ABORT:
> +    case MSR_TSX_CTRL:
> +    case MSR_MCU_OPT_CTRL:
> +    case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
> +    case MSR_U_CET:
> +    case MSR_S_CET:
> +    case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
> +    case MSR_AMD64_LWP_CFG:
> +    case MSR_AMD64_LWP_CBADDR:
> +    case MSR_PPIN_CTL:
> +    case MSR_PPIN:
> +    case MSR_AMD_PPIN_CTL:
> +    case MSR_AMD_PPIN:
> +        goto gp_fault;
> +
>      case MSR_AMD_PATCHLEVEL:
>          BUILD_BUG_ON(MSR_IA32_UCODE_REV != MSR_AMD_PATCHLEVEL);
>          /*
>