Re: [Xen-devel] [PATCH v2 5/5] x86/msr: Blacklist various MSRs which guests definitely shouldn't be using

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.03.18 at 19:58, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -197,7 +197,28 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>          ret = guest_rdmsr_xen(v, msr, val);
>          goto out;
>  
> +        /* Specific blacklisted MSRs while the legacy handlers still exist. 
> */
> +    case MSR_SGX_PUBKEY_HASH(0) ... MSR_SGX_PUBKEY_HASH(3):

Did you intentionally misalign the comment wrt the case labels?

> +    case MSR_SGX_SVN_STATUS:
> +    case MSR_DEBUG_INTERFACE:
> +    case MSR_L3_QOS_CFG:
> +    case MSR_L2_QOS_CFG:
> +    case MSR_QM_EVTSEL:
> +    case MSR_QM_CTR:
> +    case MSR_PQR_ASSOC:
> +    case MSR_CAT_MASK_START ... MSR_CAT_MASK_LAST:
> +        goto gp_fault;
> +
>      default:
> +        /*
> +         * Blacklist the architecturally inaccessable MSRs. No point 
> wandering
> +         * the legacy handlers.
> +         */
> +        if ( msr > 0x1fff &&
> +             (msr < 0xc0000000 || msr > 0xc0001fff) &&
> +             (msr < 0xc0010000 || msr > 0xc0011fff) )
> +            goto gp_fault;

I wonder what you derive "architecturally inaccessable" from: On
one hand nothing exists there at present. Otoh it looks to me as if
you derive that state from MSRs outside these ranges always
being intercepted (which is not the same as inaccessable, just take
the hypervisor MSR as example). And then the last of these ranges
would appear to be AMD specific.

> @@ -69,6 +71,18 @@
>  /* Lower 6 bits define the format of the address in the LBR stack */
>  #define MSR_IA32_PERF_CAP_LBR_FORMAT 0x3f
>  
> +#define MSR_SGX_SVN_STATUS           0x00000500
> +
> +#define MSR_DEBUG_INTERFACE          0x00000c80
> +
> +#define MSR_L3_QOS_CFG                       0x00000c81
> +#define MSR_L2_QOS_CFG                       0x00000c82
> +#define MSR_QM_EVTSEL                        0x00000c8d
> +#define MSR_QM_CTR                   0x00000c8e
> +#define MSR_PQR_ASSOC                        0x00000c8f
> +#define MSR_CAT_MASK_START           0x00000c90
> +#define MSR_CAT_MASK_LAST            0x00000d8f

For one, MSR_CAT_MASK_FIRST to better pair with ..._LAST
again? And then - why all these duplicates? We already have

/* Platform Shared Resource MSRs */
#define MSR_IA32_CMT_EVTSEL             0x00000c8d
#define MSR_IA32_CMT_EVTSEL_UE_MASK     0x0000ffff
#define MSR_IA32_CMT_CTR                0x00000c8e
#define MSR_IA32_PSR_ASSOC              0x00000c8f
#define MSR_IA32_PSR_L3_QOS_CFG 0x00000c81
#define MSR_IA32_PSR_L3_MASK(n) (0x00000c90 + (n))
#define MSR_IA32_PSR_L3_MASK_CODE(n)    (0x00000c90 + (n) * 2 + 1)
#define MSR_IA32_PSR_L3_MASK_DATA(n)    (0x00000c90 + (n) * 2)
#define MSR_IA32_PSR_L2_MASK(n)         (0x00000d10 + (n))
#define MSR_IA32_PSR_MBA_MASK(n)        (0x00000d50 + (n))

and considering that we supposedly have L2 support I'm a little
puzzled that there's no #define of use of 0xc82 so far.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel