Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Wed, Jul 5, 2017 at 4:14 PM, Jerry Hoemann  wrote:
> On Wed, Jul 05, 2017 at 09:35:48AM -0700, Dan Williams wrote:
[..]
>> It was a mistake to use _DSM for common root-level functionality, and
>> we shouldn't double down on that mistake by allowing unfettered
>
> As to the moral aspects of ACPI's decision to standardiz the DSM for NVDIMM,
> I take no position on whether it was a good thing or a bad thing; but it
> is a thing.  We need to handle it.  I see no particular benefit to
> making our own lives more difficult.

We do handle everything we need to. Making future updates move at the
same pace as standard ACPI enabing is the goal as well as not adding
any momentum to continue abusing _DSM when we should be creating named
methods for bus-level generic functionality. As a maintainer of this
subsystem I'm fine with the burden of continuing to touch the code as
the specification evolves and that stance matches standard Linux
practice.

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Wed, Jul 5, 2017 at 4:14 PM, Jerry Hoemann  wrote:
> On Wed, Jul 05, 2017 at 09:35:48AM -0700, Dan Williams wrote:
[..]
>> It was a mistake to use _DSM for common root-level functionality, and
>> we shouldn't double down on that mistake by allowing unfettered
>
> As to the moral aspects of ACPI's decision to standardiz the DSM for NVDIMM,
> I take no position on whether it was a good thing or a bad thing; but it
> is a thing.  We need to handle it.  I see no particular benefit to
> making our own lives more difficult.

We do handle everything we need to. Making future updates move at the
same pace as standard ACPI enabing is the goal as well as not adding
any momentum to continue abusing _DSM when we should be creating named
methods for bus-level generic functionality. As a maintainer of this
subsystem I'm fine with the burden of continuing to touch the code as
the specification evolves and that stance matches standard Linux
practice.

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Jerry Hoemann]

On Wed, Jul 05, 2017 at 09:35:48AM -0700, Dan Williams wrote:
> On Wed, Jul 5, 2017 at 9:24 AM, Jerry Hoemann  wrote:
> > On Tue, Jul 04, 2017 at 01:37:43PM -0700, Dan Williams wrote:
> >> On Tue, Jul 4, 2017 at 1:08 PM, Jerry Hoemann  
> >> wrote:
> >> > On Sat, Jul 01, 2017 at 01:46:03PM -0700, Dan Williams wrote:
> >> >> On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  
> >> >> wrote:
> >> >> > On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
> >> >> >> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams 
> >> >> >>  wrote:
> >> >> >> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann 
> >> >> >> >  wrote:
> >> >> >> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
> >> >> >> >>
> >> >> >> >> ...
> >
> > ...
> >
> >> >> >> >>>
> >> >> >> >>> This drops function number 0 which userspace has no need to call.
> >> >> >> >>
> >> >> >> >> Actually I like to call function 0.  Its an excellent test when
> >> >> >> >> modifying the code path as its a no side effects function whose 
> >> >> >> >> output
> >> >> >> >> is known in advance and instantly recognizable.  I also use it 
> >> >> >> >> when
> >> >> >> >> testing new firmware.
> >> >> >> >>
> >> >> >> >> What is the downside to allowing it?  What bad things happen?
> >> >> >> >
> >> >> >> > It allows implementations to bypass the standardization process and
> >> >> >> > ship new root DSMs. It's always possible to patch the kernel 
> >> >> >> > locally
> >> >> >> > for development, so I see no reason to ship this capability 
> >> >> >> > globally.
> >> >> >
> >> >> > I don't understand this comment, but I think your next comment
> >> >> > essentially says to disregard this comment?
> >> >>
> >> >> Yes, sorry.
> >> >>
> >> >> >> Actually, just the discovery portion does not lead to this leak, but
> >> >> >> it's redundant when we have the 'dsm_mask' sysfs attribute.
> >> >> >
> >> >> > No.  The generation of the mask in sysfs is not done by
> >> >> > executing the code in acpi_nfit_ctl.  One of the reasons I call
> >> >> > function 0 to test changes I am making to the ioctl path itself.
> >> >> > The sysfs has nothing to do with that path and cannot be used
> >> >> > to serve this purpose.
> >> >> >
> >> >> > And since the content of sysfs has been edited it also can not be
> >> >> > used as a basic test of firmware.
> >> >> >
> >> >> > What is the downside to allowing the calling of function 0?
> >> >>
> >> >> It needlessly expands the kernel ABI. I would suggest, if you want to
> >> >
> >> > No.  It is not needless.  It is not an ABI extension.
> >> > Same goes for the override feature.
> >>
> >> If the need is testing then we have a tools/testing/nvdimm for that.
> >
> >
> >
> >> Of course it's an ABI extension, it allows userspace to discover DSM
> >> function numbers the kernel didn't know about at compile time.
> >
> >
> > A modification to a library or kernel that changes the results of a
> > function (or system call) doesn't necessarily break (or extend) an ABI.
> > An obvious example is that of a random number generator function.
> > A library/kernel is completely free to change the implementation
> > of the random number generator (and the values it returns)
> > without breaking the ABI provided all other rules of ABI preservation
> > are followed.
> >
> > Now lets look at problem at hand.  The pass thru mechanism has very
> > little semantic overhead.  Fill in the nd_cmd_pkg as described in ndctl.h,
> > call the ioctl w/ argument with ND_CMD_CALL, and the kernel will marshal
> > up the arguments, call the DSM and return the results.  The values
> > of nd_command could be any value and it is for the DSM to either accept
> > or reject the input argument.  I wrote this interface and this is how
> > I defined it.
> >
> > The user application is not changing irrespective of if the kernel applies
> > a mask to the passed in nd_command argument.  The data structures are not
> > changing at either source level or binary level. The calling convention is 
> > not
> > changing.  No object file changes are required.  Nothing related to ABI
> > preservation is impacted.  The only question is whether the application
> > of a mask to special case function 0 breaks/extends the ABI.
> >
> > It turns out that this point doesn't really matter as your position
> > is invalid either way.
> >
> > The argument for this not being an API breakage/extension:
> >
> > A DSM could either implement or not a function index for any value of N.
> > So, a correctly written application must take into account that for
> > any value of N, the DSM may return error or not.  Preserving an ABI
> > doesn't require the library/kernel preserve incorrect application
> > behavior.
> >
> > Now, assume that the special casing of function zero does constitute
> > a breakage/extension of the ABI:
> >
> > I'm not the one wishing to special case function 0, you 

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Jerry Hoemann]

