Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Igor Mammedov]

On Wed, 6 Jun 2018 22:06:17 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Wed, Jun 06, 2018 at 11:08:49AM -0600, Ross Zwisler wrote:
> > On Wed, Jun 06, 2018 at 11:00:33AM -0600, Ross Zwisler wrote:  
> > > On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:  
> > > > On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:  
> > > > > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > > > > <address@hidden> wrote:  
> > > > > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote: 
> > > > > >  
> > > > > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:  
> > > > > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > > > > >> > <address@hidden> wrote:  
> > > > > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin 
> > > > > >> > > wrote:  
> > > > > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote: 
> > > > > >> > >>  
> > > > > >> > >> > Add a machine command line option to allow the user to 
> > > > > >> > >> > control the Platform
> > > > > >> > >> > Capabilities Structure in the virtualized NFIT.  This 
> > > > > >> > >> > Platform Capabilities
> > > > > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > > > > >> > >> >
> > > > > >> > >> > Signed-off-by: Ross Zwisler <address@hidden>  
> > > > > >> > >>
> > > > > >> > >> I tried playing with it and encoding the capabilities is
> > > > > >> > >> quite awkward.
> > > > > >> > >>
> > > > > >> > >> Can we add bits for specific capabilities instead of 
> > > > > >> > >> nvdimm-cap?
> > > > > >> > >>
> > > > > >> > >> How about:
> > > > > >> > >>
> > > > > >> > >> "cpu-flush-on-power-loss-cap"
> > > > > >> > >> "memory-flush-on-power-loss-cap"
> > > > > >> > >> "byte-addressable-mirroring-cap"  
> > > > > >> > >
> > > > > >> > > Hmmm...I don't like that as much because:
> > > > > >> > >
> > > > > >> > > a) It's very verbose.  Looking at my current qemu command line 
> > > > > >> > > few other
> > > > > >> > >    options require that many characters, and you'd commonly be 
> > > > > >> > > defining more
> > > > > >> > >    than one of these for a given VM.
> > > > > >> > >
> > > > > >> > > b) It means that the QEMU will need to be updated if/when new 
> > > > > >> > > flags are added,
> > > > > >> > >    because we'll have to have new options for each flag.  The 
> > > > > >> > > current
> > > > > >> > >    implementation is more future-proof because you can specify 
> > > > > >> > > any flags
> > > > > >> > >    value you want.
> > > > > >> > >
> > > > > >> > > However, if you feel strongly about this, I'll make the 
> > > > > >> > > change.  
> > > > > >> >
> > > > > >> > Straw-man: Could we do something similar with what we are doing 
> > > > > >> > in ndctl?
> > > > > >> >
> > > > > >> > enum ndctl_persistence_domain {
> > > > > >> >         PERSISTENCE_NONE = 0,
> > > > > >> >         PERSISTENCE_MEM_CTRL = 10,
> > > > > >> >         PERSISTENCE_CPU_CACHE = 20,
> > > > > >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > > > > >> > };
> > > > > >> >
> > > > > >> > ...and have the command line take a number where "10" and "20" 
> > > > > >> > are
> > > > > >> > supported today, but allows us to adapt to new persistence 
> > > > > >> > domains in
> > > > > >> > the future.  
> > > > > >>
> > > > > >> I'm fine with that except can we have symbolic names instead of 
> > > > > >> numbers
> > > > > >> on command line?
> > > > > >>
> > > > > >> --
> > > > > >> MST  
> > > > > >
> > > > > > Okay, we can move to the symbolic names.  Do you want them to be 
> > > > > > that long, or
> > > > > > would:
> > > > > >
> > > > > > nvdimm-cap-cpu
> > > > > > nvdimm-cap-mem-ctrl
> > > > > > nvdimm-cap-mirroring  
> > > > > 
> > > > > Wait, why is mirroring part of this?
> > > > > 
> > > > > I was thinking this option would be:
> > > > > 
> > > > >     --persistence-domain={cpu,mem-ctrl}
> > > > > 
> > > > > ...and try not to let ACPI specifics leak into the qemu command line
> > > > > interface. For example PowerPC qemu could have a persistence domain
> > > > > communicated via Open Firmware or some other mechanism.  
> > > > 
> > > > Sure, this seems fine, though we may want to throw an "nvdimm" in the 
> > > > name
> > > > somewhere so it's clear what the option affects.
> > > > 
> > > > nvdimm-persistence={cpu,mem-ctrl} maybe?
> > > > 
> > > > Michael, does this work for you?  
> > > 
> > > Hmm...also, the version of these patches that used numeric values did go
> > > upstream in QEMU.  So, do we need to leave that interface intact, and 
> > > just add
> > > a new one that uses symbolic names?  Or did you still just want to 
> > > replace the
> > > numeric one since it hasn't appeared in a QEMU release yet?  
> > 
> > I guess another alternative would be to just add symbolic name options to 
> > the
> > existing command line option, i.e. allow all of these:
> > 
> > nvdimm-cap=3                # CPU cache
> > nvdimm-cap=cpu              # CPU cache
> > nvdimm-cap=2                # memory controller
> > nvdimm-cap=mem-ctrl # memory controller
> > 
> > And just have them as aliases.  This retains backwards compatibility with
> > what is already there, allows for other numeric values without QEMU updates 
> > if
> > other bits are defined (though we are still tightly tied to ACPI in this
> > case), and provides a symbolic name which is easier to use.
> > 
> > Thoughts?  
> 
> I'm inclined to say let's just keep the symbolic names.
> Less of a chance users configure something incorrectly,
> it somehow kind of works and then we get stuck with working
> around these bugs.
We used numeric values before (OSPM status) in QMP which is ACPI spec defined,
so we probably should do so in this case as well, as far as there is clear
explanation for user where values come from. We can have symbolic aliases,
but it would be just extra burden to maintain (so I'd prefer not to have it).