Re: [PATCH v16 08/10] x86: add CMT related MSRs in allowed list

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Sep 26, 2014 at 09:38:35AM +0100, Jan Beulich wrote:
> >>> On 25.09.14 at 22:58, <konrad.wilk@oracle.com> wrote:
> > On Thu, Sep 25, 2014 at 06:19:08PM +0800, Chao Peng wrote:
> >> Tool stack will try to access the two MSRs to perform CMT
> >> related operations, thus added them in the allowed list.
> >> 
> >> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
> >> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> >> ---
> >>  xen/arch/x86/platform_hypercall.c |    7 +++++++
> >>  xen/include/asm-x86/msr-index.h   |    2 ++
> >>  2 files changed, 9 insertions(+)
> >> 
> >> diff --git a/xen/arch/x86/platform_hypercall.c 
> > b/xen/arch/x86/platform_hypercall.c
> >> index 081d9f5..be06f3a 100644
> >> --- a/xen/arch/x86/platform_hypercall.c
> >> +++ b/xen/arch/x86/platform_hypercall.c
> >> @@ -69,6 +69,13 @@ struct xen_resource_access {
> >>  
> >>  static bool_t allow_access_msr(unsigned int msr)
> >>  {
> >> +    switch ( msr )
> >> +    {
> >> +    case MSR_IA32_QOSEVTSEL:
> >> +    case MSR_IA32_QMC:
> >> +        return 1;
> >> +    }
> >> +
> >>      return 0;
> >>  }
> >>  
> >> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> >> index dcb2b87..ae089fb 100644
> >> --- a/xen/include/asm-x86/msr-index.h
> >> +++ b/xen/include/asm-x86/msr-index.h
> >> @@ -324,6 +324,8 @@
> >>  #define MSR_IA32_ENERGY_PERF_BIAS	0x000001b0
> >>  
> >>  /* Platform Shared Resource MSRs */
> >> +#define MSR_IA32_QOSEVTSEL		0x00000c8d
> >> +#define MSR_IA32_QMC			0x00000c8e
> > 
> > Could you mention where they are in the SDM ?
> 
> In the MSR related appendix of course. Let's not go overboard with
> adding all kinds of information here that was never added for other
> MSRs: The header's purpose is just giving names to numbers.

They are of course there, but the explanation of how to use it or what
they are good for in usually in other chapters in the SDM. I was hoping
an chapter to just that. But if the header is not the right location
perhaps then in 'allow_access_msr' function?

Anyhow not going to argue strongly for it - as it is a minor part
of the patches - so either way I am comfortable with it.

> 
> Jan
>