On Wed, Jul 05, 2017 at 09:35:48AM -0700, Dan Williams wrote:
> On Wed, Jul 5, 2017 at 9:24 AM, Jerry Hoemann  wrote:
> > On Tue, Jul 04, 2017 at 01:37:43PM -0700, Dan Williams wrote:
> >> On Tue, Jul 4, 2017 at 1:08 PM, Jerry Hoemann  
> >> wrote:
> >> > On Sat, Jul 01, 2017 at 01:46:03PM -0700, Dan Williams wrote:
> >> >> On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  
> >> >> wrote:
> >> >> > On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
> >> >> >> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams 
> >> >> >>  wrote:
> >> >> >> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann 
> >> >> >> >  wrote:
> >> >> >> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
> >> >> >> >>
> >> >> >> >> ...
> >
> > ...
> >
> >> >> >> >>>
> >> >> >> >>> This drops function number 0 which userspace has no need to call.
> >> >> >> >>
> >> >> >> >> Actually I like to call function 0.  Its an excellent test when
> >> >> >> >> modifying the code path as its a no side effects function whose 
> >> >> >> >> output
> >> >> >> >> is known in advance and instantly recognizable.  I also use it 
> >> >> >> >> when
> >> >> >> >> testing new firmware.
> >> >> >> >>
> >> >> >> >> What is the downside to allowing it?  What bad things happen?
> >> >> >> >
> >> >> >> > It allows implementations to bypass the standardization process and
> >> >> >> > ship new root DSMs. It's always possible to patch the kernel 
> >> >> >> > locally
> >> >> >> > for development, so I see no reason to ship this capability 
> >> >> >> > globally.
> >> >> >
> >> >> > I don't understand this comment, but I think your next comment
> >> >> > essentially says to disregard this comment?
> >> >>
> >> >> Yes, sorry.
> >> >>
> >> >> >> Actually, just the discovery portion does not lead to this leak, but
> >> >> >> it's redundant when we have the 'dsm_mask' sysfs attribute.
> >> >> >
> >> >> > No.  The generation of the mask in sysfs is not done by
> >> >> > executing the code in acpi_nfit_ctl.  One of the reasons I call
> >> >> > function 0 to test changes I am making to the ioctl path itself.
> >> >> > The sysfs has nothing to do with that path and cannot be used
> >> >> > to serve this purpose.
> >> >> >
> >> >> > And since the content of sysfs has been edited it also can not be
> >> >> > used as a basic test of firmware.
> >> >> >
> >> >> > What is the downside to allowing the calling of function 0?
> >> >>
> >> >> It needlessly expands the kernel ABI. I would suggest, if you want to
> >> >
> >> > No.  It is not needless.  It is not an ABI extension.
> >> > Same goes for the override feature.
> >>
> >> If the need is testing then we have a tools/testing/nvdimm for that.
> >
> >
> >
> >> Of course it's an ABI extension, it allows userspace to discover DSM
> >> function numbers the kernel didn't know about at compile time.
> >
> >
> > A modification to a library or kernel that changes the results of a
> > function (or system call) doesn't necessarily break (or extend) an ABI.
> > An obvious example is that of a random number generator function.
> > A library/kernel is completely free to change the implementation
> > of the random number generator (and the values it returns)
> > without breaking the ABI provided all other rules of ABI preservation
> > are followed.
> >
> > Now lets look at problem at hand.  The pass thru mechanism has very
> > little semantic overhead.  Fill in the nd_cmd_pkg as described in ndctl.h,
> > call the ioctl w/ argument with ND_CMD_CALL, and the kernel will marshal
> > up the arguments, call the DSM and return the results.  The values
> > of nd_command could be any value and it is for the DSM to either accept
> > or reject the input argument.  I wrote this interface and this is how
> > I defined it.
> >
> > The user application is not changing irrespective of if the kernel applies
> > a mask to the passed in nd_command argument.  The data structures are not
> > changing at either source level or binary level. The calling convention is 
> > not
> > changing.  No object file changes are required.  Nothing related to ABI
> > preservation is impacted.  The only question is whether the application
> > of a mask to special case function 0 breaks/extends the ABI.
> >
> > It turns out that this point doesn't really matter as your position
> > is invalid either way.
> >
> > The argument for this not being an API breakage/extension:
> >
> > A DSM could either implement or not a function index for any value of N.
> > So, a correctly written application must take into account that for
> > any value of N, the DSM may return error or not.  Preserving an ABI
> > doesn't require the library/kernel preserve incorrect application
> > behavior.
> >
> > Now, assume that the special casing of function zero does constitute
> > a breakage/extension of the ABI:
> >
> > I'm not the one wishing to special case function 0, you are.
> > So, to this point I say, Dan please don't make needless extension to
> > the ABI. Its and extension and you've 

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Wed, Jul 5, 2017 at 9:24 AM, Jerry Hoemann  wrote:
> On Tue, Jul 04, 2017 at 01:37:43PM -0700, Dan Williams wrote:
>> On Tue, Jul 4, 2017 at 1:08 PM, Jerry Hoemann  wrote:
>> > On Sat, Jul 01, 2017 at 01:46:03PM -0700, Dan Williams wrote:
>> >> On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  
>> >> wrote:
>> >> > On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
>> >> >> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams 
>> >> >>  wrote:
>> >> >> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann 
>> >> >> >  wrote:
>> >> >> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>> >> >> >>
>> >> >> >> ...
>
> ...
>
>> >> >> >>>
>> >> >> >>> This drops function number 0 which userspace has no need to call.
>> >> >> >>
>> >> >> >> Actually I like to call function 0.  Its an excellent test when
>> >> >> >> modifying the code path as its a no side effects function whose 
>> >> >> >> output
>> >> >> >> is known in advance and instantly recognizable.  I also use it when
>> >> >> >> testing new firmware.
>> >> >> >>
>> >> >> >> What is the downside to allowing it?  What bad things happen?
>> >> >> >
>> >> >> > It allows implementations to bypass the standardization process and
>> >> >> > ship new root DSMs. It's always possible to patch the kernel locally
>> >> >> > for development, so I see no reason to ship this capability globally.
>> >> >
>> >> > I don't understand this comment, but I think your next comment
>> >> > essentially says to disregard this comment?
>> >>
>> >> Yes, sorry.
>> >>
>> >> >> Actually, just the discovery portion does not lead to this leak, but
>> >> >> it's redundant when we have the 'dsm_mask' sysfs attribute.
>> >> >
>> >> > No.  The generation of the mask in sysfs is not done by
>> >> > executing the code in acpi_nfit_ctl.  One of the reasons I call
>> >> > function 0 to test changes I am making to the ioctl path itself.
>> >> > The sysfs has nothing to do with that path and cannot be used
>> >> > to serve this purpose.
>> >> >
>> >> > And since the content of sysfs has been edited it also can not be
>> >> > used as a basic test of firmware.
>> >> >
>> >> > What is the downside to allowing the calling of function 0?
>> >>
>> >> It needlessly expands the kernel ABI. I would suggest, if you want to
>> >
>> > No.  It is not needless.  It is not an ABI extension.
>> > Same goes for the override feature.
>>
>> If the need is testing then we have a tools/testing/nvdimm for that.
>
>
>
>> Of course it's an ABI extension, it allows userspace to discover DSM
>> function numbers the kernel didn't know about at compile time.
>
>
> A modification to a library or kernel that changes the results of a
> function (or system call) doesn't necessarily break (or extend) an ABI.
> An obvious example is that of a random number generator function.
> A library/kernel is completely free to change the implementation
> of the random number generator (and the values it returns)
> without breaking the ABI provided all other rules of ABI preservation
> are followed.
>
> Now lets look at problem at hand.  The pass thru mechanism has very
> little semantic overhead.  Fill in the nd_cmd_pkg as described in ndctl.h,
> call the ioctl w/ argument with ND_CMD_CALL, and the kernel will marshal
> up the arguments, call the DSM and return the results.  The values
> of nd_command could be any value and it is for the DSM to either accept
> or reject the input argument.  I wrote this interface and this is how
> I defined it.
>
> The user application is not changing irrespective of if the kernel applies
> a mask to the passed in nd_command argument.  The data structures are not
> changing at either source level or binary level. The calling convention is not
> changing.  No object file changes are required.  Nothing related to ABI
> preservation is impacted.  The only question is whether the application
> of a mask to special case function 0 breaks/extends the ABI.
>
> It turns out that this point doesn't really matter as your position
> is invalid either way.
>
> The argument for this not being an API breakage/extension:
>
> A DSM could either implement or not a function index for any value of N.
> So, a correctly written application must take into account that for
> any value of N, the DSM may return error or not.  Preserving an ABI
> doesn't require the library/kernel preserve incorrect application
> behavior.
>
> Now, assume that the special casing of function zero does constitute
> a breakage/extension of the ABI:
>
> I'm not the one wishing to special case function 0, you are.
> So, to this point I say, Dan please don't make needless extension to
> the ABI. Its and extension and you've  provided no valid reason
> for making it.
>
> Your argument to disallow function zero is invalid.
>
> There is nothing harmful per se to allow function 0.  All DSMs that return
> non zero are required to 

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Wed, Jul 5, 2017 at 9:24 AM, Jerry Hoemann  wrote:
> On Tue, Jul 04, 2017 at 01:37:43PM -0700, Dan Williams wrote:
>> On Tue, Jul 4, 2017 at 1:08 PM, Jerry Hoemann  wrote:
>> > On Sat, Jul 01, 2017 at 01:46:03PM -0700, Dan Williams wrote:
>> >> On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  
>> >> wrote:
>> >> > On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
>> >> >> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams 
>> >> >>  wrote:
>> >> >> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann 
>> >> >> >  wrote:
>> >> >> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>> >> >> >>
>> >> >> >> ...
>
> ...
>
>> >> >> >>>
>> >> >> >>> This drops function number 0 which userspace has no need to call.
>> >> >> >>
>> >> >> >> Actually I like to call function 0.  Its an excellent test when
>> >> >> >> modifying the code path as its a no side effects function whose 
>> >> >> >> output
>> >> >> >> is known in advance and instantly recognizable.  I also use it when
>> >> >> >> testing new firmware.
>> >> >> >>
>> >> >> >> What is the downside to allowing it?  What bad things happen?
>> >> >> >
>> >> >> > It allows implementations to bypass the standardization process and
>> >> >> > ship new root DSMs. It's always possible to patch the kernel locally
>> >> >> > for development, so I see no reason to ship this capability globally.
>> >> >
>> >> > I don't understand this comment, but I think your next comment
>> >> > essentially says to disregard this comment?
>> >>
>> >> Yes, sorry.
>> >>
>> >> >> Actually, just the discovery portion does not lead to this leak, but
>> >> >> it's redundant when we have the 'dsm_mask' sysfs attribute.
>> >> >
>> >> > No.  The generation of the mask in sysfs is not done by
>> >> > executing the code in acpi_nfit_ctl.  One of the reasons I call
>> >> > function 0 to test changes I am making to the ioctl path itself.
>> >> > The sysfs has nothing to do with that path and cannot be used
>> >> > to serve this purpose.
>> >> >
>> >> > And since the content of sysfs has been edited it also can not be
>> >> > used as a basic test of firmware.
>> >> >
>> >> > What is the downside to allowing the calling of function 0?
>> >>
>> >> It needlessly expands the kernel ABI. I would suggest, if you want to
>> >
>> > No.  It is not needless.  It is not an ABI extension.
>> > Same goes for the override feature.
>>
>> If the need is testing then we have a tools/testing/nvdimm for that.
>
>
>
>> Of course it's an ABI extension, it allows userspace to discover DSM
>> function numbers the kernel didn't know about at compile time.
>
>
> A modification to a library or kernel that changes the results of a
> function (or system call) doesn't necessarily break (or extend) an ABI.
> An obvious example is that of a random number generator function.
> A library/kernel is completely free to change the implementation
> of the random number generator (and the values it returns)
> without breaking the ABI provided all other rules of ABI preservation
> are followed.
>
> Now lets look at problem at hand.  The pass thru mechanism has very
> little semantic overhead.  Fill in the nd_cmd_pkg as described in ndctl.h,
> call the ioctl w/ argument with ND_CMD_CALL, and the kernel will marshal
> up the arguments, call the DSM and return the results.  The values
> of nd_command could be any value and it is for the DSM to either accept
> or reject the input argument.  I wrote this interface and this is how
> I defined it.
>
> The user application is not changing irrespective of if the kernel applies
> a mask to the passed in nd_command argument.  The data structures are not
> changing at either source level or binary level. The calling convention is not
> changing.  No object file changes are required.  Nothing related to ABI
> preservation is impacted.  The only question is whether the application
> of a mask to special case function 0 breaks/extends the ABI.
>
> It turns out that this point doesn't really matter as your position
> is invalid either way.
>
> The argument for this not being an API breakage/extension:
>
> A DSM could either implement or not a function index for any value of N.
> So, a correctly written application must take into account that for
> any value of N, the DSM may return error or not.  Preserving an ABI
> doesn't require the library/kernel preserve incorrect application
> behavior.
>
> Now, assume that the special casing of function zero does constitute
> a breakage/extension of the ABI:
>
> I'm not the one wishing to special case function 0, you are.
> So, to this point I say, Dan please don't make needless extension to
> the ABI. Its and extension and you've  provided no valid reason
> for making it.
>
> Your argument to disallow function zero is invalid.
>
> There is nothing harmful per se to allow function 0.  All DSMs that return
> non zero are required to have it. By excluding it, you actually create the
> impression that the underlying DSM is violating the DSM 

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Jerry Hoemann]

On Tue, Jul 04, 2017 at 01:37:43PM -0700, Dan Williams wrote:
> On Tue, Jul 4, 2017 at 1:08 PM, Jerry Hoemann  wrote:
> > On Sat, Jul 01, 2017 at 01:46:03PM -0700, Dan Williams wrote:
> >> On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  
> >> wrote:
> >> > On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
> >> >> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  
> >> >> wrote:
> >> >> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann 
> >> >> >  wrote:
> >> >> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
> >> >> >>
> >> >> >> ...

...

> >> >> >>>
> >> >> >>> This drops function number 0 which userspace has no need to call.
> >> >> >>
> >> >> >> Actually I like to call function 0.  Its an excellent test when
> >> >> >> modifying the code path as its a no side effects function whose 
> >> >> >> output
> >> >> >> is known in advance and instantly recognizable.  I also use it when
> >> >> >> testing new firmware.
> >> >> >>
> >> >> >> What is the downside to allowing it?  What bad things happen?
> >> >> >
> >> >> > It allows implementations to bypass the standardization process and
> >> >> > ship new root DSMs. It's always possible to patch the kernel locally
> >> >> > for development, so I see no reason to ship this capability globally.
> >> >
> >> > I don't understand this comment, but I think your next comment
> >> > essentially says to disregard this comment?
> >>
> >> Yes, sorry.
> >>
> >> >> Actually, just the discovery portion does not lead to this leak, but
> >> >> it's redundant when we have the 'dsm_mask' sysfs attribute.
> >> >
> >> > No.  The generation of the mask in sysfs is not done by
> >> > executing the code in acpi_nfit_ctl.  One of the reasons I call
> >> > function 0 to test changes I am making to the ioctl path itself.
> >> > The sysfs has nothing to do with that path and cannot be used
> >> > to serve this purpose.
> >> >
> >> > And since the content of sysfs has been edited it also can not be
> >> > used as a basic test of firmware.
> >> >
> >> > What is the downside to allowing the calling of function 0?
> >>
> >> It needlessly expands the kernel ABI. I would suggest, if you want to
> >
> > No.  It is not needless.  It is not an ABI extension.
> > Same goes for the override feature.
> 
> If the need is testing then we have a tools/testing/nvdimm for that.



> Of course it's an ABI extension, it allows userspace to discover DSM
> function numbers the kernel didn't know about at compile time.


A modification to a library or kernel that changes the results of a
function (or system call) doesn't necessarily break (or extend) an ABI.
An obvious example is that of a random number generator function.
A library/kernel is completely free to change the implementation
of the random number generator (and the values it returns)
without breaking the ABI provided all other rules of ABI preservation
are followed.

Now lets look at problem at hand.  The pass thru mechanism has very
little semantic overhead.  Fill in the nd_cmd_pkg as described in ndctl.h,
call the ioctl w/ argument with ND_CMD_CALL, and the kernel will marshal
up the arguments, call the DSM and return the results.  The values
of nd_command could be any value and it is for the DSM to either accept
or reject the input argument.  I wrote this interface and this is how
I defined it.

The user application is not changing irrespective of if the kernel applies
a mask to the passed in nd_command argument.  The data structures are not
changing at either source level or binary level. The calling convention is not
changing.  No object file changes are required.  Nothing related to ABI
preservation is impacted.  The only question is whether the application
of a mask to special case function 0 breaks/extends the ABI.

It turns out that this point doesn't really matter as your position
is invalid either way.

The argument for this not being an API breakage/extension:

A DSM could either implement or not a function index for any value of N.
So, a correctly written application must take into account that for
any value of N, the DSM may return error or not.  Preserving an ABI
doesn't require the library/kernel preserve incorrect application
behavior.

Now, assume that the special casing of function zero does constitute
a breakage/extension of the ABI:

I'm not the one wishing to special case function 0, you are.
So, to this point I say, Dan please don't make needless extension to
the ABI. Its and extension and you've  provided no valid reason
for making it.

Your argument to disallow function zero is invalid.

There is nothing harmful per se to allow function 0.  All DSMs that return
non zero are required to have it. By excluding it, you actually create the
impression that the underlying DSM is violating the DSM specification.


-- 

-
Jerry Hoemann  

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Jerry Hoemann]

On Tue, Jul 04, 2017 at 01:37:43PM -0700, Dan Williams wrote:
> On Tue, Jul 4, 2017 at 1:08 PM, Jerry Hoemann  wrote:
> > On Sat, Jul 01, 2017 at 01:46:03PM -0700, Dan Williams wrote:
> >> On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  
> >> wrote:
> >> > On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
> >> >> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  
> >> >> wrote:
> >> >> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann 
> >> >> >  wrote:
> >> >> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
> >> >> >>
> >> >> >> ...

...

> >> >> >>>
> >> >> >>> This drops function number 0 which userspace has no need to call.
> >> >> >>
> >> >> >> Actually I like to call function 0.  Its an excellent test when
> >> >> >> modifying the code path as its a no side effects function whose 
> >> >> >> output
> >> >> >> is known in advance and instantly recognizable.  I also use it when
> >> >> >> testing new firmware.
> >> >> >>
> >> >> >> What is the downside to allowing it?  What bad things happen?
> >> >> >
> >> >> > It allows implementations to bypass the standardization process and
> >> >> > ship new root DSMs. It's always possible to patch the kernel locally
> >> >> > for development, so I see no reason to ship this capability globally.
> >> >
> >> > I don't understand this comment, but I think your next comment
> >> > essentially says to disregard this comment?
> >>
> >> Yes, sorry.
> >>
> >> >> Actually, just the discovery portion does not lead to this leak, but
> >> >> it's redundant when we have the 'dsm_mask' sysfs attribute.
> >> >
> >> > No.  The generation of the mask in sysfs is not done by
> >> > executing the code in acpi_nfit_ctl.  One of the reasons I call
> >> > function 0 to test changes I am making to the ioctl path itself.
> >> > The sysfs has nothing to do with that path and cannot be used
> >> > to serve this purpose.
> >> >
> >> > And since the content of sysfs has been edited it also can not be
> >> > used as a basic test of firmware.
> >> >
> >> > What is the downside to allowing the calling of function 0?
> >>
> >> It needlessly expands the kernel ABI. I would suggest, if you want to
> >
> > No.  It is not needless.  It is not an ABI extension.
> > Same goes for the override feature.
> 
> If the need is testing then we have a tools/testing/nvdimm for that.



> Of course it's an ABI extension, it allows userspace to discover DSM
> function numbers the kernel didn't know about at compile time.


A modification to a library or kernel that changes the results of a
function (or system call) doesn't necessarily break (or extend) an ABI.
An obvious example is that of a random number generator function.
A library/kernel is completely free to change the implementation
of the random number generator (and the values it returns)
without breaking the ABI provided all other rules of ABI preservation
are followed.

Now lets look at problem at hand.  The pass thru mechanism has very
little semantic overhead.  Fill in the nd_cmd_pkg as described in ndctl.h,
call the ioctl w/ argument with ND_CMD_CALL, and the kernel will marshal
up the arguments, call the DSM and return the results.  The values
of nd_command could be any value and it is for the DSM to either accept
or reject the input argument.  I wrote this interface and this is how
I defined it.

The user application is not changing irrespective of if the kernel applies
a mask to the passed in nd_command argument.  The data structures are not
changing at either source level or binary level. The calling convention is not
changing.  No object file changes are required.  Nothing related to ABI
preservation is impacted.  The only question is whether the application
of a mask to special case function 0 breaks/extends the ABI.

It turns out that this point doesn't really matter as your position
is invalid either way.

The argument for this not being an API breakage/extension:

A DSM could either implement or not a function index for any value of N.
So, a correctly written application must take into account that for
any value of N, the DSM may return error or not.  Preserving an ABI
doesn't require the library/kernel preserve incorrect application
behavior.

Now, assume that the special casing of function zero does constitute
a breakage/extension of the ABI:

I'm not the one wishing to special case function 0, you are.
So, to this point I say, Dan please don't make needless extension to
the ABI. Its and extension and you've  provided no valid reason
for making it.

Your argument to disallow function zero is invalid.

There is nothing harmful per se to allow function 0.  All DSMs that return
non zero are required to have it. By excluding it, you actually create the
impression that the underlying DSM is violating the DSM specification.


-- 

-
Jerry Hoemann  Software Engineer   Hewlett Packard Enterprise

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Linda Knippers]

On 07/04/2017 04:37 PM, Dan Williams wrote:
> On Tue, Jul 4, 2017 at 1:08 PM, Jerry Hoemann  wrote:
>> On Sat, Jul 01, 2017 at 01:46:03PM -0700, Dan Williams wrote:
>>> On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  wrote:
 On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  
> wrote:
>> On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  
>> wrote:
>>> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>>>
>>> ...
>>>
 On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  
 wrote:
> +   if (cmd == ND_CMD_CALL)
> +   dsm_mask = nd_desc->bus_dsm_mask;
> desc = nd_cmd_bus_desc(cmd);
> uuid = to_nfit_uuid(NFIT_DEV_BUS);
> handle = adev->handle;
> @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
> acpi_nfit_desc *acpi_desc)
> struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
> const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
> struct acpi_device *adev;
> +   unsigned long dsm_mask;
> int i;
>
> nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
> acpi_nfit_desc *acpi_desc)
> if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> set_bit(i, _desc->cmd_mask);
> set_bit(ND_CMD_CALL, _desc->cmd_mask);
> +
> +   dsm_mask = 0x3bf;

 I went ahead and fixed this up to use dsm_mask defined like this:

 +   dsm_mask =
 +   (1 << ND_CMD_ARS_CAP) |
 +   (1 << ND_CMD_ARS_START) |
 +   (1 << ND_CMD_ARS_STATUS) |
 +   (1 << ND_CMD_CLEAR_ERROR) |
 +   (1 << NFIT_CMD_TRANSLATE_SPA) |
 +   (1 << NFIT_CMD_ARS_INJECT_SET) |
 +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
 +   (1 << NFIT_CMD_ARS_INJECT_GET);

 This drops function number 0 which userspace has no need to call.
>>>
>>> Actually I like to call function 0.  Its an excellent test when
>>> modifying the code path as its a no side effects function whose output
>>> is known in advance and instantly recognizable.  I also use it when
>>> testing new firmware.
>>>
>>> What is the downside to allowing it?  What bad things happen?
>>
>> It allows implementations to bypass the standardization process and
>> ship new root DSMs. It's always possible to patch the kernel locally
>> for development, so I see no reason to ship this capability globally.

 I don't understand this comment, but I think your next comment
 essentially says to disregard this comment?
