Re: [PATCH v2 07/15] x86: implement set value flow for MBA

From: Yi Sun <yi.y.sun@linux.intel.com>
To: "Roger Pau Monn�" <roger.pau@citrix.com>
Cc: kevin.tian@intel.com, wei.liu2@citrix.com,
	andrew.cooper3@citrix.com, dario.faggioli@citrix.com,
	ian.jackson@eu.citrix.com, julien.grall@arm.com,
	mengxu@cis.upenn.edu, jbeulich@suse.com,
	chao.p.peng@linux.intel.com, xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 07/15] x86: implement set value flow for MBA
Date: Thu, 31 Aug 2017 17:13:08 +0800	[thread overview]
Message-ID: <20170831091308.GG23665@yi.y.sun> (raw)
In-Reply-To: <20170831083047.v4pdfkwm3bjigjhy@MacBook-Pro-de-Roger.local>

On 17-08-31 09:30:47, Roger Pau Monn� wrote:
> On Thu, Aug 31, 2017 at 10:20:10AM +0800, Yi Sun wrote:
> > On 17-08-30 09:31:04, Roger Pau Monn� wrote:
> > > On Thu, Aug 24, 2017 at 09:14:41AM +0800, Yi Sun wrote:
> > > > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> > > > index 4a0c982..ce82975 100644
> > > > --- a/xen/arch/x86/psr.c
> > > > +++ b/xen/arch/x86/psr.c
> > > > @@ -138,6 +138,12 @@ static const struct feat_props {
> > > >  
> > > >      /* write_msr is used to write out feature MSR register. */
> > > >      void (*write_msr)(unsigned int cos, uint32_t val, enum psr_val_type type);
> > > > +
> > > > +    /*
> > > > +     * check_val is used to check if input val fulfills SDM requirement.
> > > > +     * Change it to valid value if SDM allows.
> > > 
> > > I'm not really sure it's a good idea to change the value to a valid
> > > one, IMHO you should just check and print an error if the value is
> > > invalid (and return false of course).
> > > 
> > Per SDM, the HW has ability to automatically change the input value to what it
> > wants. E.g:
> >   Linear mode: HW wants the input value be 10/20/30/.../90. But user inputs 15.
> >                Then, HW can automatically change it to 10.
> 
> This seems like a very, very bad idea IMHO. What if I input 15 (which
> is invalid), but I would rather prefer 20 instead of 10?
> 
> I would very much prefer an error, so I can get to chose a valid value
> myself instead of the hardware deciding on my back.
> 
> > Even user inputs a value that does not fulfill HW requirement, HW can handle it.
> > So, we do not need return error to user. Otherwise, user needs to know details
> > of MBA.
> > 
> > But the issue here is how we get the actual value and show it to user. There are
> > two ways to do that:
> > 1. When setting value, check and change it to valid one and save it to our cache.
> 
> Instead of performing those checks manually, why not simply write the
> value and read it back to see the one the hardware has actually
> chosen (and cache it)?
> 
So, I guess you prefer optioin 2, right? If so, do we need check if input value
is valid? I see above comment that you prefer to return an error and leave the
decision to user.

> Is there any risk in writing an invalid value?

If input value is no more than the thrtl_max got through CPUID, it is fine that
HW can handle it.

> 
> > 2. When getting value, call rdmsr to read the actual value back from HW.
> > 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel