Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751898AbdGEQfv (ORCPT ); Wed, 5 Jul 2017 12:35:51 -0400 Received: from mail-yb0-f175.google.com ([209.85.213.175]:36451 "EHLO mail-yb0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751678AbdGEQft (ORCPT ); Wed, 5 Jul 2017 12:35:49 -0400 MIME-Version: 1.0 In-Reply-To: <20170705162418.GA24924@anatevka.americas.hpqcorp.net> References: <293ee143dcabb386ca06b384a384171c256a2ecc.1498810220.git.jerry.hoemann@hpe.com> <20170701195805.GA13259@anatevka.americas.hpqcorp.net> <20170701203813.GA13574@anatevka.americas.hpqcorp.net> <20170704200849.GA15713@anatevka.americas.hpqcorp.net> <20170705162418.GA24924@anatevka.americas.hpqcorp.net> From: Dan Williams Date: Wed, 5 Jul 2017 09:35:48 -0700 Message-ID: Subject: Re: [PATCH v4 3/6] libnvdimm, acpi, nfit: Add bus level dsm mask for pass thru. To: Jerry Hoemann Cc: "linux-nvdimm@lists.01.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6013 Lines: 122 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 specification. This goes back to the original reasoning for pushing back on the override for the leaf-level _DSM methods. Specifically the ability to bypass the standardization process to ship vendor-specific behavior. Now, the other side of the argument is that if the next spec adds new _DSMs a simple override can enable them. I am more sympathetic to the override for the leaf / DIMM level because those _DSMs truly are DIMM-vendor specific, but the root device is not. Also, none of the root-level DSMs added for 6.2 are in any way critical for proper operation of the platform, and I do not see any bus-level functionality on the horizon that we need to aggressively pre-enable. It was a mistake to use _DSM for common root-level functionality, and we shouldn't double down on that mistake by allowing unfettered definition of new interfaces. NVDIMM is not so special that it needs to bypass the standard ACPI-to-kernel development pipeline.