Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
On Wed, Jun 06, 2018 at 11:20:58PM +, Elliott, Robert (Persistent Memory) wrote: > > > > 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? > > This data structure is intended to report any kind of platform capability, > not > just platform persistence capabilities. > > We could add a short symbolic name to the definitions in ACPI that matches > the ones selected for this command line option, if that'll help people > find the right names to use. > > I recommend mc rather than mem-ctrl to keep dashes as special. > I'm not sure it's a good idea: dashes aren't special in qemu, and mc's way too brief. -- MST ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
On Wed, Jun 06, 2018 at 04:40:31PM -0700, Dan Williams wrote: > On Wed, Jun 6, 2018 at 4:20 PM, Elliott, Robert (Persistent Memory) > wrote: > > > >> > 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? > > > > This data structure is intended to report any kind of platform capability, > > not > > just platform persistence capabilities. > > Yes, but here's nothing actionable that a qemu guest OS can do with > that mirroring information, so there's no need at this time to add cli > cruft and code to support it. I agree. -- MST ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
On Wed, Jun 6, 2018 at 4:20 PM, Elliott, Robert (Persistent Memory) wrote: > >> > 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? > > This data structure is intended to report any kind of platform capability, not > just platform persistence capabilities. Yes, but here's nothing actionable that a qemu guest OS can do with that mirroring information, so there's no need at this time to add cli cruft and code to support it. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
RE: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
> > 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? This data structure is intended to report any kind of platform capability, not just platform persistence capabilities. We could add a short symbolic name to the definitions in ACPI that matches the ones selected for this command line option, if that'll help people find the right names to use. I recommend mc rather than mem-ctrl to keep dashes as special. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
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
> > 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
> > >> > 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
> > >> > >>
> > >> > >> 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?
Sounds good to me.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
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
> > > > 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
> > > > >> > 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
> > > > >> > >>
> > > > >> > >> 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
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
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
> > > 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
> > > >> > 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
> > > >> > >>
> > > >> > >> 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?
The later. No release => no stable APIs.
--
MST
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
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
> > > 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
> > > >> > 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
> > > >> > >>
> > > >> > >> 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?
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
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
> > 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
> > >> > 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
> > >> > >>
> > >> > >> 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?
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> 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
> >> > 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
> >> > >>
> >> > >> 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?
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
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
>> > 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
>> > >>
>> > >> 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.
>
> or something be better?
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
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
> > 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
> > >>
> > >> 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
or something be better?
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> 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
> >>
> >> 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
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
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
>>
>> 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.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
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 > > 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. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities
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
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"
?
> ---
> docs/nvdimm.txt | 27 +++
> hw/acpi/nvdimm.c| 45 +
> hw/i386/pc.c| 31 +++
> include/hw/i386/pc.h| 1 +
> include/hw/mem/nvdimm.h | 5 +
> 5 files changed, 105 insertions(+), 4 deletions(-)
>
> diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
> index e903d8bb09..8b48fb4633 100644
> --- a/docs/nvdimm.txt
> +++ b/docs/nvdimm.txt
> @@ -153,3 +153,30 @@ guest NVDIMM region mapping structure. This unarmed
> flag indicates
> guest software that this vNVDIMM device contains a region that cannot
> accept persistent writes. In result, for example, the guest Linux
> NVDIMM driver, marks such vNVDIMM device as read-only.
> +
> +Platform Capabilities
> +-
> +
> +ACPI 6.2 Errata A added support for a new Platform Capabilities Structure
> +which allows the platform to communicate what features it supports related to
> +NVDIMM data durability. Users can provide a capabilities value to a guest
> via
> +the optional "nvdimm-cap" machine command line option:
> +
> +-machine pc,accel=kvm,nvdimm,nvdimm-cap=2
> +
> +This "nvdimm-cap" field is an integer, and is the combined value of the
> +various capability bits defined in table 5-137 of the ACPI 6.2 Errata A spec.
> +
> +Here is a quick summary of the three bits that are defined as of that spec:
> +
> +Bit[0] - CPU Cache Flush to NVDIMM Durability on Power Loss Capable.
> +Bit[1] - Memory Controller Flush to NVDIMM Durability on Power Loss Capable.
> + Note: If bit 0 is set to 1 then this bit shall be set to 1 as well.
> +Bit[2] - Byte Addressable Persistent Memory Hardware Mirroring Capable.
> +
> +So, a "nvdimm-cap" value of 2 would mean that the platform supports Memory
> +Controller Flush on Power Loss, a value of 3 would mean that the platform
> +supports CPU Cache Flush and Memory Controller Flush on Power Loss, etc.
> +
> +For a complete list of the flags available and for more detailed
> descriptions,
> +please consult the ACPI spec.
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 59d6e4254c..87e4280c71 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -169,6 +169,21 @@ struct NvdimmNfitControlRegion {
> } QEMU_PACKED;
> typedef struct NvdimmNfitControlRegion NvdimmNfitControlRegion;
>
> +/*
> + * NVDIMM Platform Capabilities Structure
> + *
> + * Defined in section 5.2.25.9 of ACPI 6.2 Errata A, September 2017
> + */
> +struct NvdimmNfitPlatformCaps {
> +uint16_t type;
> +uint16_t length;
> +uint8_t highest_cap;
> +uint8_t reserved[3];
> +uint32_t capabilities;
> +uint8_t reserved2[4];
> +} QEMU_PACKED;
> +typedef struct NvdimmNfitPlatformCaps NvdimmNfitPlatformCaps;
> +
> /*
> * Module serial number is a unique number for each device. We use the
> * slot id of NVDIMM device to generate this number so that each device
> @@ -351,7 +366,23 @@ static void nvdimm_build_structure_dcr(GArray
> *structures, DeviceState *dev)
> JEDEC Annex L Release 3. */);
> }
>
> -static GArray *nvdimm_build_device_structure(void)
> +/*
> + * ACPI 6.2 Errata A: 5.2.25.9 NVDIMM Platform Capabilities Structure
> + */
> +static void
> +nvdimm_build_structure_caps(GArray *structures, uint32_t capabilities)
> +{
> +NvdimmNfitPlatformCaps *nfit_caps;
> +
> +nfit_caps = acpi_data_push(structures, sizeof(*nfit_caps));
> +
> +nfit_caps->type = cpu_to_le16(7 /* NVDIMM Platform Capabilities */);
> +nfit_caps->length = cpu_to_le16(sizeof(*nfit_caps));
> +nfit_caps->highest_cap = 31 - clz32(capabilities);
> +nfit_caps->capabilities = cpu_to_le32(capabilities);
> +}
> +
> +static GArray *nvdimm_build_device_structure(AcpiNVDIMMState *state)
> {
> GSList *device_list = nvdimm_get_device_list();
> GArray *structures = g_array_new(false, true /* clear */, 1);
> @@ -373,6 +404,10 @@ static GArray *nvdimm_build_device_structure(void)
> }
> g_slist_free(device_list);
>
> +if (state->capabilities) {
> +nvdimm_build_structure_caps(structures, state->capabilities);
> +}
> +
> return structures;
> }
>
> @@ -381,16 +416,18 @@ static void nvdimm_init_fit_buffer(NvdimmFitBuffer
> *fit_buf)
> fit_buf->fit = g_array_new(false, true /* clear */, 1);
> }
>
> -static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
