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

From: "Jan Beulich" <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Sergey Dyasli" <sergey.dyasli@citrix.com>,
	Xen-devel <xen-devel@lists.xen.org>,
	"Wei Liu" <wei.liu2@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v2 5/5] x86/msr: Blacklist various MSRs which guests definitely shouldn't be using
Date: Tue, 13 Mar 2018 09:35:28 -0600	[thread overview]
Message-ID: <5AA7FDD002000078001B1146@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1520449116-15443-6-git-send-email-andrew.cooper3@citrix.com>

>>> On 07.03.18 at 19:58, <andrew.cooper3@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel