Re: [PATCH v2 2/4] x86: Introduce MSR_UNHANDLED

[Jan Beulich <jbeulich@suse.com>]

On 20.01.2021 23:49, Boris Ostrovsky wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -295,7 +295,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>          }
>  
>          /* Fallthrough. */
> -    case 0x40000200 ... 0x400002ff:
> +    case 0x40000200 ... 0x400002fe:
>          ret = guest_rdmsr_xen(v, msr, val);
>          break;
>  
> @@ -514,7 +514,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>          }
>  
>          /* Fallthrough. */
> -    case 0x40000200 ... 0x400002ff:
> +    case 0x40000200 ... 0x400002fe:
>          ret = guest_wrmsr_xen(v, msr, val);
>          break;

For both of these, we need some kind of connection to
MSR_UNHANDLED. Could be a BUILD_BUG_ON() or an explicit
"case MSR_UNHANDLED:" (preventing someone "correcting" the
apparent mistake) or yet something else.

> --- a/xen/include/xen/lib/x86/msr.h
> +++ b/xen/include/xen/lib/x86/msr.h
> @@ -2,8 +2,21 @@
>  #ifndef XEN_LIB_X86_MSR_H
>  #define XEN_LIB_X86_MSR_H
>  
> +/*
> + * Behavior on accesses to MSRs that are not handled by emulation:
> + *  0 = return #GP, warning emitted
> + *  1 = read as 0, writes are dropped, no warning
> + *  2 = read as 0, writes are dropped, warning emitted
> + */
> +#define MSR_UNHANDLED_NEVER     0
> +#define MSR_UNHANDLED_SILENT    1
> +#define MSR_UNHANDLED_VERBOSE   2
> +
> +/* MSR that is not explicitly processed by emulation */
> +#define MSR_UNHANDLED           0x400002ff

MSR indexes as well as definitions for MSR contents generally
live in asm-x86/msr-index.h. I think it would be better for
the above to also go there.

Additionally the comment on MSR_UNHANDLED should not only
say what will not happen for this index, but also what its
intended purpose is.

> @@ -45,6 +58,8 @@ struct msr_policy
>              bool taa_no:1;
>          };
>      } arch_caps;
> +
> +    uint8_t ignore_msrs;

Add a brief comment along the lines of /* MSR_UNHANDLED_* */
here to make the connection to intended values?

Also, Andrew, (I think I did say so before) - I definitely
would want your general consent with this model, as what gets
altered here is almost all relatively recent contributions
by you. Nor would I exclude the approach being controversial.

Jan