>>>
>>> Yes, sorry.
>>>
> Actually, just the discovery portion does not lead to this leak, but
> it's redundant when we have the 'dsm_mask' sysfs attribute.

 No.  The generation of the mask in sysfs is not done by
 executing the code in acpi_nfit_ctl.  One of the reasons I call
 function 0 to test changes I am making to the ioctl path itself.
 The sysfs has nothing to do with that path and cannot be used
 to serve this purpose.

 And since the content of sysfs has been edited it also can not be
 used as a basic test of firmware.

 What is the downside to allowing the calling of function 0?
>>>
>>> It needlessly expands the kernel ABI. I would suggest, if you want to
>>
>> No.  It is not needless.  It is not an ABI extension.
>> Same goes for the override feature.

I have never understood why allowing function 0 is considered harmful.
It is a standard function defined by ACPI in general and specifically
for NVDIMM Rood Device _DSMs.  It is also defined for each vendor-specific
DSM family.  It is not an ABI extension.  It is a standard.

> If the need is testing then we have a tools/testing/nvdimm for that.
> Of course it's an ABI extension, it allows userspace to discover DSM
> function numbers the kernel didn't know about at compile time.

It also allows user space to determine which DSMs are actually supported
by the platform, which may be a subset of the defined set, in a standard
way.  Exposing information only in /sys just makes it harder for people
writing software (tools, tests, whatever) that need to support more than
just Linux.

>> I hope that ACPI doesn't extend the specification in the future because
>> we'll just have to redo these patches yet again.
> 
> Hopefully this is the last ACPI spec version where we add new DSMs to
> the root device. 

I wouldn't bet on it.

> 

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Linda Knippers]

On 07/04/2017 04:37 PM, Dan Williams wrote:
> On Tue, Jul 4, 2017 at 1:08 PM, Jerry Hoemann  wrote:
>> On Sat, Jul 01, 2017 at 01:46:03PM -0700, Dan Williams wrote:
>>> On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  wrote:
 On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  
> wrote:
>> On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  
>> wrote:
>>> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>>>
>>> ...
>>>
 On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  
 wrote:
> +   if (cmd == ND_CMD_CALL)
> +   dsm_mask = nd_desc->bus_dsm_mask;
> desc = nd_cmd_bus_desc(cmd);
> uuid = to_nfit_uuid(NFIT_DEV_BUS);
> handle = adev->handle;
> @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
> acpi_nfit_desc *acpi_desc)
> struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
> const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
> struct acpi_device *adev;
> +   unsigned long dsm_mask;
> int i;
>
> nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
> acpi_nfit_desc *acpi_desc)
> if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> set_bit(i, _desc->cmd_mask);
> set_bit(ND_CMD_CALL, _desc->cmd_mask);
> +
> +   dsm_mask = 0x3bf;

 I went ahead and fixed this up to use dsm_mask defined like this:

 +   dsm_mask =
 +   (1 << ND_CMD_ARS_CAP) |
 +   (1 << ND_CMD_ARS_START) |
 +   (1 << ND_CMD_ARS_STATUS) |
 +   (1 << ND_CMD_CLEAR_ERROR) |
 +   (1 << NFIT_CMD_TRANSLATE_SPA) |
 +   (1 << NFIT_CMD_ARS_INJECT_SET) |
 +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
 +   (1 << NFIT_CMD_ARS_INJECT_GET);

 This drops function number 0 which userspace has no need to call.
>>>
>>> Actually I like to call function 0.  Its an excellent test when
>>> modifying the code path as its a no side effects function whose output
>>> is known in advance and instantly recognizable.  I also use it when
>>> testing new firmware.
>>>
>>> What is the downside to allowing it?  What bad things happen?
>>
>> It allows implementations to bypass the standardization process and
>> ship new root DSMs. It's always possible to patch the kernel locally
>> for development, so I see no reason to ship this capability globally.

 I don't understand this comment, but I think your next comment
 essentially says to disregard this comment?
>>>
>>> Yes, sorry.
>>>
> Actually, just the discovery portion does not lead to this leak, but
> it's redundant when we have the 'dsm_mask' sysfs attribute.

 No.  The generation of the mask in sysfs is not done by
 executing the code in acpi_nfit_ctl.  One of the reasons I call
 function 0 to test changes I am making to the ioctl path itself.
 The sysfs has nothing to do with that path and cannot be used
 to serve this purpose.

 And since the content of sysfs has been edited it also can not be
 used as a basic test of firmware.

 What is the downside to allowing the calling of function 0?
>>>
>>> It needlessly expands the kernel ABI. I would suggest, if you want to
>>
>> No.  It is not needless.  It is not an ABI extension.
>> Same goes for the override feature.

I have never understood why allowing function 0 is considered harmful.
It is a standard function defined by ACPI in general and specifically
for NVDIMM Rood Device _DSMs.  It is also defined for each vendor-specific
DSM family.  It is not an ABI extension.  It is a standard.

> If the need is testing then we have a tools/testing/nvdimm for that.
> Of course it's an ABI extension, it allows userspace to discover DSM
> function numbers the kernel didn't know about at compile time.

It also allows user space to determine which DSMs are actually supported
by the platform, which may be a subset of the defined set, in a standard
way.  Exposing information only in /sys just makes it harder for people
writing software (tools, tests, whatever) that need to support more than
just Linux.

>> I hope that ACPI doesn't extend the specification in the future because
>> we'll just have to redo these patches yet again.
> 
> Hopefully this is the last ACPI spec version where we add new DSMs to
> the root device. 

I wouldn't bet on it.

> All future methods should be named methods like what
> the specification started doing for NVIDMM leaf devices with 

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Tue, Jul 4, 2017 at 1:08 PM, Jerry Hoemann  wrote:
> On Sat, Jul 01, 2017 at 01:46:03PM -0700, Dan Williams wrote:
>> On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  wrote:
>> > On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
>> >> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  
>> >> wrote:
>> >> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  
>> >> > wrote:
>> >> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>> >> >>
>> >> >> ...
>> >> >>
>> >> >>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann 
>> >> >>>  wrote:
>> >> >>> > +   if (cmd == ND_CMD_CALL)
>> >> >>> > +   dsm_mask = nd_desc->bus_dsm_mask;
>> >> >>> > desc = nd_cmd_bus_desc(cmd);
>> >> >>> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >> >>> > handle = adev->handle;
>> >> >>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
>> >> >>> > acpi_nfit_desc *acpi_desc)
>> >> >>> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
>> >> >>> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >> >>> > struct acpi_device *adev;
>> >> >>> > +   unsigned long dsm_mask;
>> >> >>> > int i;
>> >> >>> >
>> >> >>> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
>> >> >>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
>> >> >>> > acpi_nfit_desc *acpi_desc)
>> >> >>> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << 
>> >> >>> > i))
>> >> >>> > set_bit(i, _desc->cmd_mask);
>> >> >>> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
>> >> >>> > +
>> >> >>> > +   dsm_mask = 0x3bf;
>> >> >>>
>> >> >>> I went ahead and fixed this up to use dsm_mask defined like this:
>> >> >>>
>> >> >>> +   dsm_mask =
>> >> >>> +   (1 << ND_CMD_ARS_CAP) |
>> >> >>> +   (1 << ND_CMD_ARS_START) |
>> >> >>> +   (1 << ND_CMD_ARS_STATUS) |
>> >> >>> +   (1 << ND_CMD_CLEAR_ERROR) |
>> >> >>> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
>> >> >>> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
>> >> >>> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>> >> >>> +   (1 << NFIT_CMD_ARS_INJECT_GET);
>> >> >>>
>> >> >>> This drops function number 0 which userspace has no need to call.
>> >> >>
>> >> >> Actually I like to call function 0.  Its an excellent test when
>> >> >> modifying the code path as its a no side effects function whose output
>> >> >> is known in advance and instantly recognizable.  I also use it when
>> >> >> testing new firmware.
>> >> >>
>> >> >> What is the downside to allowing it?  What bad things happen?
>> >> >
>> >> > It allows implementations to bypass the standardization process and
>> >> > ship new root DSMs. It's always possible to patch the kernel locally
>> >> > for development, so I see no reason to ship this capability globally.
>> >
>> > I don't understand this comment, but I think your next comment
>> > essentially says to disregard this comment?
>>
>> Yes, sorry.
>>
>> >> Actually, just the discovery portion does not lead to this leak, but
>> >> it's redundant when we have the 'dsm_mask' sysfs attribute.
>> >
>> > No.  The generation of the mask in sysfs is not done by
>> > executing the code in acpi_nfit_ctl.  One of the reasons I call
>> > function 0 to test changes I am making to the ioctl path itself.
>> > The sysfs has nothing to do with that path and cannot be used
>> > to serve this purpose.
>> >
>> > And since the content of sysfs has been edited it also can not be
>> > used as a basic test of firmware.
>> >
>> > What is the downside to allowing the calling of function 0?
>>
>> It needlessly expands the kernel ABI. I would suggest, if you want to
>
> No.  It is not needless.  It is not an ABI extension.
> Same goes for the override feature.

If the need is testing then we have a tools/testing/nvdimm for that.
Of course it's an ABI extension, it allows userspace to discover DSM
function numbers the kernel didn't know about at compile time.

> I hope that ACPI doesn't extend the specification in the future because
> we'll just have to redo these patches yet again.

