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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2021 21:34, Boris Ostrovsky wrote:
> --- 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:

What about ones handled by emulation, when emulation decides to
raise #GP and this still causes trouble to a guest? (Slightly
orthogonal, so we may want to defer this aspect.)

> + *  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 -1

Not sure about this choice: I'd consider an MSR index in the Xen
range more safe to use, not the least because this value
effectively becomes part of the migration ABI. Or if you use one
outside, then maybe a less prominent one than 0xffffffff (I
guess -1, irrespective of the missing parentheses, isn't
appropriate to use here).

> @@ -77,7 +78,7 @@ int x86_msr_copy_from_buffer(struct msr_policy *p,
>          if ( copy_from_buffer_offset(&data, msrs, i, 1) )
>              return -EFAULT;
>  
> -        if ( data.flags ) /* .flags MBZ */
> +        if ( data.idx != MSR_UNHANDLED && data.flags )

You permit all flags bits set here, but ...

> @@ -101,6 +102,7 @@ int x86_msr_copy_from_buffer(struct msr_policy *p,
>  
>          case MSR_INTEL_PLATFORM_INFO: ASSIGN(platform_info.raw); break;
>          case MSR_ARCH_CAPABILITIES:   ASSIGN(arch_caps.raw);     break;
> +        case MSR_UNHANDLED:           p->ignore_msrs = data.flags & 0xff;    
>    break;

... you use only the low 8 ones here. You want to forbid any we
don't know of, and even restrict the low two ones to just the
three values you define. E.g. for now

        if ( data.idx != MSR_UNHANDLED ? data.flags
                                       : data.flags > MSR_UNHANDLED_VERBOSE )
        {
            rc = -EINVAL;
            goto err;

Otoh I don't see why you need to use flags here - I think you
could as well use the MSR value to convey the setting. And if
you don't, you'd want to check the value to be zero.

Nit: You can shorten line length by omitting the "& 0xff" and
reducing the number of blanks ahead of "break;".

Jan