Hopefully this is the last ACPI spec version where we add new DSMs to
the root device. All future methods should be named methods like what
the specification started doing for NVIDMM leaf devices with _LSI,
_LSR, and _LSW.

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Tue, Jul 4, 2017 at 1:08 PM, Jerry Hoemann  wrote:
> On Sat, Jul 01, 2017 at 01:46:03PM -0700, Dan Williams wrote:
>> On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  wrote:
>> > On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
>> >> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  
>> >> wrote:
>> >> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  
>> >> > wrote:
>> >> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>> >> >>
>> >> >> ...
>> >> >>
>> >> >>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann 
>> >> >>>  wrote:
>> >> >>> > +   if (cmd == ND_CMD_CALL)
>> >> >>> > +   dsm_mask = nd_desc->bus_dsm_mask;
>> >> >>> > desc = nd_cmd_bus_desc(cmd);
>> >> >>> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >> >>> > handle = adev->handle;
>> >> >>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
>> >> >>> > acpi_nfit_desc *acpi_desc)
>> >> >>> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
>> >> >>> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >> >>> > struct acpi_device *adev;
>> >> >>> > +   unsigned long dsm_mask;
>> >> >>> > int i;
>> >> >>> >
>> >> >>> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
>> >> >>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
>> >> >>> > acpi_nfit_desc *acpi_desc)
>> >> >>> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << 
>> >> >>> > i))
>> >> >>> > set_bit(i, _desc->cmd_mask);
>> >> >>> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
>> >> >>> > +
>> >> >>> > +   dsm_mask = 0x3bf;
>> >> >>>
>> >> >>> I went ahead and fixed this up to use dsm_mask defined like this:
>> >> >>>
>> >> >>> +   dsm_mask =
>> >> >>> +   (1 << ND_CMD_ARS_CAP) |
>> >> >>> +   (1 << ND_CMD_ARS_START) |
>> >> >>> +   (1 << ND_CMD_ARS_STATUS) |
>> >> >>> +   (1 << ND_CMD_CLEAR_ERROR) |
>> >> >>> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
>> >> >>> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
>> >> >>> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>> >> >>> +   (1 << NFIT_CMD_ARS_INJECT_GET);
>> >> >>>
>> >> >>> This drops function number 0 which userspace has no need to call.
>> >> >>
>> >> >> Actually I like to call function 0.  Its an excellent test when
>> >> >> modifying the code path as its a no side effects function whose output
>> >> >> is known in advance and instantly recognizable.  I also use it when
>> >> >> testing new firmware.
>> >> >>
>> >> >> What is the downside to allowing it?  What bad things happen?
>> >> >
>> >> > It allows implementations to bypass the standardization process and
>> >> > ship new root DSMs. It's always possible to patch the kernel locally
>> >> > for development, so I see no reason to ship this capability globally.
>> >
>> > I don't understand this comment, but I think your next comment
>> > essentially says to disregard this comment?
>>
>> Yes, sorry.
>>
>> >> Actually, just the discovery portion does not lead to this leak, but
>> >> it's redundant when we have the 'dsm_mask' sysfs attribute.
>> >
>> > No.  The generation of the mask in sysfs is not done by
>> > executing the code in acpi_nfit_ctl.  One of the reasons I call
>> > function 0 to test changes I am making to the ioctl path itself.
>> > The sysfs has nothing to do with that path and cannot be used
>> > to serve this purpose.
>> >
>> > And since the content of sysfs has been edited it also can not be
>> > used as a basic test of firmware.
>> >
>> > What is the downside to allowing the calling of function 0?
>>
>> It needlessly expands the kernel ABI. I would suggest, if you want to
>
> No.  It is not needless.  It is not an ABI extension.
> Same goes for the override feature.

If the need is testing then we have a tools/testing/nvdimm for that.
Of course it's an ABI extension, it allows userspace to discover DSM
function numbers the kernel didn't know about at compile time.

> I hope that ACPI doesn't extend the specification in the future because
> we'll just have to redo these patches yet again.

Hopefully this is the last ACPI spec version where we add new DSMs to
the root device. All future methods should be named methods like what
the specification started doing for NVIDMM leaf devices with _LSI,
_LSR, and _LSW.

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Jerry Hoemann]

On Sat, Jul 01, 2017 at 01:46:03PM -0700, Dan Williams wrote:
> On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  wrote:
> > On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
> >> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  
> >> wrote:
> >> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  
> >> > wrote:
> >> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
> >> >>
> >> >> ...
> >> >>
> >> >>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  
> >> >>> wrote:
> >> >>> > +   if (cmd == ND_CMD_CALL)
> >> >>> > +   dsm_mask = nd_desc->bus_dsm_mask;
> >> >>> > desc = nd_cmd_bus_desc(cmd);
> >> >>> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >> >>> > handle = adev->handle;
> >> >>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
> >> >>> > acpi_nfit_desc *acpi_desc)
> >> >>> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
> >> >>> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >> >>> > struct acpi_device *adev;
> >> >>> > +   unsigned long dsm_mask;
> >> >>> > int i;
> >> >>> >
> >> >>> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> >> >>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
> >> >>> > acpi_nfit_desc *acpi_desc)
> >> >>> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> >> >>> > set_bit(i, _desc->cmd_mask);
> >> >>> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
> >> >>> > +
> >> >>> > +   dsm_mask = 0x3bf;
> >> >>>
> >> >>> I went ahead and fixed this up to use dsm_mask defined like this:
> >> >>>
> >> >>> +   dsm_mask =
> >> >>> +   (1 << ND_CMD_ARS_CAP) |
> >> >>> +   (1 << ND_CMD_ARS_START) |
> >> >>> +   (1 << ND_CMD_ARS_STATUS) |
> >> >>> +   (1 << ND_CMD_CLEAR_ERROR) |
> >> >>> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
> >> >>> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
> >> >>> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
> >> >>> +   (1 << NFIT_CMD_ARS_INJECT_GET);
> >> >>>
> >> >>> This drops function number 0 which userspace has no need to call.
> >> >>
> >> >> Actually I like to call function 0.  Its an excellent test when
> >> >> modifying the code path as its a no side effects function whose output
> >> >> is known in advance and instantly recognizable.  I also use it when
> >> >> testing new firmware.
> >> >>
> >> >> What is the downside to allowing it?  What bad things happen?
> >> >
> >> > It allows implementations to bypass the standardization process and
> >> > ship new root DSMs. It's always possible to patch the kernel locally
> >> > for development, so I see no reason to ship this capability globally.
> >
> > I don't understand this comment, but I think your next comment
> > essentially says to disregard this comment?
> 
> Yes, sorry.
> 
> >> Actually, just the discovery portion does not lead to this leak, but
> >> it's redundant when we have the 'dsm_mask' sysfs attribute.
> >
> > No.  The generation of the mask in sysfs is not done by
> > executing the code in acpi_nfit_ctl.  One of the reasons I call
> > function 0 to test changes I am making to the ioctl path itself.
> > The sysfs has nothing to do with that path and cannot be used
> > to serve this purpose.
> >
> > And since the content of sysfs has been edited it also can not be
> > used as a basic test of firmware.
> >
> > What is the downside to allowing the calling of function 0?
> 
> It needlessly expands the kernel ABI. I would suggest, if you want to

No.  It is not needless.  It is not an ABI extension.
Same goes for the override feature.

I hope that ACPI doesn't extend the specification in the future because
we'll just have to redo these patches yet again.



-- 

-
Jerry Hoemann  Software Engineer   Hewlett Packard Enterprise
-

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Jerry Hoemann]

On Sat, Jul 01, 2017 at 01:46:03PM -0700, Dan Williams wrote:
> On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  wrote:
> > On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
> >> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  
> >> wrote:
> >> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  
> >> > wrote:
> >> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
> >> >>
> >> >> ...
> >> >>
> >> >>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  
> >> >>> wrote:
> >> >>> > +   if (cmd == ND_CMD_CALL)
> >> >>> > +   dsm_mask = nd_desc->bus_dsm_mask;
> >> >>> > desc = nd_cmd_bus_desc(cmd);
> >> >>> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >> >>> > handle = adev->handle;
> >> >>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
> >> >>> > acpi_nfit_desc *acpi_desc)
> >> >>> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
> >> >>> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >> >>> > struct acpi_device *adev;
> >> >>> > +   unsigned long dsm_mask;
> >> >>> > int i;
> >> >>> >
> >> >>> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> >> >>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
> >> >>> > acpi_nfit_desc *acpi_desc)
> >> >>> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> >> >>> > set_bit(i, _desc->cmd_mask);
> >> >>> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
> >> >>> > +
> >> >>> > +   dsm_mask = 0x3bf;
> >> >>>
> >> >>> I went ahead and fixed this up to use dsm_mask defined like this:
> >> >>>
> >> >>> +   dsm_mask =
> >> >>> +   (1 << ND_CMD_ARS_CAP) |
> >> >>> +   (1 << ND_CMD_ARS_START) |
> >> >>> +   (1 << ND_CMD_ARS_STATUS) |
> >> >>> +   (1 << ND_CMD_CLEAR_ERROR) |
> >> >>> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
> >> >>> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
> >> >>> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
> >> >>> +   (1 << NFIT_CMD_ARS_INJECT_GET);
> >> >>>
> >> >>> This drops function number 0 which userspace has no need to call.
> >> >>
> >> >> Actually I like to call function 0.  Its an excellent test when
> >> >> modifying the code path as its a no side effects function whose output
> >> >> is known in advance and instantly recognizable.  I also use it when
> >> >> testing new firmware.
> >> >>
> >> >> What is the downside to allowing it?  What bad things happen?
> >> >
> >> > It allows implementations to bypass the standardization process and
> >> > ship new root DSMs. It's always possible to patch the kernel locally
> >> > for development, so I see no reason to ship this capability globally.
> >
> > I don't understand this comment, but I think your next comment
> > essentially says to disregard this comment?
> 
> Yes, sorry.
> 
> >> Actually, just the discovery portion does not lead to this leak, but
> >> it's redundant when we have the 'dsm_mask' sysfs attribute.
> >
> > No.  The generation of the mask in sysfs is not done by
> > executing the code in acpi_nfit_ctl.  One of the reasons I call
> > function 0 to test changes I am making to the ioctl path itself.
> > The sysfs has nothing to do with that path and cannot be used
> > to serve this purpose.
> >
> > And since the content of sysfs has been edited it also can not be
> > used as a basic test of firmware.
> >
> > What is the downside to allowing the calling of function 0?
> 
> It needlessly expands the kernel ABI. I would suggest, if you want to

No.  It is not needless.  It is not an ABI extension.
Same goes for the override feature.

I hope that ACPI doesn't extend the specification in the future because
we'll just have to redo these patches yet again.



-- 

-
Jerry Hoemann  Software Engineer   Hewlett Packard Enterprise
-

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  wrote:
> On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
>> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  
>> wrote:
>> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  
>> > wrote:
>> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>> >>
>> >> ...
>> >>
>> >>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  
>> >>> wrote:
>> >>> > +   if (cmd == ND_CMD_CALL)
>> >>> > +   dsm_mask = nd_desc->bus_dsm_mask;
>> >>> > desc = nd_cmd_bus_desc(cmd);
>> >>> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >>> > handle = adev->handle;
>> >>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
>> >>> > acpi_nfit_desc *acpi_desc)
>> >>> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
>> >>> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >>> > struct acpi_device *adev;
>> >>> > +   unsigned long dsm_mask;
>> >>> > int i;
>> >>> >
>> >>> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
>> >>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
>> >>> > acpi_nfit_desc *acpi_desc)
>> >>> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>> >>> > set_bit(i, _desc->cmd_mask);
>> >>> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
>> >>> > +
>> >>> > +   dsm_mask = 0x3bf;
>> >>>
>> >>> I went ahead and fixed this up to use dsm_mask defined like this:
>> >>>
>> >>> +   dsm_mask =
>> >>> +   (1 << ND_CMD_ARS_CAP) |
>> >>> +   (1 << ND_CMD_ARS_START) |
>> >>> +   (1 << ND_CMD_ARS_STATUS) |
>> >>> +   (1 << ND_CMD_CLEAR_ERROR) |
>> >>> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
>> >>> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
>> >>> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>> >>> +   (1 << NFIT_CMD_ARS_INJECT_GET);
>> >>>
>> >>> This drops function number 0 which userspace has no need to call.
>> >>
>> >> Actually I like to call function 0.  Its an excellent test when
>> >> modifying the code path as its a no side effects function whose output
>> >> is known in advance and instantly recognizable.  I also use it when
>> >> testing new firmware.
>> >>
>> >> What is the downside to allowing it?  What bad things happen?
>> >
>> > It allows implementations to bypass the standardization process and
>> > ship new root DSMs. It's always possible to patch the kernel locally
>> > for development, so I see no reason to ship this capability globally.
>
> I don't understand this comment, but I think your next comment
> essentially says to disregard this comment?

Yes, sorry.

>> Actually, just the discovery portion does not lead to this leak, but
>> it's redundant when we have the 'dsm_mask' sysfs attribute.
>
> No.  The generation of the mask in sysfs is not done by
> executing the code in acpi_nfit_ctl.  One of the reasons I call
> function 0 to test changes I am making to the ioctl path itself.
> The sysfs has nothing to do with that path and cannot be used
> to serve this purpose.
>
> And since the content of sysfs has been edited it also can not be
> used as a basic test of firmware.
>
> What is the downside to allowing the calling of function 0?

It needlessly expands the kernel ABI. I would suggest, if you want to
test acpi_nfit_ctl() path changes, expand the existing test
infrastructure we have in nfit_ctl_test(). If you want to test
firmware you don't need the upstream kernel to carry firmware debug
enabling in the production path, but I would support expanding
tools/testing/nvdimm/ to make it easier to test firmware.

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Sat, Jul 1, 2017 at 1:38 PM, Jerry Hoemann  wrote:
> On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
>> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  
>> wrote:
>> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  
>> > wrote:
>> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>> >>
>> >> ...
>> >>
>> >>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  
>> >>> wrote:
>> >>> > +   if (cmd == ND_CMD_CALL)
>> >>> > +   dsm_mask = nd_desc->bus_dsm_mask;
>> >>> > desc = nd_cmd_bus_desc(cmd);
>> >>> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >>> > handle = adev->handle;
>> >>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
>> >>> > acpi_nfit_desc *acpi_desc)
>> >>> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
>> >>> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> >>> > struct acpi_device *adev;
>> >>> > +   unsigned long dsm_mask;
>> >>> > int i;
>> >>> >
>> >>> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
>> >>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
>> >>> > acpi_nfit_desc *acpi_desc)
>> >>> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>> >>> > set_bit(i, _desc->cmd_mask);
>> >>> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
>> >>> > +
>> >>> > +   dsm_mask = 0x3bf;
>> >>>
>> >>> I went ahead and fixed this up to use dsm_mask defined like this:
>> >>>
>> >>> +   dsm_mask =
>> >>> +   (1 << ND_CMD_ARS_CAP) |
>> >>> +   (1 << ND_CMD_ARS_START) |
>> >>> +   (1 << ND_CMD_ARS_STATUS) |
>> >>> +   (1 << ND_CMD_CLEAR_ERROR) |
>> >>> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
>> >>> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
>> >>> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>> >>> +   (1 << NFIT_CMD_ARS_INJECT_GET);
>> >>>
>> >>> This drops function number 0 which userspace has no need to call.
>> >>
>> >> Actually I like to call function 0.  Its an excellent test when
>> >> modifying the code path as its a no side effects function whose output
>> >> is known in advance and instantly recognizable.  I also use it when
>> >> testing new firmware.
>> >>
>> >> What is the downside to allowing it?  What bad things happen?
>> >
>> > It allows implementations to bypass the standardization process and
>> > ship new root DSMs. It's always possible to patch the kernel locally
>> > for development, so I see no reason to ship this capability globally.
>
> I don't understand this comment, but I think your next comment
> essentially says to disregard this comment?

Yes, sorry.

>> Actually, just the discovery portion does not lead to this leak, but
>> it's redundant when we have the 'dsm_mask' sysfs attribute.
>
> No.  The generation of the mask in sysfs is not done by
> executing the code in acpi_nfit_ctl.  One of the reasons I call
> function 0 to test changes I am making to the ioctl path itself.
> The sysfs has nothing to do with that path and cannot be used
> to serve this purpose.
>
> And since the content of sysfs has been edited it also can not be
> used as a basic test of firmware.
>
> What is the downside to allowing the calling of function 0?

It needlessly expands the kernel ABI. I would suggest, if you want to
test acpi_nfit_ctl() path changes, expand the existing test
infrastructure we have in nfit_ctl_test(). If you want to test
firmware you don't need the upstream kernel to carry firmware debug
enabling in the production path, but I would support expanding
tools/testing/nvdimm/ to make it easier to test firmware.

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Jerry Hoemann]

On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  wrote:
> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  
> > wrote:
> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
> >>
> >> ...
> >>
> >>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  
> >>> wrote:
> >>> > +   if (cmd == ND_CMD_CALL)
> >>> > +   dsm_mask = nd_desc->bus_dsm_mask;
> >>> > desc = nd_cmd_bus_desc(cmd);
> >>> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >>> > handle = adev->handle;
> >>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
> >>> > acpi_nfit_desc *acpi_desc)
> >>> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
> >>> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >>> > struct acpi_device *adev;
> >>> > +   unsigned long dsm_mask;
> >>> > int i;
> >>> >
> >>> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> >>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
> >>> > acpi_nfit_desc *acpi_desc)
> >>> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> >>> > set_bit(i, _desc->cmd_mask);
> >>> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
> >>> > +
> >>> > +   dsm_mask = 0x3bf;
> >>>
> >>> I went ahead and fixed this up to use dsm_mask defined like this:
> >>>
> >>> +   dsm_mask =
> >>> +   (1 << ND_CMD_ARS_CAP) |
> >>> +   (1 << ND_CMD_ARS_START) |
> >>> +   (1 << ND_CMD_ARS_STATUS) |
> >>> +   (1 << ND_CMD_CLEAR_ERROR) |
> >>> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
> >>> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
> >>> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
> >>> +   (1 << NFIT_CMD_ARS_INJECT_GET);
> >>>
> >>> This drops function number 0 which userspace has no need to call.
> >>
> >> Actually I like to call function 0.  Its an excellent test when
> >> modifying the code path as its a no side effects function whose output
> >> is known in advance and instantly recognizable.  I also use it when
> >> testing new firmware.
> >>
> >> What is the downside to allowing it?  What bad things happen?
> >
> > It allows implementations to bypass the standardization process and
> > ship new root DSMs. It's always possible to patch the kernel locally
> > for development, so I see no reason to ship this capability globally.

I don't understand this comment, but I think your next comment 
essentially says to disregard this comment?

> 
> Actually, just the discovery portion does not lead to this leak, but
> it's redundant when we have the 'dsm_mask' sysfs attribute.

No.  The generation of the mask in sysfs is not done by
executing the code in acpi_nfit_ctl.  One of the reasons I call
function 0 to test changes I am making to the ioctl path itself.
The sysfs has nothing to do with that path and cannot be used
to serve this purpose.

And since the content of sysfs has been edited it also can not be
used as a basic test of firmware.

What is the downside to allowing the calling of function 0?



-- 

-
Jerry Hoemann  Software Engineer   Hewlett Packard Enterprise
-

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Jerry Hoemann]

On Sat, Jul 01, 2017 at 01:10:31PM -0700, Dan Williams wrote:
> On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  wrote:
> > On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  
> > wrote:
> >> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
> >>
> >> ...
> >>
> >>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  
> >>> wrote:
> >>> > +   if (cmd == ND_CMD_CALL)
> >>> > +   dsm_mask = nd_desc->bus_dsm_mask;
> >>> > desc = nd_cmd_bus_desc(cmd);
> >>> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >>> > handle = adev->handle;
> >>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
> >>> > acpi_nfit_desc *acpi_desc)
> >>> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
> >>> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
> >>> > struct acpi_device *adev;
> >>> > +   unsigned long dsm_mask;
> >>> > int i;
> >>> >
> >>> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> >>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
> >>> > acpi_nfit_desc *acpi_desc)
> >>> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> >>> > set_bit(i, _desc->cmd_mask);
> >>> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
> >>> > +
> >>> > +   dsm_mask = 0x3bf;
> >>>
> >>> I went ahead and fixed this up to use dsm_mask defined like this:
> >>>
> >>> +   dsm_mask =
> >>> +   (1 << ND_CMD_ARS_CAP) |
> >>> +   (1 << ND_CMD_ARS_START) |
> >>> +   (1 << ND_CMD_ARS_STATUS) |
> >>> +   (1 << ND_CMD_CLEAR_ERROR) |
> >>> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
> >>> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
> >>> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
> >>> +   (1 << NFIT_CMD_ARS_INJECT_GET);
> >>>
> >>> This drops function number 0 which userspace has no need to call.
> >>
> >> Actually I like to call function 0.  Its an excellent test when
> >> modifying the code path as its a no side effects function whose output
> >> is known in advance and instantly recognizable.  I also use it when
> >> testing new firmware.
> >>
> >> What is the downside to allowing it?  What bad things happen?
> >
> > It allows implementations to bypass the standardization process and
> > ship new root DSMs. It's always possible to patch the kernel locally
> > for development, so I see no reason to ship this capability globally.

I don't understand this comment, but I think your next comment 
essentially says to disregard this comment?

> 
> Actually, just the discovery portion does not lead to this leak, but
> it's redundant when we have the 'dsm_mask' sysfs attribute.

No.  The generation of the mask in sysfs is not done by
executing the code in acpi_nfit_ctl.  One of the reasons I call
function 0 to test changes I am making to the ioctl path itself.
The sysfs has nothing to do with that path and cannot be used
to serve this purpose.

And since the content of sysfs has been edited it also can not be
used as a basic test of firmware.

What is the downside to allowing the calling of function 0?



-- 

-
Jerry Hoemann  Software Engineer   Hewlett Packard Enterprise
-

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  wrote:
> On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  wrote:
>> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>>
>> ...
>>
>>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  
>>> wrote:
>>> > +   if (cmd == ND_CMD_CALL)
>>> > +   dsm_mask = nd_desc->bus_dsm_mask;
>>> > desc = nd_cmd_bus_desc(cmd);
>>> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
>>> > handle = adev->handle;
>>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
>>> > acpi_nfit_desc *acpi_desc)
>>> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
>>> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
>>> > struct acpi_device *adev;
>>> > +   unsigned long dsm_mask;
>>> > int i;
>>> >
>>> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
>>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
>>> > acpi_nfit_desc *acpi_desc)
>>> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>>> > set_bit(i, _desc->cmd_mask);
>>> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
>>> > +
>>> > +   dsm_mask = 0x3bf;
>>>
>>> I went ahead and fixed this up to use dsm_mask defined like this:
>>>
>>> +   dsm_mask =
>>> +   (1 << ND_CMD_ARS_CAP) |
>>> +   (1 << ND_CMD_ARS_START) |
>>> +   (1 << ND_CMD_ARS_STATUS) |
>>> +   (1 << ND_CMD_CLEAR_ERROR) |
>>> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
>>> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
>>> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>>> +   (1 << NFIT_CMD_ARS_INJECT_GET);
>>>
>>> This drops function number 0 which userspace has no need to call.
>>
>> Actually I like to call function 0.  Its an excellent test when
>> modifying the code path as its a no side effects function whose output
>> is known in advance and instantly recognizable.  I also use it when
>> testing new firmware.
>>
>> What is the downside to allowing it?  What bad things happen?
>
> It allows implementations to bypass the standardization process and
> ship new root DSMs. It's always possible to patch the kernel locally
> for development, so I see no reason to ship this capability globally.

Actually, just the discovery portion does not lead to this leak, but
it's redundant when we have the 'dsm_mask' sysfs attribute.

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Sat, Jul 1, 2017 at 1:08 PM, Dan Williams  wrote:
> On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  wrote:
>> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>>
>> ...
>>
>>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  
>>> wrote:
>>> > +   if (cmd == ND_CMD_CALL)
>>> > +   dsm_mask = nd_desc->bus_dsm_mask;
>>> > desc = nd_cmd_bus_desc(cmd);
>>> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
>>> > handle = adev->handle;
>>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
>>> > acpi_nfit_desc *acpi_desc)
>>> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
>>> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
>>> > struct acpi_device *adev;
>>> > +   unsigned long dsm_mask;
>>> > int i;
>>> >
>>> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
>>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
>>> > acpi_nfit_desc *acpi_desc)
>>> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>>> > set_bit(i, _desc->cmd_mask);
>>> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
>>> > +
>>> > +   dsm_mask = 0x3bf;
>>>
>>> I went ahead and fixed this up to use dsm_mask defined like this:
>>>
>>> +   dsm_mask =
>>> +   (1 << ND_CMD_ARS_CAP) |
>>> +   (1 << ND_CMD_ARS_START) |
>>> +   (1 << ND_CMD_ARS_STATUS) |
>>> +   (1 << ND_CMD_CLEAR_ERROR) |
>>> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
>>> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
>>> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>>> +   (1 << NFIT_CMD_ARS_INJECT_GET);
>>>
>>> This drops function number 0 which userspace has no need to call.
>>
>> Actually I like to call function 0.  Its an excellent test when
>> modifying the code path as its a no side effects function whose output
>> is known in advance and instantly recognizable.  I also use it when
>> testing new firmware.
>>
>> What is the downside to allowing it?  What bad things happen?
>
> It allows implementations to bypass the standardization process and
> ship new root DSMs. It's always possible to patch the kernel locally
> for development, so I see no reason to ship this capability globally.

Actually, just the discovery portion does not lead to this leak, but
it's redundant when we have the 'dsm_mask' sysfs attribute.

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  wrote:
> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>
> ...
>
>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  wrote:
>> > +   if (cmd == ND_CMD_CALL)
>> > +   dsm_mask = nd_desc->bus_dsm_mask;
>> > desc = nd_cmd_bus_desc(cmd);
>> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> > handle = adev->handle;
>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
>> > acpi_nfit_desc *acpi_desc)
>> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
>> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> > struct acpi_device *adev;
>> > +   unsigned long dsm_mask;
>> > int i;
>> >
>> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
>> > acpi_nfit_desc *acpi_desc)
>> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>> > set_bit(i, _desc->cmd_mask);
>> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
>> > +
>> > +   dsm_mask = 0x3bf;
>>
>> I went ahead and fixed this up to use dsm_mask defined like this:
>>
>> +   dsm_mask =
>> +   (1 << ND_CMD_ARS_CAP) |
>> +   (1 << ND_CMD_ARS_START) |
>> +   (1 << ND_CMD_ARS_STATUS) |
>> +   (1 << ND_CMD_CLEAR_ERROR) |
>> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
>> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
>> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>> +   (1 << NFIT_CMD_ARS_INJECT_GET);
>>
>> This drops function number 0 which userspace has no need to call.
>
> Actually I like to call function 0.  Its an excellent test when
> modifying the code path as its a no side effects function whose output
> is known in advance and instantly recognizable.  I also use it when
> testing new firmware.
>
> What is the downside to allowing it?  What bad things happen?

It allows implementations to bypass the standardization process and
ship new root DSMs. It's always possible to patch the kernel locally
for development, so I see no reason to ship this capability globally.

> Also, I do have to ask why you allow function zero for NVDIMM_FAMILY_MSFT?

Yeah, that's an oversight / mistake, but it's also benign since it
can't be used to add support for new function numbers to the family
since all 32 numbers are already taken. We also allow override for
leaf devices since there's quite a bit more per vendor differentiation
that might take a while to standardize.

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Sat, Jul 1, 2017 at 12:58 PM, Jerry Hoemann  wrote:
> On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:
>
> ...
>
>> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  wrote:
>> > +   if (cmd == ND_CMD_CALL)
>> > +   dsm_mask = nd_desc->bus_dsm_mask;
>> > desc = nd_cmd_bus_desc(cmd);
>> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> > handle = adev->handle;
>> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct 
>> > acpi_nfit_desc *acpi_desc)
>> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
>> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
>> > struct acpi_device *adev;
>> > +   unsigned long dsm_mask;
>> > int i;
>> >
>> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
>> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
>> > acpi_nfit_desc *acpi_desc)
>> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
>> > set_bit(i, _desc->cmd_mask);
>> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
>> > +
>> > +   dsm_mask = 0x3bf;
>>
>> I went ahead and fixed this up to use dsm_mask defined like this:
>>
>> +   dsm_mask =
>> +   (1 << ND_CMD_ARS_CAP) |
>> +   (1 << ND_CMD_ARS_START) |
>> +   (1 << ND_CMD_ARS_STATUS) |
>> +   (1 << ND_CMD_CLEAR_ERROR) |
>> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
>> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
>> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
>> +   (1 << NFIT_CMD_ARS_INJECT_GET);
>>
>> This drops function number 0 which userspace has no need to call.
>
> Actually I like to call function 0.  Its an excellent test when
> modifying the code path as its a no side effects function whose output
> is known in advance and instantly recognizable.  I also use it when
> testing new firmware.
>
> What is the downside to allowing it?  What bad things happen?

It allows implementations to bypass the standardization process and
ship new root DSMs. It's always possible to patch the kernel locally
for development, so I see no reason to ship this capability globally.

> Also, I do have to ask why you allow function zero for NVDIMM_FAMILY_MSFT?

Yeah, that's an oversight / mistake, but it's also benign since it
can't be used to add support for new function numbers to the family
since all 32 numbers are already taken. We also allow override for
leaf devices since there's quite a bit more per vendor differentiation
that might take a while to standardize.

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Jerry Hoemann]

On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:

...

> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  wrote:
> > +   if (cmd == ND_CMD_CALL)
> > +   dsm_mask = nd_desc->bus_dsm_mask;
> > desc = nd_cmd_bus_desc(cmd);
> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
> > handle = adev->handle;
> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc 
> > *acpi_desc)
> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
> > struct acpi_device *adev;
> > +   unsigned long dsm_mask;
> > int i;
> >
> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
> > acpi_nfit_desc *acpi_desc)
> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> > set_bit(i, _desc->cmd_mask);
> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
> > +
> > +   dsm_mask = 0x3bf;
> 
> I went ahead and fixed this up to use dsm_mask defined like this:
> 
> +   dsm_mask =
> +   (1 << ND_CMD_ARS_CAP) |
> +   (1 << ND_CMD_ARS_START) |
> +   (1 << ND_CMD_ARS_STATUS) |
> +   (1 << ND_CMD_CLEAR_ERROR) |
> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
> +   (1 << NFIT_CMD_ARS_INJECT_GET);
> 
> This drops function number 0 which userspace has no need to call.

Actually I like to call function 0.  Its an excellent test when
modifying the code path as its a no side effects function whose output
is known in advance and instantly recognizable.  I also use it when
testing new firmware.  

What is the downside to allowing it?  What bad things happen?

Also, I do have to ask why you allow function zero for NVDIMM_FAMILY_MSFT?
-- 

-
Jerry Hoemann  Software Engineer   Hewlett Packard Enterprise
-

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Jerry Hoemann]

On Fri, Jun 30, 2017 at 08:55:22PM -0700, Dan Williams wrote:

...

> On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  wrote:
> > +   if (cmd == ND_CMD_CALL)
> > +   dsm_mask = nd_desc->bus_dsm_mask;
> > desc = nd_cmd_bus_desc(cmd);
> > uuid = to_nfit_uuid(NFIT_DEV_BUS);
> > handle = adev->handle;
> > @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc 
> > *acpi_desc)
> > struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
> > const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
> > struct acpi_device *adev;
> > +   unsigned long dsm_mask;
> > int i;
> >
> > nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> > @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct 
> > acpi_nfit_desc *acpi_desc)
> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> > set_bit(i, _desc->cmd_mask);
> > set_bit(ND_CMD_CALL, _desc->cmd_mask);
> > +
> > +   dsm_mask = 0x3bf;
> 
> I went ahead and fixed this up to use dsm_mask defined like this:
> 
> +   dsm_mask =
> +   (1 << ND_CMD_ARS_CAP) |
> +   (1 << ND_CMD_ARS_START) |
> +   (1 << ND_CMD_ARS_STATUS) |
> +   (1 << ND_CMD_CLEAR_ERROR) |
> +   (1 << NFIT_CMD_TRANSLATE_SPA) |
> +   (1 << NFIT_CMD_ARS_INJECT_SET) |
> +   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
> +   (1 << NFIT_CMD_ARS_INJECT_GET);
> 
> This drops function number 0 which userspace has no need to call.

Actually I like to call function 0.  Its an excellent test when
modifying the code path as its a no side effects function whose output
is known in advance and instantly recognizable.  I also use it when
testing new firmware.  

What is the downside to allowing it?  What bad things happen?

Also, I do have to ask why you allow function zero for NVDIMM_FAMILY_MSFT?
-- 

-
Jerry Hoemann  Software Engineer   Hewlett Packard Enterprise
-

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  wrote:
> Add a bus level dsm_mask to nvdimm_bus_descriptor to allow the passthru
> calling mechanism to specify a different mask from the cmd_mask.
>
> Populate bus_dsm_mask and use it to filter dsm calls that user can
> make through the pass thru interface.
>
> Signed-off-by: Jerry Hoemann 
> ---
>  drivers/acpi/nfit/core.c  | 8 
>  include/linux/libnvdimm.h | 1 +
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index b46fca2..5e4c137 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, 
> struct nvdimm *nvdimm,
> cmd_name = nvdimm_bus_cmd_name(cmd);
> cmd_mask = nd_desc->cmd_mask;
> dsm_mask = cmd_mask;
> +   if (cmd == ND_CMD_CALL)
> +   dsm_mask = nd_desc->bus_dsm_mask;
> desc = nd_cmd_bus_desc(cmd);
> uuid = to_nfit_uuid(NFIT_DEV_BUS);
> handle = adev->handle;
> @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc 
> *acpi_desc)
> struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
> const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
> struct acpi_device *adev;
> +   unsigned long dsm_mask;
> int i;
>
> nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc 
> *acpi_desc)
> if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> set_bit(i, _desc->cmd_mask);
> set_bit(ND_CMD_CALL, _desc->cmd_mask);
> +
> +   dsm_mask = 0x3bf;

I went ahead and fixed this up to use dsm_mask defined like this:

+   dsm_mask =
+   (1 << ND_CMD_ARS_CAP) |
+   (1 << ND_CMD_ARS_START) |
+   (1 << ND_CMD_ARS_STATUS) |
+   (1 << ND_CMD_CLEAR_ERROR) |
+   (1 << NFIT_CMD_TRANSLATE_SPA) |
+   (1 << NFIT_CMD_ARS_INJECT_SET) |
+   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
+   (1 << NFIT_CMD_ARS_INJECT_GET);

This drops function number 0 which userspace has no need to call.

Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Dan Williams]

On Fri, Jun 30, 2017 at 9:09 AM, Jerry Hoemann  wrote:
> Add a bus level dsm_mask to nvdimm_bus_descriptor to allow the passthru
> calling mechanism to specify a different mask from the cmd_mask.
>
> Populate bus_dsm_mask and use it to filter dsm calls that user can
> make through the pass thru interface.
>
> Signed-off-by: Jerry Hoemann 
> ---
>  drivers/acpi/nfit/core.c  | 8 
>  include/linux/libnvdimm.h | 1 +
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index b46fca2..5e4c137 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, 
> struct nvdimm *nvdimm,
> cmd_name = nvdimm_bus_cmd_name(cmd);
> cmd_mask = nd_desc->cmd_mask;
> dsm_mask = cmd_mask;
> +   if (cmd == ND_CMD_CALL)
> +   dsm_mask = nd_desc->bus_dsm_mask;
> desc = nd_cmd_bus_desc(cmd);
> uuid = to_nfit_uuid(NFIT_DEV_BUS);
> handle = adev->handle;
> @@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc 
> *acpi_desc)
> struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
> const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
> struct acpi_device *adev;
> +   unsigned long dsm_mask;
> int i;
>
> nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
> @@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc 
> *acpi_desc)
> if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
> set_bit(i, _desc->cmd_mask);
> set_bit(ND_CMD_CALL, _desc->cmd_mask);
> +
> +   dsm_mask = 0x3bf;

I went ahead and fixed this up to use dsm_mask defined like this:

+   dsm_mask =
+   (1 << ND_CMD_ARS_CAP) |
+   (1 << ND_CMD_ARS_START) |
+   (1 << ND_CMD_ARS_STATUS) |
+   (1 << ND_CMD_CLEAR_ERROR) |
+   (1 << NFIT_CMD_TRANSLATE_SPA) |
+   (1 << NFIT_CMD_ARS_INJECT_SET) |
+   (1 << NFIT_CMD_ARS_INJECT_CLEAR) |
+   (1 << NFIT_CMD_ARS_INJECT_GET);

This drops function number 0 which userspace has no need to call.

[PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Jerry Hoemann]

Add a bus level dsm_mask to nvdimm_bus_descriptor to allow the passthru
calling mechanism to specify a different mask from the cmd_mask.

Populate bus_dsm_mask and use it to filter dsm calls that user can
make through the pass thru interface.

Signed-off-by: Jerry Hoemann 
---
 drivers/acpi/nfit/core.c  | 8 
 include/linux/libnvdimm.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index b46fca2..5e4c137 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, 
struct nvdimm *nvdimm,
cmd_name = nvdimm_bus_cmd_name(cmd);
cmd_mask = nd_desc->cmd_mask;
dsm_mask = cmd_mask;
+   if (cmd == ND_CMD_CALL)
+   dsm_mask = nd_desc->bus_dsm_mask;
desc = nd_cmd_bus_desc(cmd);
uuid = to_nfit_uuid(NFIT_DEV_BUS);
handle = adev->handle;
@@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc 
*acpi_desc)
struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
struct acpi_device *adev;
+   unsigned long dsm_mask;
int i;
 
nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
@@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc 
*acpi_desc)
if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
set_bit(i, _desc->cmd_mask);
set_bit(ND_CMD_CALL, _desc->cmd_mask);
+
+   dsm_mask = 0x3bf;
+   for_each_set_bit(i, _mask, BITS_PER_LONG)
+   if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
+   set_bit(i, _desc->bus_dsm_mask);
 }
 
 static ssize_t range_index_show(struct device *dev,
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 6c80701..f8b8f43 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -54,6 +54,7 @@ typedef int (*ndctl_fn)(struct nvdimm_bus_descriptor *nd_desc,
 
 struct nvdimm_bus_descriptor {
const struct attribute_group **attr_groups;
+   unsigned long bus_dsm_mask;
unsigned long cmd_mask;
struct module *module;
char *provider_name;
-- 
1.8.5.6

[PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru.

[Jerry Hoemann]

Add a bus level dsm_mask to nvdimm_bus_descriptor to allow the passthru
calling mechanism to specify a different mask from the cmd_mask.

Populate bus_dsm_mask and use it to filter dsm calls that user can
make through the pass thru interface.

Signed-off-by: Jerry Hoemann 
---
 drivers/acpi/nfit/core.c  | 8 
 include/linux/libnvdimm.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index b46fca2..5e4c137 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, 
struct nvdimm *nvdimm,
cmd_name = nvdimm_bus_cmd_name(cmd);
cmd_mask = nd_desc->cmd_mask;
dsm_mask = cmd_mask;
+   if (cmd == ND_CMD_CALL)
+   dsm_mask = nd_desc->bus_dsm_mask;
desc = nd_cmd_bus_desc(cmd);
uuid = to_nfit_uuid(NFIT_DEV_BUS);
handle = adev->handle;
@@ -1613,6 +1615,7 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc 
*acpi_desc)
struct nvdimm_bus_descriptor *nd_desc = _desc->nd_desc;
const u8 *uuid = to_nfit_uuid(NFIT_DEV_BUS);
struct acpi_device *adev;
+   unsigned long dsm_mask;
int i;
 
nd_desc->cmd_mask = acpi_desc->bus_cmd_force_en;
@@ -1624,6 +1627,11 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc 
*acpi_desc)
if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
set_bit(i, _desc->cmd_mask);
set_bit(ND_CMD_CALL, _desc->cmd_mask);
+
+   dsm_mask = 0x3bf;
+   for_each_set_bit(i, _mask, BITS_PER_LONG)
+   if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i))
+   set_bit(i, _desc->bus_dsm_mask);
 }
 
 static ssize_t range_index_show(struct device *dev,
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 6c80701..f8b8f43 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -54,6 +54,7 @@ typedef int (*ndctl_fn)(struct nvdimm_bus_descriptor *nd_desc,
 
 struct nvdimm_bus_descriptor {
const struct attribute_group **attr_groups;
+   unsigned long bus_dsm_mask;
unsigned long cmd_mask;
struct module *module;
char *provider_name;
-- 
1.8.5.6