[v11,5/6] ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods

Commit Message

Vaibhav Jain June 7, 2020, 1:13 p.m. UTC

Introduce support for PAPR NVDIMM Specific Methods (PDSM) in papr_scm
module and add the command family NVDIMM_FAMILY_PAPR to the white list
of NVDIMM command sets. Also advertise support for ND_CMD_CALL for the
nvdimm command mask and implement necessary scaffolding in the module
to handle ND_CMD_CALL ioctl and PDSM requests that we receive.

The layout of the PDSM request as we expect from libnvdimm/libndctl is
described in newly introduced uapi header 'papr_pdsm.h' which
defines a new 'struct nd_pdsm_cmd_pkg' header. This header is used
to communicate the PDSM request via member
'nd_cmd_pkg.nd_command' and size of payload that need to be
sent/received for servicing the PDSM.

A new function is_cmd_valid() is implemented that reads the args to
papr_scm_ndctl() and performs sanity tests on them. A new function
papr_scm_service_pdsm() is introduced and is called from
papr_scm_ndctl() in case of a PDSM request is received via ND_CMD_CALL
command from libnvdimm.

Cc: "Aneesh Kumar K . V" <[e-mail]>
Cc: Dan Williams <[e-mail]>
Cc: Michael Ellerman <[e-mail]>
Cc: Ira Weiny <[e-mail]>
Signed-off-by: Vaibhav Jain <[e-mail]>
---
Changelog:

v10..v11:
* Moved in-lines 'nd_pdsm_cmd_pkg()' and 'pdsm_cmd_to_payload()' from
  'papr_pdsm.h' header to 'papr_scm.c'. The avoids a potential license
  incompatibility issue with non-GPL-2.0 user-space code trying to
  include the header in its code. [ Ira ]
* Verified papr_pdsm.h with UAPI_HEADER_TEST config.
* Moved the is_cmd_valid() check in papr_scm_ndctl() before check for
  cmd_rc == NULL. This prevents cmd_rc to be updated in case the
  nd-cmd is invalid or unknown.

v9..v10:
* Simplified 'struct nd_pdsm_cmd_pkg' by removing the
  'payload_version' field.
* Removed the corrosponding documentation on versioning and backward
  compatibility from 'papr_pdsm.h'
* Reduced the size of reserved fields to 4-bytes making 'struct
  nd_pdsm_cmd_pkg' 64 + 8 bytes long.
* Updated is_cmd_valid() to enforce validation checks on pdsm
  commands. [ Dan Williams ]
* Added check for reserved fields being set to '0' in is_cmd_valid()
  [ Ira ]
* Moved changes for checking cmd_rc == NULL and logging improvements
  to a separate prelim patch [ Ira ].
* Moved  pdsm package validation checks from papr_scm_service_pdsm()
  to is_cmd_valid().
* Marked papr_scm_service_pdsm() return type as 'void' since errors
  are reported in nd_pdsm_cmd_pkg.cmd_status field.

Resend:
* Added ack from Aneesh.

v8..v9:
* Reduced the usage of term SCM replacing it with appropriate
  replacement [ Dan Williams, Aneesh ]
* Renamed 'papr_scm_pdsm.h' to 'papr_pdsm.h'
* s/PAPR_SCM_PDSM_*/PAPR_PDSM_*/g
* s/NVDIMM_FAMILY_PAPR_SCM/NVDIMM_FAMILY_PAPR/g
* Minor updates to 'papr_psdm.h' to replace usage of term 'SCM'.
* Minor update to patch description.

v7..v8:
* Removed the 'payload_offset' field from 'struct
  nd_pdsm_cmd_pkg'. Instead command payload is always assumed to start
  at 'nd_pdsm_cmd_pkg.payload'. [ Aneesh ]
* To enable introducing new fields to 'struct nd_pdsm_cmd_pkg',
  'reserved' field of 10-bytes is introduced. [ Aneesh ]
* Fixed a typo in "Backward Compatibility" section of papr_scm_pdsm.h
  [ Ira ]

Resend:
* None

v6..v7 :
* Removed the re-definitions of __packed macro from papr_scm_pdsm.h
  [Mpe].
* Removed the usage of __KERNEL__ macros in papr_scm_pdsm.h [Mpe].
* Removed macros that were unused in papr_scm.c from papr_scm_pdsm.h
  [Mpe].
* Made functions defined in papr_scm_pdsm.h as static inline. [Mpe]

v5..v6 :
* Changed the usage of the term DSM to PDSM to distinguish it from the
  ACPI term [ Dan Williams ]
* Renamed papr_scm_dsm.h to papr_scm_pdsm.h and updated various struct
  to reflect the new terminology.
* Updated the patch description and title to reflect the new terminology.
* Squashed patch to introduce new command family in 'ndctl.h' with
  this patch [ Dan Williams ]
* Updated the papr_scm_pdsm method starting index from 0x10000 to 0x0
  [ Dan Williams ]
* Removed redundant license text from the papr_scm_psdm.h file.
  [ Dan Williams ]
* s/envelop/envelope/ at various places [ Dan Williams ]
* Added '__packed' attribute to command package header to gaurd
  against different compiler adding paddings between the fields.
  [ Dan Williams]
* Converted various pr_debug to dev_debug [ Dan Williams ]

v4..v5 :
* None

v3..v4 :
* None

v2..v3 :
* Updated the patch prefix to 'ndctl/uapi' [Aneesh]

v1..v2 :
* None
---
 arch/powerpc/include/uapi/asm/papr_pdsm.h |  84 +++++++++++++++
 arch/powerpc/platforms/pseries/papr_scm.c | 126 +++++++++++++++++++++-
 include/uapi/linux/ndctl.h                |   1 +
 3 files changed, 207 insertions(+), 4 deletions(-)
 create mode 100644 arch/powerpc/include/uapi/asm/papr_pdsm.h

Comments

Ira Weiny June 8, 2020, 4:24 p.m. UTC | #1

On Sun, Jun 07, 2020 at 06:43:38PM +0530, Vaibhav Jain wrote:
> Introduce support for PAPR NVDIMM Specific Methods (PDSM) in papr_scm
> module and add the command family NVDIMM_FAMILY_PAPR to the white list
> of NVDIMM command sets. Also advertise support for ND_CMD_CALL for the
> nvdimm command mask and implement necessary scaffolding in the module
> to handle ND_CMD_CALL ioctl and PDSM requests that we receive.
> 
> The layout of the PDSM request as we expect from libnvdimm/libndctl is
> described in newly introduced uapi header 'papr_pdsm.h' which
> defines a new 'struct nd_pdsm_cmd_pkg' header. This header is used
> to communicate the PDSM request via member
> 'nd_cmd_pkg.nd_command' and size of payload that need to be
> sent/received for servicing the PDSM.
> 
> A new function is_cmd_valid() is implemented that reads the args to
> papr_scm_ndctl() and performs sanity tests on them. A new function
> papr_scm_service_pdsm() is introduced and is called from
> papr_scm_ndctl() in case of a PDSM request is received via ND_CMD_CALL
> command from libnvdimm.
> 
> Cc: "Aneesh Kumar K . V" <[e-mail]>
> Cc: Dan Williams <[e-mail]>
> Cc: Michael Ellerman <[e-mail]>
> Cc: Ira Weiny <[e-mail]>
> Signed-off-by: Vaibhav Jain <[e-mail]>
> ---
> Changelog:
> 
> v10..v11:
> * Moved in-lines 'nd_pdsm_cmd_pkg()' and 'pdsm_cmd_to_payload()' from
>   'papr_pdsm.h' header to 'papr_scm.c'. The avoids a potential license
>   incompatibility issue with non-GPL-2.0 user-space code trying to
>   include the header in its code. [ Ira ]
> * Verified papr_pdsm.h with UAPI_HEADER_TEST config.
> * Moved the is_cmd_valid() check in papr_scm_ndctl() before check for
>   cmd_rc == NULL. This prevents cmd_rc to be updated in case the
>   nd-cmd is invalid or unknown.
> 
> v9..v10:
> * Simplified 'struct nd_pdsm_cmd_pkg' by removing the
>   'payload_version' field.
> * Removed the corrosponding documentation on versioning and backward
>   compatibility from 'papr_pdsm.h'
> * Reduced the size of reserved fields to 4-bytes making 'struct
>   nd_pdsm_cmd_pkg' 64 + 8 bytes long.
> * Updated is_cmd_valid() to enforce validation checks on pdsm
>   commands. [ Dan Williams ]
> * Added check for reserved fields being set to '0' in is_cmd_valid()
>   [ Ira ]
> * Moved changes for checking cmd_rc == NULL and logging improvements
>   to a separate prelim patch [ Ira ].
> * Moved  pdsm package validation checks from papr_scm_service_pdsm()
>   to is_cmd_valid().
> * Marked papr_scm_service_pdsm() return type as 'void' since errors
>   are reported in nd_pdsm_cmd_pkg.cmd_status field.
> 
> Resend:
> * Added ack from Aneesh.
> 
> v8..v9:
> * Reduced the usage of term SCM replacing it with appropriate
>   replacement [ Dan Williams, Aneesh ]
> * Renamed 'papr_scm_pdsm.h' to 'papr_pdsm.h'
> * s/PAPR_SCM_PDSM_*/PAPR_PDSM_*/g
> * s/NVDIMM_FAMILY_PAPR_SCM/NVDIMM_FAMILY_PAPR/g
> * Minor updates to 'papr_psdm.h' to replace usage of term 'SCM'.
> * Minor update to patch description.
> 
> v7..v8:
> * Removed the 'payload_offset' field from 'struct
>   nd_pdsm_cmd_pkg'. Instead command payload is always assumed to start
>   at 'nd_pdsm_cmd_pkg.payload'. [ Aneesh ]
> * To enable introducing new fields to 'struct nd_pdsm_cmd_pkg',
>   'reserved' field of 10-bytes is introduced. [ Aneesh ]
> * Fixed a typo in "Backward Compatibility" section of papr_scm_pdsm.h
>   [ Ira ]
> 
> Resend:
> * None
> 
> v6..v7 :
> * Removed the re-definitions of __packed macro from papr_scm_pdsm.h
>   [Mpe].
> * Removed the usage of __KERNEL__ macros in papr_scm_pdsm.h [Mpe].
> * Removed macros that were unused in papr_scm.c from papr_scm_pdsm.h
>   [Mpe].
> * Made functions defined in papr_scm_pdsm.h as static inline. [Mpe]
> 
> v5..v6 :
> * Changed the usage of the term DSM to PDSM to distinguish it from the
>   ACPI term [ Dan Williams ]
> * Renamed papr_scm_dsm.h to papr_scm_pdsm.h and updated various struct
>   to reflect the new terminology.
> * Updated the patch description and title to reflect the new terminology.
> * Squashed patch to introduce new command family in 'ndctl.h' with
>   this patch [ Dan Williams ]
> * Updated the papr_scm_pdsm method starting index from 0x10000 to 0x0
>   [ Dan Williams ]
> * Removed redundant license text from the papr_scm_psdm.h file.
>   [ Dan Williams ]
> * s/envelop/envelope/ at various places [ Dan Williams ]
> * Added '__packed' attribute to command package header to gaurd
>   against different compiler adding paddings between the fields.
>   [ Dan Williams]
> * Converted various pr_debug to dev_debug [ Dan Williams ]
> 
> v4..v5 :
> * None
> 
> v3..v4 :
> * None
> 
> v2..v3 :
> * Updated the patch prefix to 'ndctl/uapi' [Aneesh]
> 
> v1..v2 :
> * None
> ---
>  arch/powerpc/include/uapi/asm/papr_pdsm.h |  84 +++++++++++++++
>  arch/powerpc/platforms/pseries/papr_scm.c | 126 +++++++++++++++++++++-
>  include/uapi/linux/ndctl.h                |   1 +
>  3 files changed, 207 insertions(+), 4 deletions(-)
>  create mode 100644 arch/powerpc/include/uapi/asm/papr_pdsm.h
> 
> diff --git a/arch/powerpc/include/uapi/asm/papr_pdsm.h b/arch/powerpc/include/uapi/asm/papr_pdsm.h
> new file mode 100644
> index 000000000000..df2447455cfe
> --- /dev/null
> +++ b/arch/powerpc/include/uapi/asm/papr_pdsm.h
> @@ -0,0 +1,84 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * PAPR nvDimm Specific Methods (PDSM) and structs for libndctl
> + *
> + * (C) Copyright IBM 2020
> + *
> + * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
> + */
> +
> +#ifndef _UAPI_ASM_POWERPC_PAPR_PDSM_H_
> +#define _UAPI_ASM_POWERPC_PAPR_PDSM_H_
> +
> +#include <linux/types.h>
> +#include <linux/ndctl.h>
> +
> +/*
> + * PDSM Envelope:
> + *
> + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
> + * envelope which consists of a header and user-defined payload sections.
> + * The header is described by 'struct nd_pdsm_cmd_pkg' which expects a
> + * payload following it and accessible via 'nd_pdsm_cmd_pkg.payload' field.
> + * There is reserved field that can used to introduce new fields to the
> + * structure in future. It also tries to ensure that 'nd_pdsm_cmd_pkg.payload'
> + * lies at a 8-byte boundary.
> + *
> + *  +-------------+---------------------+---------------------------+
> + *  |   64-Bytes  |       8-Bytes       |       Max 184-Bytes       |
> + *  +-------------+---------------------+---------------------------+
> + *  |               nd_pdsm_cmd_pkg     |                           |
> + *  |-------------+                     |                           |
> + *  |  nd_cmd_pkg |                     |                           |
> + *  +-------------+---------------------+---------------------------+
> + *  | nd_family   |                     |                           |
> + *  | nd_size_out | cmd_status          |                           |
> + *  | nd_size_in  | reserved            |     payload               |
> + *  | nd_command  |                     |                           |
> + *  | nd_fw_size  |                     |                           |
> + *  +-------------+---------------------+---------------------------+
> + *
> + * PDSM Header:
> + *
> + * The header is defined as 'struct nd_pdsm_cmd_pkg' which embeds a
> + * 'struct nd_cmd_pkg' instance. The PDSM command is assigned to member
> + * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelope which is
> + * contained in 'struct nd_cmd_pkg', the header also has members following
> + * members:
> + *
> + * 'cmd_status'		: (Out) Errors if any encountered while servicing PDSM.
> + * 'reserved'		: Not used, reserved for future and should be set to 0.
> + *
> + * PDSM Payload:
> + *
> + * The layout of the PDSM Payload is defined by various structs shared between
> + * papr_scm and libndctl so that contents of payload can be interpreted. During
> + * servicing of a PDSM the papr_scm module will read input args from the payload
> + * field by casting its contents to an appropriate struct pointer based on the
> + * PDSM command. Similarly the output of servicing the PDSM command will be
> + * copied to the payload field using the same struct.
> + *
> + * 'libnvdimm' enforces a hard limit of 256 bytes on the envelope size, which
> + * leaves around 184 bytes for the envelope payload

'around'?

> (ignoring any padding that
> + * the compiler may silently introduce).

When building user interfaces like this you have to be more exact.  I think the
code is fine but you can't have the compiler silently moving things around or
have different compilers move things differently between the user app and the
kernel.  So these statements are not correct.

Ira

> + *
> + */
> +
> +/* PDSM-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
> +struct nd_pdsm_cmd_pkg {
> +	struct nd_cmd_pkg hdr;	/* Package header containing sub-cmd */
> +	__s32 cmd_status;	/* Out: Sub-cmd status returned back */
> +	__u16 reserved[2];	/* Ignored and to be used in future */
> +	__u8 payload[];		/* In/Out: Sub-cmd data buffer */
> +} __packed;
> +
> +/*
> + * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
> + * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
> + */
> +enum papr_pdsm {
> +	PAPR_PDSM_MIN = 0x0,
> +	PAPR_PDSM_MAX,
> +};
> +
> +#endif /* _UAPI_ASM_POWERPC_PAPR_PDSM_H_ */
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 692ad3d79826..167fcf0e249d 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -15,13 +15,15 @@
>  #include <linux/seq_buf.h>
>  
>  #include <asm/plpar_wrappers.h>
> +#include <asm/papr_pdsm.h>
>  
>  #define BIND_ANY_ADDR (~0ul)
>  
>  #define PAPR_SCM_DIMM_CMD_MASK \
>  	((1ul << ND_CMD_GET_CONFIG_SIZE) | \
>  	 (1ul << ND_CMD_GET_CONFIG_DATA) | \
> -	 (1ul << ND_CMD_SET_CONFIG_DATA))
> +	 (1ul << ND_CMD_SET_CONFIG_DATA) | \
> +	 (1ul << ND_CMD_CALL))
>  
>  /* DIMM health bitmap bitmap indicators */
>  /* SCM device is unable to persist memory contents */
> @@ -89,6 +91,21 @@ struct papr_scm_priv {
>  	u64 health_bitmap;
>  };
>  
> +/* Convert a libnvdimm nd_cmd_pkg to pdsm specific pkg */
> +static inline struct nd_pdsm_cmd_pkg *nd_to_pdsm_cmd_pkg(struct nd_cmd_pkg *cmd)
> +{
> +	return (struct nd_pdsm_cmd_pkg *)cmd;
> +}
> +
> +/* Return the payload pointer for a given pcmd */
> +static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
> +{
> +	if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
> +		return NULL;
> +	else
> +		return (void *)(pcmd->payload);
> +}
> +
>  static int drc_pmem_bind(struct papr_scm_priv *p)
>  {
>  	unsigned long ret[PLPAR_HCALL_BUFSIZE];
> @@ -349,17 +366,113 @@ static int papr_scm_meta_set(struct papr_scm_priv *p,
>  	return 0;
>  }
>  
> +/*
> + * Do a sanity checks on the inputs args to dimm-control function and return
> + * '0' if valid. This also does validation on ND_CMD_CALL sub-command packages.
> + */
> +static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
> +			unsigned int buf_len)
> +{
> +	unsigned long cmd_mask = PAPR_SCM_DIMM_CMD_MASK;
> +	struct nd_pdsm_cmd_pkg *pkg;
> +	struct nd_cmd_pkg *nd_cmd;
> +	struct papr_scm_priv *p;
> +	enum papr_pdsm pdsm;
> +
> +	/* Only dimm-specific calls are supported atm */
> +	if (!nvdimm)
> +		return -EINVAL;
> +
> +	/* get the provider data from struct nvdimm */
> +	p = nvdimm_provider_data(nvdimm);
> +
> +	if (!test_bit(cmd, &cmd_mask)) {
> +		dev_dbg(&p->pdev->dev, "Unsupported cmd=%u\n", cmd);
> +		return -EINVAL;
> +	}
> +
> +	/* For CMD_CALL verify pdsm request */
> +	if (cmd == ND_CMD_CALL) {
> +		/* Verify the envelope package */
> +		if (!buf || buf_len < sizeof(struct nd_pdsm_cmd_pkg)) {
> +			dev_dbg(&p->pdev->dev, "Invalid pkg size=%u\n",
> +				buf_len);
> +			return -EINVAL;
> +		}
> +
> +		/* Verify that the nd_cmd_pkg.nd_family is correct */
> +		nd_cmd = (struct nd_cmd_pkg *)buf;
> +		if (nd_cmd->nd_family != NVDIMM_FAMILY_PAPR) {
> +			dev_dbg(&p->pdev->dev, "Invalid pkg family=0x%llx\n",
> +				nd_cmd->nd_family);
> +			return -EINVAL;
> +		}
> +
> +		/* Get the pdsm request package and the command */
> +		pkg = nd_to_pdsm_cmd_pkg(nd_cmd);
> +		pdsm = pkg->hdr.nd_command;
> +
> +		/* Verify if the psdm command is valid */
> +		if (pdsm <= PAPR_PDSM_MIN || pdsm >= PAPR_PDSM_MAX) {
> +			dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Invalid PDSM\n", pdsm);
> +			return -EINVAL;
> +		}
> +
> +		/* We except a payload with all PDSM commands */
> +		if (!pdsm_cmd_to_payload(pkg)) {
> +			dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Empty payload\n", pdsm);
> +			return -EINVAL;
> +		}
> +
> +		/* Ensure reserved fields of the pdsm header are set to 0 */
> +		if (pkg->reserved[0] || pkg->reserved[1]) {
> +			dev_dbg(&p->pdev->dev,
> +				"PDSM[0x%x]: Invalid reserved field usage\n", pdsm);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Let the command be further processed */
> +	return 0;
> +}
> +
> +/*
> + * For a given pdsm request call an appropriate service function.
> + * Returns errors if any while handling the pdsm command package.
> + */
> +static int papr_scm_service_pdsm(struct papr_scm_priv *p,
> +				 struct nd_pdsm_cmd_pkg *pkg)
> +{
> +	const enum papr_pdsm pdsm = pkg->hdr.nd_command;
> +
> +	dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Servicing..\n", pdsm);
> +
> +	/* Call pdsm service function */
> +	switch (pdsm) {
> +	default:
> +		dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Unsupported PDSM request\n",
> +			pdsm);
> +		pkg->cmd_status = -ENOENT;
> +		break;
> +	}
> +
> +	return pkg->cmd_status;
> +}
> +
>  static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
>  			  struct nvdimm *nvdimm, unsigned int cmd, void *buf,
>  			  unsigned int buf_len, int *cmd_rc)
>  {
>  	struct nd_cmd_get_config_size *get_size_hdr;
> +	struct nd_pdsm_cmd_pkg *call_pkg = NULL;
>  	struct papr_scm_priv *p;
>  	int rc;
>  
> -	/* Only dimm-specific calls are supported atm */
> -	if (!nvdimm)
> -		return -EINVAL;
> +	rc = is_cmd_valid(nvdimm, cmd, buf, buf_len);
> +	if (rc) {
> +		pr_debug("Invalid cmd=0x%x. Err=%d\n", cmd, rc);
> +		return rc;
> +	}
>  
>  	/* Use a local variable in case cmd_rc pointer is NULL */
>  	if (!cmd_rc)
> @@ -385,6 +498,11 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
>  		*cmd_rc = papr_scm_meta_set(p, buf);
>  		break;
>  
> +	case ND_CMD_CALL:
> +		call_pkg = nd_to_pdsm_cmd_pkg(buf);
> +		*cmd_rc = papr_scm_service_pdsm(p, call_pkg);
> +		break;
> +
>  	default:
>  		dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
>  		return -EINVAL;
> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
> index de5d90212409..0e09dc5cec19 100644
> --- a/include/uapi/linux/ndctl.h
> +++ b/include/uapi/linux/ndctl.h
> @@ -244,6 +244,7 @@ struct nd_cmd_pkg {
>  #define NVDIMM_FAMILY_HPE2 2
>  #define NVDIMM_FAMILY_MSFT 3
>  #define NVDIMM_FAMILY_HYPERV 4
> +#define NVDIMM_FAMILY_PAPR 5
>  
>  #define ND_IOCTL_CALL			_IOWR(ND_IOCTL, ND_CMD_CALL,\
>  					struct nd_cmd_pkg)
> -- 
> 2.26.2
>

kernel test robot June 8, 2020, 4:59 p.m. UTC | #2

Hi Vaibhav,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on powerpc/next]
[also build test WARNING on linus/master v5.7 next-20200605]
[cannot apply to linux-nvdimm/libnvdimm-for-next scottwood/next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Vaibhav-Jain/powerpc-papr_scm-Add-support-for-reporting-nvdimm-health/20200607-211653
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-r016-20200607 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

In file included from <built-in>:1:
>> ./usr/include/asm/papr_pdsm.h:69:20: warning: field 'hdr' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
struct nd_cmd_pkg hdr;  /* Package header containing sub-cmd */
^
1 warning generated.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Vaibhav Jain June 8, 2020, 6:10 p.m. UTC | #3

Ira Weiny <[e-mail]> writes:

> On Sun, Jun 07, 2020 at 06:43:38PM +0530, Vaibhav Jain wrote:
>> Introduce support for PAPR NVDIMM Specific Methods (PDSM) in papr_scm
>> module and add the command family NVDIMM_FAMILY_PAPR to the white list
>> of NVDIMM command sets. Also advertise support for ND_CMD_CALL for the
>> nvdimm command mask and implement necessary scaffolding in the module
>> to handle ND_CMD_CALL ioctl and PDSM requests that we receive.
>> 
>> The layout of the PDSM request as we expect from libnvdimm/libndctl is
>> described in newly introduced uapi header 'papr_pdsm.h' which
>> defines a new 'struct nd_pdsm_cmd_pkg' header. This header is used
>> to communicate the PDSM request via member
>> 'nd_cmd_pkg.nd_command' and size of payload that need to be
>> sent/received for servicing the PDSM.
>> 
>> A new function is_cmd_valid() is implemented that reads the args to
>> papr_scm_ndctl() and performs sanity tests on them. A new function
>> papr_scm_service_pdsm() is introduced and is called from
>> papr_scm_ndctl() in case of a PDSM request is received via ND_CMD_CALL
>> command from libnvdimm.
>> 
>> Cc: "Aneesh Kumar K . V" <[e-mail]>
>> Cc: Dan Williams <[e-mail]>
>> Cc: Michael Ellerman <[e-mail]>
>> Cc: Ira Weiny <[e-mail]>
>> Signed-off-by: Vaibhav Jain <[e-mail]>
>> ---
>> Changelog:
>> 
>> v10..v11:
>> * Moved in-lines 'nd_pdsm_cmd_pkg()' and 'pdsm_cmd_to_payload()' from
>>   'papr_pdsm.h' header to 'papr_scm.c'. The avoids a potential license
>>   incompatibility issue with non-GPL-2.0 user-space code trying to
>>   include the header in its code. [ Ira ]
>> * Verified papr_pdsm.h with UAPI_HEADER_TEST config.
>> * Moved the is_cmd_valid() check in papr_scm_ndctl() before check for
>>   cmd_rc == NULL. This prevents cmd_rc to be updated in case the
>>   nd-cmd is invalid or unknown.
>> 
>> v9..v10:
>> * Simplified 'struct nd_pdsm_cmd_pkg' by removing the
>>   'payload_version' field.
>> * Removed the corrosponding documentation on versioning and backward
>>   compatibility from 'papr_pdsm.h'
>> * Reduced the size of reserved fields to 4-bytes making 'struct
>>   nd_pdsm_cmd_pkg' 64 + 8 bytes long.
>> * Updated is_cmd_valid() to enforce validation checks on pdsm
>>   commands. [ Dan Williams ]
>> * Added check for reserved fields being set to '0' in is_cmd_valid()
>>   [ Ira ]
>> * Moved changes for checking cmd_rc == NULL and logging improvements
>>   to a separate prelim patch [ Ira ].
>> * Moved  pdsm package validation checks from papr_scm_service_pdsm()
>>   to is_cmd_valid().
>> * Marked papr_scm_service_pdsm() return type as 'void' since errors
>>   are reported in nd_pdsm_cmd_pkg.cmd_status field.
>> 
>> Resend:
>> * Added ack from Aneesh.
>> 
>> v8..v9:
>> * Reduced the usage of term SCM replacing it with appropriate
>>   replacement [ Dan Williams, Aneesh ]
>> * Renamed 'papr_scm_pdsm.h' to 'papr_pdsm.h'
>> * s/PAPR_SCM_PDSM_*/PAPR_PDSM_*/g
>> * s/NVDIMM_FAMILY_PAPR_SCM/NVDIMM_FAMILY_PAPR/g
>> * Minor updates to 'papr_psdm.h' to replace usage of term 'SCM'.
>> * Minor update to patch description.
>> 
>> v7..v8:
>> * Removed the 'payload_offset' field from 'struct
>>   nd_pdsm_cmd_pkg'. Instead command payload is always assumed to start
>>   at 'nd_pdsm_cmd_pkg.payload'. [ Aneesh ]
>> * To enable introducing new fields to 'struct nd_pdsm_cmd_pkg',
>>   'reserved' field of 10-bytes is introduced. [ Aneesh ]
>> * Fixed a typo in "Backward Compatibility" section of papr_scm_pdsm.h
>>   [ Ira ]
>> 
>> Resend:
>> * None
>> 
>> v6..v7 :
>> * Removed the re-definitions of __packed macro from papr_scm_pdsm.h
>>   [Mpe].
>> * Removed the usage of __KERNEL__ macros in papr_scm_pdsm.h [Mpe].
>> * Removed macros that were unused in papr_scm.c from papr_scm_pdsm.h
>>   [Mpe].
>> * Made functions defined in papr_scm_pdsm.h as static inline. [Mpe]
>> 
>> v5..v6 :
>> * Changed the usage of the term DSM to PDSM to distinguish it from the
>>   ACPI term [ Dan Williams ]
>> * Renamed papr_scm_dsm.h to papr_scm_pdsm.h and updated various struct
>>   to reflect the new terminology.
>> * Updated the patch description and title to reflect the new terminology.
>> * Squashed patch to introduce new command family in 'ndctl.h' with
>>   this patch [ Dan Williams ]
>> * Updated the papr_scm_pdsm method starting index from 0x10000 to 0x0
>>   [ Dan Williams ]
>> * Removed redundant license text from the papr_scm_psdm.h file.
>>   [ Dan Williams ]
>> * s/envelop/envelope/ at various places [ Dan Williams ]
>> * Added '__packed' attribute to command package header to gaurd
>>   against different compiler adding paddings between the fields.
>>   [ Dan Williams]
>> * Converted various pr_debug to dev_debug [ Dan Williams ]
>> 
>> v4..v5 :
>> * None
>> 
>> v3..v4 :
>> * None
>> 
>> v2..v3 :
>> * Updated the patch prefix to 'ndctl/uapi' [Aneesh]
>> 
>> v1..v2 :
>> * None
>> ---
>>  arch/powerpc/include/uapi/asm/papr_pdsm.h |  84 +++++++++++++++
>>  arch/powerpc/platforms/pseries/papr_scm.c | 126 +++++++++++++++++++++-
>>  include/uapi/linux/ndctl.h                |   1 +
>>  3 files changed, 207 insertions(+), 4 deletions(-)
>>  create mode 100644 arch/powerpc/include/uapi/asm/papr_pdsm.h
>> 
>> diff --git a/arch/powerpc/include/uapi/asm/papr_pdsm.h b/arch/powerpc/include/uapi/asm/papr_pdsm.h
>> new file mode 100644
>> index 000000000000..df2447455cfe
>> --- /dev/null
>> +++ b/arch/powerpc/include/uapi/asm/papr_pdsm.h
>> @@ -0,0 +1,84 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * PAPR nvDimm Specific Methods (PDSM) and structs for libndctl
>> + *
>> + * (C) Copyright IBM 2020
>> + *
>> + * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
>> + */
>> +
>> +#ifndef _UAPI_ASM_POWERPC_PAPR_PDSM_H_
>> +#define _UAPI_ASM_POWERPC_PAPR_PDSM_H_
>> +
>> +#include <linux/types.h>
>> +#include <linux/ndctl.h>
>> +
>> +/*
>> + * PDSM Envelope:
>> + *
>> + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
>> + * envelope which consists of a header and user-defined payload sections.
>> + * The header is described by 'struct nd_pdsm_cmd_pkg' which expects a
>> + * payload following it and accessible via 'nd_pdsm_cmd_pkg.payload' field.
>> + * There is reserved field that can used to introduce new fields to the
>> + * structure in future. It also tries to ensure that 'nd_pdsm_cmd_pkg.payload'
>> + * lies at a 8-byte boundary.
>> + *
>> + *  +-------------+---------------------+---------------------------+
>> + *  |   64-Bytes  |       8-Bytes       |       Max 184-Bytes       |
>> + *  +-------------+---------------------+---------------------------+
>> + *  |               nd_pdsm_cmd_pkg     |                           |
>> + *  |-------------+                     |                           |
>> + *  |  nd_cmd_pkg |                     |                           |
>> + *  +-------------+---------------------+---------------------------+
>> + *  | nd_family   |                     |                           |
>> + *  | nd_size_out | cmd_status          |                           |
>> + *  | nd_size_in  | reserved            |     payload               |
>> + *  | nd_command  |                     |                           |
>> + *  | nd_fw_size  |                     |                           |
>> + *  +-------------+---------------------+---------------------------+
>> + *
>> + * PDSM Header:
>> + *
>> + * The header is defined as 'struct nd_pdsm_cmd_pkg' which embeds a
>> + * 'struct nd_cmd_pkg' instance. The PDSM command is assigned to member
>> + * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelope which is
>> + * contained in 'struct nd_cmd_pkg', the header also has members following
>> + * members:
>> + *
>> + * 'cmd_status'		: (Out) Errors if any encountered while servicing PDSM.
>> + * 'reserved'		: Not used, reserved for future and should be set to 0.
>> + *
>> + * PDSM Payload:
>> + *
>> + * The layout of the PDSM Payload is defined by various structs shared between
>> + * papr_scm and libndctl so that contents of payload can be interpreted. During
>> + * servicing of a PDSM the papr_scm module will read input args from the payload
>> + * field by casting its contents to an appropriate struct pointer based on the
>> + * PDSM command. Similarly the output of servicing the PDSM command will be
>> + * copied to the payload field using the same struct.
[.]
>> + *
>> + * 'libnvdimm' enforces a hard limit of 256 bytes on the envelope size, which
>> + * leaves around 184 bytes for the envelope payload
>
> 'around'?
>
>> (ignoring any padding that
>> + * the compiler may silently introduce).
>
> When building user interfaces like this you have to be more exact.  I think the
> code is fine but you can't have the compiler silently moving things around or
> have different compilers move things differently between the user app and the
> kernel.  So these statements are not correct.
>
Sure, will update these comment block to better express the '__packed' gcc
attribute semantics for struct nd_pdsm_cmd_pkg.

~ Vaibhav

> Ira
>
>> + *
>> + */
>> +
>> +/* PDSM-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
>> +struct nd_pdsm_cmd_pkg {
>> +	struct nd_cmd_pkg hdr;	/* Package header containing sub-cmd */
>> +	__s32 cmd_status;	/* Out: Sub-cmd status returned back */
>> +	__u16 reserved[2];	/* Ignored and to be used in future */
>> +	__u8 payload[];		/* In/Out: Sub-cmd data buffer */
>> +} __packed;
>> +
>> +/*
>> + * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
>> + * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
>> + */
>> +enum papr_pdsm {
>> +	PAPR_PDSM_MIN = 0x0,
>> +	PAPR_PDSM_MAX,
>> +};
>> +
>> +#endif /* _UAPI_ASM_POWERPC_PAPR_PDSM_H_ */
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 692ad3d79826..167fcf0e249d 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -15,13 +15,15 @@
>>  #include <linux/seq_buf.h>
>>  
>>  #include <asm/plpar_wrappers.h>
>> +#include <asm/papr_pdsm.h>
>>  
>>  #define BIND_ANY_ADDR (~0ul)
>>  
>>  #define PAPR_SCM_DIMM_CMD_MASK \
>>  	((1ul << ND_CMD_GET_CONFIG_SIZE) | \
>>  	 (1ul << ND_CMD_GET_CONFIG_DATA) | \
>> -	 (1ul << ND_CMD_SET_CONFIG_DATA))
>> +	 (1ul << ND_CMD_SET_CONFIG_DATA) | \
>> +	 (1ul << ND_CMD_CALL))
>>  
>>  /* DIMM health bitmap bitmap indicators */
>>  /* SCM device is unable to persist memory contents */
>> @@ -89,6 +91,21 @@ struct papr_scm_priv {
>>  	u64 health_bitmap;
>>  };
>>  
>> +/* Convert a libnvdimm nd_cmd_pkg to pdsm specific pkg */
>> +static inline struct nd_pdsm_cmd_pkg *nd_to_pdsm_cmd_pkg(struct nd_cmd_pkg *cmd)
>> +{
>> +	return (struct nd_pdsm_cmd_pkg *)cmd;
>> +}
>> +
>> +/* Return the payload pointer for a given pcmd */
>> +static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
>> +{
>> +	if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
>> +		return NULL;
>> +	else
>> +		return (void *)(pcmd->payload);
>> +}
>> +
>>  static int drc_pmem_bind(struct papr_scm_priv *p)
>>  {
>>  	unsigned long ret[PLPAR_HCALL_BUFSIZE];
>> @@ -349,17 +366,113 @@ static int papr_scm_meta_set(struct papr_scm_priv *p,
>>  	return 0;
>>  }
>>  
>> +/*
>> + * Do a sanity checks on the inputs args to dimm-control function and return
>> + * '0' if valid. This also does validation on ND_CMD_CALL sub-command packages.
>> + */
>> +static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
>> +			unsigned int buf_len)
>> +{
>> +	unsigned long cmd_mask = PAPR_SCM_DIMM_CMD_MASK;
>> +	struct nd_pdsm_cmd_pkg *pkg;
>> +	struct nd_cmd_pkg *nd_cmd;
>> +	struct papr_scm_priv *p;
>> +	enum papr_pdsm pdsm;
>> +
>> +	/* Only dimm-specific calls are supported atm */
>> +	if (!nvdimm)
>> +		return -EINVAL;
>> +
>> +	/* get the provider data from struct nvdimm */
>> +	p = nvdimm_provider_data(nvdimm);
>> +
>> +	if (!test_bit(cmd, &cmd_mask)) {
>> +		dev_dbg(&p->pdev->dev, "Unsupported cmd=%u\n", cmd);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* For CMD_CALL verify pdsm request */
>> +	if (cmd == ND_CMD_CALL) {
>> +		/* Verify the envelope package */
>> +		if (!buf || buf_len < sizeof(struct nd_pdsm_cmd_pkg)) {
>> +			dev_dbg(&p->pdev->dev, "Invalid pkg size=%u\n",
>> +				buf_len);
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* Verify that the nd_cmd_pkg.nd_family is correct */
>> +		nd_cmd = (struct nd_cmd_pkg *)buf;
>> +		if (nd_cmd->nd_family != NVDIMM_FAMILY_PAPR) {
>> +			dev_dbg(&p->pdev->dev, "Invalid pkg family=0x%llx\n",
>> +				nd_cmd->nd_family);
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* Get the pdsm request package and the command */
>> +		pkg = nd_to_pdsm_cmd_pkg(nd_cmd);
>> +		pdsm = pkg->hdr.nd_command;
>> +
>> +		/* Verify if the psdm command is valid */
>> +		if (pdsm <= PAPR_PDSM_MIN || pdsm >= PAPR_PDSM_MAX) {
>> +			dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Invalid PDSM\n", pdsm);
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* We except a payload with all PDSM commands */
>> +		if (!pdsm_cmd_to_payload(pkg)) {
>> +			dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Empty payload\n", pdsm);
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* Ensure reserved fields of the pdsm header are set to 0 */
>> +		if (pkg->reserved[0] || pkg->reserved[1]) {
>> +			dev_dbg(&p->pdev->dev,
>> +				"PDSM[0x%x]: Invalid reserved field usage\n", pdsm);
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	/* Let the command be further processed */
>> +	return 0;
>> +}
>> +
>> +/*
>> + * For a given pdsm request call an appropriate service function.
>> + * Returns errors if any while handling the pdsm command package.
>> + */
>> +static int papr_scm_service_pdsm(struct papr_scm_priv *p,
>> +				 struct nd_pdsm_cmd_pkg *pkg)
>> +{
>> +	const enum papr_pdsm pdsm = pkg->hdr.nd_command;
>> +
>> +	dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Servicing..\n", pdsm);
>> +
>> +	/* Call pdsm service function */
>> +	switch (pdsm) {
>> +	default:
>> +		dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Unsupported PDSM request\n",
>> +			pdsm);
>> +		pkg->cmd_status = -ENOENT;
>> +		break;
>> +	}
>> +
>> +	return pkg->cmd_status;
>> +}
>> +
>>  static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
>>  			  struct nvdimm *nvdimm, unsigned int cmd, void *buf,
>>  			  unsigned int buf_len, int *cmd_rc)
>>  {
>>  	struct nd_cmd_get_config_size *get_size_hdr;
>> +	struct nd_pdsm_cmd_pkg *call_pkg = NULL;
>>  	struct papr_scm_priv *p;
>>  	int rc;
>>  
>> -	/* Only dimm-specific calls are supported atm */
>> -	if (!nvdimm)
>> -		return -EINVAL;
>> +	rc = is_cmd_valid(nvdimm, cmd, buf, buf_len);
>> +	if (rc) {
>> +		pr_debug("Invalid cmd=0x%x. Err=%d\n", cmd, rc);
>> +		return rc;
>> +	}
>>  
>>  	/* Use a local variable in case cmd_rc pointer is NULL */
>>  	if (!cmd_rc)
>> @@ -385,6 +498,11 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
>>  		*cmd_rc = papr_scm_meta_set(p, buf);
>>  		break;
>>  
>> +	case ND_CMD_CALL:
>> +		call_pkg = nd_to_pdsm_cmd_pkg(buf);
>> +		*cmd_rc = papr_scm_service_pdsm(p, call_pkg);
>> +		break;
>> +
>>  	default:
>>  		dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
>>  		return -EINVAL;
>> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
>> index de5d90212409..0e09dc5cec19 100644
>> --- a/include/uapi/linux/ndctl.h
>> +++ b/include/uapi/linux/ndctl.h
>> @@ -244,6 +244,7 @@ struct nd_cmd_pkg {
>>  #define NVDIMM_FAMILY_HPE2 2
>>  #define NVDIMM_FAMILY_MSFT 3
>>  #define NVDIMM_FAMILY_HYPERV 4
>> +#define NVDIMM_FAMILY_PAPR 5
>>  
>>  #define ND_IOCTL_CALL			_IOWR(ND_IOCTL, ND_CMD_CALL,\
>>  					struct nd_cmd_pkg)
>> -- 
>> 2.26.2
>>

Dan Williams June 9, 2020, 12:46 a.m. UTC | #4

On Mon, Jun 8, 2020 at 5:16 PM kernel test robot <[e-mail]> wrote:
>
> Hi Vaibhav,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on powerpc/next]
> [also build test WARNING on linus/master v5.7 next-20200605]
> [cannot apply to linux-nvdimm/libnvdimm-for-next scottwood/next]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url:    https://github.com/0day-ci/linux/commits/Vaibhav-Jain/powerpc-papr_scm-Add-support-for-reporting-nvdimm-health/20200607-211653
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> config: powerpc-randconfig-r016-20200607 (attached as .config)
> compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install powerpc cross compiling tool for clang build
>         # apt-get install binutils-powerpc-linux-gnu
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[e-mail]>
>
> All warnings (new ones prefixed by >>, old ones prefixed by <<):
>
> In file included from <built-in>:1:
> >> ./usr/include/asm/papr_pdsm.h:69:20: warning: field 'hdr' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
> struct nd_cmd_pkg hdr;  /* Package header containing sub-cmd */

Hi Vaibhav,

This looks like it's going to need another round to get this fixed. I
don't think 'struct nd_pdsm_cmd_pkg' should embed a definition of
'struct nd_cmd_pkg'. An instance of 'struct nd_cmd_pkg' carries a
payload that is the 'pdsm' specifics. As the code has it now it's
defined as a superset of 'struct nd_cmd_pkg' and the compiler warning
is pointing out a real 'struct' organization problem.

Given the soak time needed in -next after the code is finalized this
there's no time to do another round of updates and still make the v5.8
merge window.

Vaibhav Jain June 9, 2020, 5:53 p.m. UTC | #5

Thanks Dan for the consideration and taking time to look into this.

My responses below:

Dan Williams <[e-mail]> writes:

> On Mon, Jun 8, 2020 at 5:16 PM kernel test robot <[e-mail]> wrote:
>>
>> Hi Vaibhav,
>>
>> Thank you for the patch! Perhaps something to improve:
>>
>> [auto build test WARNING on powerpc/next]
>> [also build test WARNING on linus/master v5.7 next-20200605]
>> [cannot apply to linux-nvdimm/libnvdimm-for-next scottwood/next]
>> [if your patch is applied to the wrong git tree, please drop us a note to help
>> improve the system. BTW, we also suggest to use '--base' option to specify the
>> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>>
>> url:    https://github.com/0day-ci/linux/commits/Vaibhav-Jain/powerpc-papr_scm-Add-support-for-reporting-nvdimm-health/20200607-211653
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
>> config: powerpc-randconfig-r016-20200607 (attached as .config)
>> compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766)
>> reproduce (this is a W=1 build):
>>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>>         chmod +x ~/bin/make.cross
>>         # install powerpc cross compiling tool for clang build
>>         # apt-get install binutils-powerpc-linux-gnu
>>         # save the attached .config to linux build tree
>>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <[e-mail]>
>>
>> All warnings (new ones prefixed by >>, old ones prefixed by <<):
>>
>> In file included from <built-in>:1:
>> >> ./usr/include/asm/papr_pdsm.h:69:20: warning: field 'hdr' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>> struct nd_cmd_pkg hdr;  /* Package header containing sub-cmd */
>
> Hi Vaibhav,
>
[.]
> This looks like it's going to need another round to get this fixed. I
> don't think 'struct nd_pdsm_cmd_pkg' should embed a definition of
> 'struct nd_cmd_pkg'. An instance of 'struct nd_cmd_pkg' carries a
> payload that is the 'pdsm' specifics. As the code has it now it's
> defined as a superset of 'struct nd_cmd_pkg' and the compiler warning
> is pointing out a real 'struct' organization problem.
>
> Given the soak time needed in -next after the code is finalized this
> there's no time to do another round of updates and still make the v5.8
> merge window.

Agreed that this looks bad, a solution will probably need some more
review cycles resulting in this series missing the merge window.

I am investigating into the possible solutions for this reported issue
and made few observations:

I see command pkg for Intel, Hpe, Msft and Hyperv families using a
similar layout of embedding nd_cmd_pkg at the head of the
command-pkg. struct nd_pdsm_cmd_pkg is following the same pattern.

struct nd_pdsm_cmd_pkg {
    struct nd_cmd_pkg hdr;
    /* other members */
};

struct ndn_pkg_msft {
    struct nd_cmd_pkg gen;
    /* other members */
};
struct nd_pkg_intel {
    struct nd_cmd_pkg gen;
    /* other members */
};
struct ndn_pkg_hpe1 {
    struct nd_cmd_pkg gen;
    /* other members */
};

Even though other command families implement similar command-package
layout they were not flagged (yet) as they are (I am guessing) serviced
in vendor acpi drivers rather than in kernel like in case of papr-scm
command family.

So, I think this issue is not just specific to papr-scm command family
introduced in this patch series but rather across all other command
families. Every other command family assumes 'struct nd_cmd_pkg_hdr' to
be embeddable and puts it at the beginning of their corresponding
command-packages. And its only a matter of time when someone tries
filtering/handling of vendor specific commands in nfit module when they
hit similar issue.

Possible Solutions:

* One way would be to redefine 'struct nd_cmd_pkg' to mark field
  'nd_payload[]' from a flexible array to zero sized array as
  'nd_payload[0]'. This should make 'struct nd_cmd_pkg' embeddable and
  clang shouldn't report 'gnu-variable-sized-type-not-at-end'
  warning. Also I think this change shouldn't introduce any ABI change.
  
* Another way to solve this issue might be to redefine 'struct
  nd_pdsm_cmd_pkg' to below removing the 'struct nd_cmd_pkg' member. This
  struct should immediately follow the 'struct nd_cmd_pkg' command package
  when sent to libnvdimm:

  struct nd_pdsm_cmd_pkg {
	__s32 cmd_status;	/* Out: Sub-cmd status returned back */
	__u16 reserved[2];	/* Ignored and to be used in future */
        __u8 payload[];
        };

  This should remove the flexible member nc_cmd_pkg.nd_payload from the
  struct with just one remaining at the end. However this would make
  accessing the [in|out|fw]_size members of 'struct nd_cmd_pkg'
  difficult for the pdsm servicing functions.


Any other solution that you think, may solve this issue ?

Thanks,

Dan Williams June 9, 2020, 6:53 p.m. UTC | #6

On Tue, Jun 9, 2020 at 10:54 AM Vaibhav Jain <[e-mail]> wrote:
>
> Thanks Dan for the consideration and taking time to look into this.
>
> My responses below:
>
> Dan Williams <[e-mail]> writes:
>
> > On Mon, Jun 8, 2020 at 5:16 PM kernel test robot <[e-mail]> wrote:
> >>
> >> Hi Vaibhav,
> >>
> >> Thank you for the patch! Perhaps something to improve:
> >>
> >> [auto build test WARNING on powerpc/next]
> >> [also build test WARNING on linus/master v5.7 next-20200605]
> >> [cannot apply to linux-nvdimm/libnvdimm-for-next scottwood/next]
> >> [if your patch is applied to the wrong git tree, please drop us a note to help
> >> improve the system. BTW, we also suggest to use '--base' option to specify the
> >> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> >>
> >> url:    https://github.com/0day-ci/linux/commits/Vaibhav-Jain/powerpc-papr_scm-Add-support-for-reporting-nvdimm-health/20200607-211653
> >> base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> >> config: powerpc-randconfig-r016-20200607 (attached as .config)
> >> compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766)
> >> reproduce (this is a W=1 build):
> >>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >>         chmod +x ~/bin/make.cross
> >>         # install powerpc cross compiling tool for clang build
> >>         # apt-get install binutils-powerpc-linux-gnu
> >>         # save the attached .config to linux build tree
> >>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
> >>
> >> If you fix the issue, kindly add following tag as appropriate
> >> Reported-by: kernel test robot <[e-mail]>
> >>
> >> All warnings (new ones prefixed by >>, old ones prefixed by <<):
> >>
> >> In file included from <built-in>:1:
> >> >> ./usr/include/asm/papr_pdsm.h:69:20: warning: field 'hdr' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
> >> struct nd_cmd_pkg hdr;  /* Package header containing sub-cmd */
> >
> > Hi Vaibhav,
> >
> [.]
> > This looks like it's going to need another round to get this fixed. I
> > don't think 'struct nd_pdsm_cmd_pkg' should embed a definition of
> > 'struct nd_cmd_pkg'. An instance of 'struct nd_cmd_pkg' carries a
> > payload that is the 'pdsm' specifics. As the code has it now it's
> > defined as a superset of 'struct nd_cmd_pkg' and the compiler warning
> > is pointing out a real 'struct' organization problem.
> >
> > Given the soak time needed in -next after the code is finalized this
> > there's no time to do another round of updates and still make the v5.8
> > merge window.
>
> Agreed that this looks bad, a solution will probably need some more
> review cycles resulting in this series missing the merge window.
>
> I am investigating into the possible solutions for this reported issue
> and made few observations:
>
> I see command pkg for Intel, Hpe, Msft and Hyperv families using a
> similar layout of embedding nd_cmd_pkg at the head of the
> command-pkg. struct nd_pdsm_cmd_pkg is following the same pattern.
>
> struct nd_pdsm_cmd_pkg {
>     struct nd_cmd_pkg hdr;
>     /* other members */
> };
>
> struct ndn_pkg_msft {
>     struct nd_cmd_pkg gen;
>     /* other members */
> };
> struct nd_pkg_intel {
>     struct nd_cmd_pkg gen;
>     /* other members */
> };
> struct ndn_pkg_hpe1 {
>     struct nd_cmd_pkg gen;
>     /* other members */

In those cases the other members are a union and there is no second
variable length array. Perhaps that is why those definitions are not
getting flagged? I'm not seeing anything in ndctl build options that
would explicitly disable this warning, but I'm not sure if the ndctl
build environment is missing this build warning by accident.

Those variable size payloads are also not being used in any code paths
that would look at the size of the command payload, like the kernel
ioctl() path. The payload validation code needs static sizes and the
payload parsing code wants to cast the payload to a known type. I
don't think you can use the same struct definition for both those
cases which is why the ndctl parsing code uses the union layout, but
the kernel command marshaling code does strict layering.

> };
>
> Even though other command families implement similar command-package
> layout they were not flagged (yet) as they are (I am guessing) serviced
> in vendor acpi drivers rather than in kernel like in case of papr-scm
> command family.

I sincerely hope there are no vendor acpi kernel drivers outside of
the upstream one.

>
> So, I think this issue is not just specific to papr-scm command family
> introduced in this patch series but rather across all other command
> families. Every other command family assumes 'struct nd_cmd_pkg_hdr' to
> be embeddable and puts it at the beginning of their corresponding
> command-packages. And its only a matter of time when someone tries
> filtering/handling of vendor specific commands in nfit module when they
> hit similar issue.
>
> Possible Solutions:
>
> * One way would be to redefine 'struct nd_cmd_pkg' to mark field
>   'nd_payload[]' from a flexible array to zero sized array as
>   'nd_payload[0]'.

I just went through a round of removing the usage of buf[0] in ndctl
since gcc10 now warns about that too.

> This should make 'struct nd_cmd_pkg' embeddable and
>   clang shouldn't report 'gnu-variable-sized-type-not-at-end'
>   warning. Also I think this change shouldn't introduce any ABI change.
>
> * Another way to solve this issue might be to redefine 'struct
>   nd_pdsm_cmd_pkg' to below removing the 'struct nd_cmd_pkg' member. This
>   struct should immediately follow the 'struct nd_cmd_pkg' command package
>   when sent to libnvdimm:
>
>   struct nd_pdsm_cmd_pkg {
>         __s32 cmd_status;       /* Out: Sub-cmd status returned back */
>         __u16 reserved[2];      /* Ignored and to be used in future */
>         __u8 payload[];
>         };
>
>   This should remove the flexible member nc_cmd_pkg.nd_payload from the
>   struct with just one remaining at the end. However this would make
>   accessing the [in|out|fw]_size members of 'struct nd_cmd_pkg'
>   difficult for the pdsm servicing functions.
>
>
> Any other solution that you think, may solve this issue ?

The union might help, but per the above I think only for parsing the
command at which point I don't think the kernel needs a unified
structure defining both the generic envelope and the end-point
specific payload at once.

Vaibhav Jain June 10, 2020, 12:09 p.m. UTC | #7

Dan Williams <[e-mail]> writes:

> On Tue, Jun 9, 2020 at 10:54 AM Vaibhav Jain <[e-mail]> wrote:
>>
>> Thanks Dan for the consideration and taking time to look into this.
>>
>> My responses below:
>>
>> Dan Williams <[e-mail]> writes:
>>
>> > On Mon, Jun 8, 2020 at 5:16 PM kernel test robot <[e-mail]> wrote:
>> >>
>> >> Hi Vaibhav,
>> >>
>> >> Thank you for the patch! Perhaps something to improve:
>> >>
>> >> [auto build test WARNING on powerpc/next]
>> >> [also build test WARNING on linus/master v5.7 next-20200605]
>> >> [cannot apply to linux-nvdimm/libnvdimm-for-next scottwood/next]
>> >> [if your patch is applied to the wrong git tree, please drop us a note to help
>> >> improve the system. BTW, we also suggest to use '--base' option to specify the
>> >> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>> >>
>> >> url:    https://github.com/0day-ci/linux/commits/Vaibhav-Jain/powerpc-papr_scm-Add-support-for-reporting-nvdimm-health/20200607-211653
>> >> base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
>> >> config: powerpc-randconfig-r016-20200607 (attached as .config)
>> >> compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766)
>> >> reproduce (this is a W=1 build):
>> >>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>> >>         chmod +x ~/bin/make.cross
>> >>         # install powerpc cross compiling tool for clang build
>> >>         # apt-get install binutils-powerpc-linux-gnu
>> >>         # save the attached .config to linux build tree
>> >>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
>> >>
>> >> If you fix the issue, kindly add following tag as appropriate
>> >> Reported-by: kernel test robot <[e-mail]>
>> >>
>> >> All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> >>
>> >> In file included from <built-in>:1:
>> >> >> ./usr/include/asm/papr_pdsm.h:69:20: warning: field 'hdr' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>> >> struct nd_cmd_pkg hdr;  /* Package header containing sub-cmd */
>> >
>> > Hi Vaibhav,
>> >
>> [.]
>> > This looks like it's going to need another round to get this fixed. I
>> > don't think 'struct nd_pdsm_cmd_pkg' should embed a definition of
>> > 'struct nd_cmd_pkg'. An instance of 'struct nd_cmd_pkg' carries a
>> > payload that is the 'pdsm' specifics. As the code has it now it's
>> > defined as a superset of 'struct nd_cmd_pkg' and the compiler warning
>> > is pointing out a real 'struct' organization problem.
>> >
>> > Given the soak time needed in -next after the code is finalized this
>> > there's no time to do another round of updates and still make the v5.8
>> > merge window.
>>
>> Agreed that this looks bad, a solution will probably need some more
>> review cycles resulting in this series missing the merge window.
>>
>> I am investigating into the possible solutions for this reported issue
>> and made few observations:
>>
>> I see command pkg for Intel, Hpe, Msft and Hyperv families using a
>> similar layout of embedding nd_cmd_pkg at the head of the
>> command-pkg. struct nd_pdsm_cmd_pkg is following the same pattern.
>>
>> struct nd_pdsm_cmd_pkg {
>>     struct nd_cmd_pkg hdr;
>>     /* other members */
>> };
>>
>> struct ndn_pkg_msft {
>>     struct nd_cmd_pkg gen;
>>     /* other members */
>> };
>> struct nd_pkg_intel {
>>     struct nd_cmd_pkg gen;
>>     /* other members */
>> };
>> struct ndn_pkg_hpe1 {
>>     struct nd_cmd_pkg gen;
>>     /* other members */
[.]
>
> In those cases the other members are a union and there is no second
> variable length array. Perhaps that is why those definitions are not
> getting flagged? I'm not seeing anything in ndctl build options that
> would explicitly disable this warning, but I'm not sure if the ndctl
> build environment is missing this build warning by accident.

I tried building ndctl master with clang-10 with CC=clang and
CFLAGS="". Seeing the same warning messages reported for all command
package struct for existing command families.

./hpe1.h:334:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
        struct nd_cmd_pkg gen;
                          ^
./msft.h:59:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
        struct nd_cmd_pkg       gen;
                                ^
./hyperv.h:34:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
        struct nd_cmd_pkg       gen;
                                ^
>
> Those variable size payloads are also not being used in any code paths
> that would look at the size of the command payload, like the kernel
> ioctl() path. The payload validation code needs static sizes and the
> payload parsing code wants to cast the payload to a known type. I
> don't think you can use the same struct definition for both those
> cases which is why the ndctl parsing code uses the union layout, but
> the kernel command marshaling code does strict layering.
Even if I switch to union layout and replacing the flexible array 'payload'
at end to a fixed size array something like below, I still see
'-Wgnu-variable-sized-type-not-at-end' warning reported by clang:

union nd_pdsm_cmd_payload {
	struct nd_papr_pdsm_health health;
	__u8 buf[ND_PDSM_PAYLOAD_MAX_SIZE];
};

struct nd_pdsm_cmd_pkg {
	struct nd_cmd_pkg hdr;	/* Package header containing sub-cmd */
    	__s32 cmd_status;	/* Out: Sub-cmd status returned back */
	__u16 reserved[2];	/* Ignored and to be used in future */
	union nd_pdsm_cmd_payload payload;
} __attribute__((packed));


>
>> };
>>
>> Even though other command families implement similar command-package
>> layout they were not flagged (yet) as they are (I am guessing) serviced
>> in vendor acpi drivers rather than in kernel like in case of papr-scm
>> command family.
>
> I sincerely hope there are no vendor acpi kernel drivers outside of
> the upstream one.
Apologies if I was not clear. Was referring to nvdimm vendor uefi
drivers which ultimately service the DSM commands. Since CMD_CALL serves
as a conduit to send the command payload to these vendor drivers,
libnvdimm never needs to peek into the nd_cmd_pkg.payload
field. Consequently nfit module never hit this warning in kernel before.

>
>>
>> So, I think this issue is not just specific to papr-scm command family
>> introduced in this patch series but rather across all other command
>> families. Every other command family assumes 'struct nd_cmd_pkg_hdr' to
>> be embeddable and puts it at the beginning of their corresponding
>> command-packages. And its only a matter of time when someone tries
>> filtering/handling of vendor specific commands in nfit module when they
>> hit similar issue.
>>
>> Possible Solutions:
>>
>> * One way would be to redefine 'struct nd_cmd_pkg' to mark field
>>   'nd_payload[]' from a flexible array to zero sized array as
>>   'nd_payload[0]'.
>
> I just went through a round of removing the usage of buf[0] in ndctl
> since gcc10 now warns about that too.
>
>> This should make 'struct nd_cmd_pkg' embeddable and
>>   clang shouldn't report 'gnu-variable-sized-type-not-at-end'
>>   warning. Also I think this change shouldn't introduce any ABI change.
>>
>> * Another way to solve this issue might be to redefine 'struct
>>   nd_pdsm_cmd_pkg' to below removing the 'struct nd_cmd_pkg' member. This
>>   struct should immediately follow the 'struct nd_cmd_pkg' command package
>>   when sent to libnvdimm:
>>
>>   struct nd_pdsm_cmd_pkg {
>>         __s32 cmd_status;       /* Out: Sub-cmd status returned back */
>>         __u16 reserved[2];      /* Ignored and to be used in future */
>>         __u8 payload[];
>>         };
>>
>>   This should remove the flexible member nc_cmd_pkg.nd_payload from the
>>   struct with just one remaining at the end. However this would make
>>   accessing the [in|out|fw]_size members of 'struct nd_cmd_pkg'
>>   difficult for the pdsm servicing functions.
>>
>>
>> Any other solution that you think, may solve this issue ?
>
> The union might help, but per the above I think only for parsing the
> command at which point I don't think the kernel needs a unified
> structure defining both the generic envelope and the end-point
> specific payload at once.

As I tested above, switching to union too will not solve the clang
warning.

Having a unified structure for a command family defining both
generic envelop and end-point specific payload, is what I see all the
existing command families doing.

However if I split 'struct nd_pdsm_cmd_pkg' to not have an embedded
'struct nd_cmd_pkg' then it goes opposite to what existing command family
implementations.

So to me it looks like no clear way to address this :-(

Another non-conventional way to fix this might be to suppress this clang
warning messages by adding "CFLAGS_papr_scm.o +=
-Wno-gnu-variable-sized-type-not-at-end" to papr_scm Makefile.

Current implementation 'struct nd_cmd_pkg' clearly depends on gcc
extension of having a flexible payload array which is allowed to be not
necessarily placed at the end of a containing struct. So the problem can be
attributed to difference in compiler implementations between GCC and
Clang rather than how 'struct nd_pdsm_cmd_pkg' and 'struct nd_cmd_pkg'
are laid out.

Dan Williams June 10, 2020, 3:11 p.m. UTC | #8

On Wed, Jun 10, 2020 at 5:10 AM Vaibhav Jain <[e-mail]> wrote:
>
> Dan Williams <[e-mail]> writes:
>
> > On Tue, Jun 9, 2020 at 10:54 AM Vaibhav Jain <[e-mail]> wrote:
> >>
> >> Thanks Dan for the consideration and taking time to look into this.
> >>
> >> My responses below:
> >>
> >> Dan Williams <[e-mail]> writes:
> >>
> >> > On Mon, Jun 8, 2020 at 5:16 PM kernel test robot <[e-mail]> wrote:
> >> >>
> >> >> Hi Vaibhav,
> >> >>
> >> >> Thank you for the patch! Perhaps something to improve:
> >> >>
> >> >> [auto build test WARNING on powerpc/next]
> >> >> [also build test WARNING on linus/master v5.7 next-20200605]
> >> >> [cannot apply to linux-nvdimm/libnvdimm-for-next scottwood/next]
> >> >> [if your patch is applied to the wrong git tree, please drop us a note to help
> >> >> improve the system. BTW, we also suggest to use '--base' option to specify the
> >> >> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> >> >>
> >> >> url:    https://github.com/0day-ci/linux/commits/Vaibhav-Jain/powerpc-papr_scm-Add-support-for-reporting-nvdimm-health/20200607-211653
> >> >> base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> >> >> config: powerpc-randconfig-r016-20200607 (attached as .config)
> >> >> compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766)
> >> >> reproduce (this is a W=1 build):
> >> >>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >> >>         chmod +x ~/bin/make.cross
> >> >>         # install powerpc cross compiling tool for clang build
> >> >>         # apt-get install binutils-powerpc-linux-gnu
> >> >>         # save the attached .config to linux build tree
> >> >>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
> >> >>
> >> >> If you fix the issue, kindly add following tag as appropriate
> >> >> Reported-by: kernel test robot <[e-mail]>
> >> >>
> >> >> All warnings (new ones prefixed by >>, old ones prefixed by <<):
> >> >>
> >> >> In file included from <built-in>:1:
> >> >> >> ./usr/include/asm/papr_pdsm.h:69:20: warning: field 'hdr' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
> >> >> struct nd_cmd_pkg hdr;  /* Package header containing sub-cmd */
> >> >
> >> > Hi Vaibhav,
> >> >
> >> [.]
> >> > This looks like it's going to need another round to get this fixed. I
> >> > don't think 'struct nd_pdsm_cmd_pkg' should embed a definition of
> >> > 'struct nd_cmd_pkg'. An instance of 'struct nd_cmd_pkg' carries a
> >> > payload that is the 'pdsm' specifics. As the code has it now it's
> >> > defined as a superset of 'struct nd_cmd_pkg' and the compiler warning
> >> > is pointing out a real 'struct' organization problem.
> >> >
> >> > Given the soak time needed in -next after the code is finalized this
> >> > there's no time to do another round of updates and still make the v5.8
> >> > merge window.
> >>
> >> Agreed that this looks bad, a solution will probably need some more
> >> review cycles resulting in this series missing the merge window.
> >>
> >> I am investigating into the possible solutions for this reported issue
> >> and made few observations:
> >>
> >> I see command pkg for Intel, Hpe, Msft and Hyperv families using a
> >> similar layout of embedding nd_cmd_pkg at the head of the
> >> command-pkg. struct nd_pdsm_cmd_pkg is following the same pattern.
> >>
> >> struct nd_pdsm_cmd_pkg {
> >>     struct nd_cmd_pkg hdr;
> >>     /* other members */
> >> };
> >>
> >> struct ndn_pkg_msft {
> >>     struct nd_cmd_pkg gen;
> >>     /* other members */
> >> };
> >> struct nd_pkg_intel {
> >>     struct nd_cmd_pkg gen;
> >>     /* other members */
> >> };
> >> struct ndn_pkg_hpe1 {
> >>     struct nd_cmd_pkg gen;
> >>     /* other members */
> [.]
> >
> > In those cases the other members are a union and there is no second
> > variable length array. Perhaps that is why those definitions are not
> > getting flagged? I'm not seeing anything in ndctl build options that
> > would explicitly disable this warning, but I'm not sure if the ndctl
> > build environment is missing this build warning by accident.
>
> I tried building ndctl master with clang-10 with CC=clang and
> CFLAGS="". Seeing the same warning messages reported for all command
> package struct for existing command families.
>
> ./hpe1.h:334:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>         struct nd_cmd_pkg gen;
>                           ^
> ./msft.h:59:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>         struct nd_cmd_pkg       gen;
>                                 ^
> ./hyperv.h:34:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>         struct nd_cmd_pkg       gen;
>                                 ^

Good to know, but ugh now I'm just realizing this warning is only
coming from clang and not gcc. Frankly I'm not as concerned about
clang warnings and I should have been more careful looking at the
source of this warning.

> >
> > Those variable size payloads are also not being used in any code paths
> > that would look at the size of the command payload, like the kernel
> > ioctl() path. The payload validation code needs static sizes and the
> > payload parsing code wants to cast the payload to a known type. I
> > don't think you can use the same struct definition for both those
> > cases which is why the ndctl parsing code uses the union layout, but
> > the kernel command marshaling code does strict layering.
> Even if I switch to union layout and replacing the flexible array 'payload'
> at end to a fixed size array something like below, I still see
> '-Wgnu-variable-sized-type-not-at-end' warning reported by clang:
>
> union nd_pdsm_cmd_payload {
>         struct nd_papr_pdsm_health health;
>         __u8 buf[ND_PDSM_PAYLOAD_MAX_SIZE];
> };
>
> struct nd_pdsm_cmd_pkg {
>         struct nd_cmd_pkg hdr;  /* Package header containing sub-cmd */
>         __s32 cmd_status;       /* Out: Sub-cmd status returned back */
>         __u16 reserved[2];      /* Ignored and to be used in future */
>         union nd_pdsm_cmd_payload payload;
> } __attribute__((packed));

Even though this is a clang warning, I'm still not sure it's a good
idea to copy the ndctl approach into the kernel. Could you perhaps
handle this the way that drivers/acpi/nfit/intel.c handles submitting
commands through the ND_CMD_CALL interface, i.e. by just defining the
command locally like this (from intel_security_flags()):

        struct {
                struct nd_cmd_pkg pkg;
                struct nd_intel_get_security_state cmd;
        } nd_cmd = {
                .pkg = {
                        .nd_command = NVDIMM_INTEL_GET_SECURITY_STATE,
                        .nd_family = NVDIMM_FAMILY_INTEL,
                        .nd_size_out =
                                sizeof(struct nd_intel_get_security_state),
                        .nd_fw_size =
                                sizeof(struct nd_intel_get_security_state),
                },
        };

That way it's clear that the payload is 'struct
nd_intel_get_security_state' without needing to have a pre-existing
definition. For parsing in the ioctl path I think it's clearer to cast
the payload to the local pdsm structure for the command.

>
>
> >
> >> };
> >>
> >> Even though other command families implement similar command-package
> >> layout they were not flagged (yet) as they are (I am guessing) serviced
> >> in vendor acpi drivers rather than in kernel like in case of papr-scm
> >> command family.
> >
> > I sincerely hope there are no vendor acpi kernel drivers outside of
> > the upstream one.
> Apologies if I was not clear. Was referring to nvdimm vendor uefi
> drivers which ultimately service the DSM commands. Since CMD_CALL serves
> as a conduit to send the command payload to these vendor drivers,
> libnvdimm never needs to peek into the nd_cmd_pkg.payload
> field. Consequently nfit module never hit this warning in kernel before.

Ah, understood, and no, that's not the root reason this problem is not
present in the kernel. The expectation is that any payload that the
kernel would need to consider should probably have a kernel specific
translation defined. For example,

        ND_CMD_GET_CONFIG_SIZE
        ND_CMD_GET_CONFIG_DATA
        ND_CMD_SET_CONFIG_DATA

...are payloads that the kernel needs to understand. However instead
of supporting each way to read / write the label area the expectation
is that all drivers just parse this common kernel payload and
translate it if necessary. For example ND_CMD_{GET,SET}_CONFIG_DATA is
optionally translated to the Intel DSMs, generic ACPI _LSR/_LSW, or
papr_scm_meta_{get,set}.

Outside of validating command numbers the expectation is that the
kernel does not validate/consume the contents of the ND_CMD_CALL
payload, it passes it to the backend where ACPI DSM or pdsm protocol
takes over.

>
> >
> >>
> >> So, I think this issue is not just specific to papr-scm command family
> >> introduced in this patch series but rather across all other command
> >> families. Every other command family assumes 'struct nd_cmd_pkg_hdr' to
> >> be embeddable and puts it at the beginning of their corresponding
> >> command-packages. And its only a matter of time when someone tries
> >> filtering/handling of vendor specific commands in nfit module when they
> >> hit similar issue.
> >>
> >> Possible Solutions:
> >>
> >> * One way would be to redefine 'struct nd_cmd_pkg' to mark field
> >>   'nd_payload[]' from a flexible array to zero sized array as
> >>   'nd_payload[0]'.
> >
> > I just went through a round of removing the usage of buf[0] in ndctl
> > since gcc10 now warns about that too.
> >
> >> This should make 'struct nd_cmd_pkg' embeddable and
> >>   clang shouldn't report 'gnu-variable-sized-type-not-at-end'
> >>   warning. Also I think this change shouldn't introduce any ABI change.
> >>
> >> * Another way to solve this issue might be to redefine 'struct
> >>   nd_pdsm_cmd_pkg' to below removing the 'struct nd_cmd_pkg' member. This
> >>   struct should immediately follow the 'struct nd_cmd_pkg' command package
> >>   when sent to libnvdimm:
> >>
> >>   struct nd_pdsm_cmd_pkg {
> >>         __s32 cmd_status;       /* Out: Sub-cmd status returned back */
> >>         __u16 reserved[2];      /* Ignored and to be used in future */
> >>         __u8 payload[];
> >>         };
> >>
> >>   This should remove the flexible member nc_cmd_pkg.nd_payload from the
> >>   struct with just one remaining at the end. However this would make
> >>   accessing the [in|out|fw]_size members of 'struct nd_cmd_pkg'
> >>   difficult for the pdsm servicing functions.
> >>
> >>
> >> Any other solution that you think, may solve this issue ?
> >
> > The union might help, but per the above I think only for parsing the
> > command at which point I don't think the kernel needs a unified
> > structure defining both the generic envelope and the end-point
> > specific payload at once.
>
> As I tested above, switching to union too will not solve the clang
> warning.
>
> Having a unified structure for a command family defining both
> generic envelop and end-point specific payload, is what I see all the
> existing command families doing.
>
> However if I split 'struct nd_pdsm_cmd_pkg' to not have an embedded
> 'struct nd_cmd_pkg' then it goes opposite to what existing command family
> implementations.
>
> So to me it looks like no clear way to address this :-(
>
> Another non-conventional way to fix this might be to suppress this clang
> warning messages by adding "CFLAGS_papr_scm.o +=
> -Wno-gnu-variable-sized-type-not-at-end" to papr_scm Makefile.

No, I don't think it's appropriate to customize clang warnings. Have a
look at splitting parsing vs local command submission following the
approach taken in drivers/acpi/nfit/intel.c.

> Current implementation 'struct nd_cmd_pkg' clearly depends on gcc
> extension of having a flexible payload array which is allowed to be not
> necessarily placed at the end of a containing struct. So the problem can be
> attributed to difference in compiler implementations between GCC and
> Clang rather than how 'struct nd_pdsm_cmd_pkg' and 'struct nd_cmd_pkg'
> are laid out.
>
> --
> Cheers
> ~ Vaibhav

Vaibhav Jain June 11, 2020, 6:03 p.m. UTC | #9

Dan Williams <[e-mail]> writes:

> On Wed, Jun 10, 2020 at 5:10 AM Vaibhav Jain <[e-mail]> wrote:
>>
>> Dan Williams <[e-mail]> writes:
>>
>> > On Tue, Jun 9, 2020 at 10:54 AM Vaibhav Jain <[e-mail]> wrote:
>> >>
>> >> Thanks Dan for the consideration and taking time to look into this.
>> >>
>> >> My responses below:
>> >>
>> >> Dan Williams <[e-mail]> writes:
>> >>
>> >> > On Mon, Jun 8, 2020 at 5:16 PM kernel test robot <[e-mail]> wrote:
>> >> >>
>> >> >> Hi Vaibhav,
>> >> >>
>> >> >> Thank you for the patch! Perhaps something to improve:
>> >> >>
>> >> >> [auto build test WARNING on powerpc/next]
>> >> >> [also build test WARNING on linus/master v5.7 next-20200605]
>> >> >> [cannot apply to linux-nvdimm/libnvdimm-for-next scottwood/next]
>> >> >> [if your patch is applied to the wrong git tree, please drop us a note to help
>> >> >> improve the system. BTW, we also suggest to use '--base' option to specify the
>> >> >> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>> >> >>
>> >> >> url:    https://github.com/0day-ci/linux/commits/Vaibhav-Jain/powerpc-papr_scm-Add-support-for-reporting-nvdimm-health/20200607-211653
>> >> >> base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
>> >> >> config: powerpc-randconfig-r016-20200607 (attached as .config)
>> >> >> compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766)
>> >> >> reproduce (this is a W=1 build):
>> >> >>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>> >> >>         chmod +x ~/bin/make.cross
>> >> >>         # install powerpc cross compiling tool for clang build
>> >> >>         # apt-get install binutils-powerpc-linux-gnu
>> >> >>         # save the attached .config to linux build tree
>> >> >>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
>> >> >>
>> >> >> If you fix the issue, kindly add following tag as appropriate
>> >> >> Reported-by: kernel test robot <[e-mail]>
>> >> >>
>> >> >> All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> >> >>
>> >> >> In file included from <built-in>:1:
>> >> >> >> ./usr/include/asm/papr_pdsm.h:69:20: warning: field 'hdr' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>> >> >> struct nd_cmd_pkg hdr;  /* Package header containing sub-cmd */
>> >> >
>> >> > Hi Vaibhav,
>> >> >
>> >> [.]
>> >> > This looks like it's going to need another round to get this fixed. I
>> >> > don't think 'struct nd_pdsm_cmd_pkg' should embed a definition of
>> >> > 'struct nd_cmd_pkg'. An instance of 'struct nd_cmd_pkg' carries a
>> >> > payload that is the 'pdsm' specifics. As the code has it now it's
>> >> > defined as a superset of 'struct nd_cmd_pkg' and the compiler warning
>> >> > is pointing out a real 'struct' organization problem.
>> >> >
>> >> > Given the soak time needed in -next after the code is finalized this
>> >> > there's no time to do another round of updates and still make the v5.8
>> >> > merge window.
>> >>
>> >> Agreed that this looks bad, a solution will probably need some more
>> >> review cycles resulting in this series missing the merge window.
>> >>
>> >> I am investigating into the possible solutions for this reported issue
>> >> and made few observations:
>> >>
>> >> I see command pkg for Intel, Hpe, Msft and Hyperv families using a
>> >> similar layout of embedding nd_cmd_pkg at the head of the
>> >> command-pkg. struct nd_pdsm_cmd_pkg is following the same pattern.
>> >>
>> >> struct nd_pdsm_cmd_pkg {
>> >>     struct nd_cmd_pkg hdr;
>> >>     /* other members */
>> >> };
>> >>
>> >> struct ndn_pkg_msft {
>> >>     struct nd_cmd_pkg gen;
>> >>     /* other members */
>> >> };
>> >> struct nd_pkg_intel {
>> >>     struct nd_cmd_pkg gen;
>> >>     /* other members */
>> >> };
>> >> struct ndn_pkg_hpe1 {
>> >>     struct nd_cmd_pkg gen;
>> >>     /* other members */
>> [.]
>> >
>> > In those cases the other members are a union and there is no second
>> > variable length array. Perhaps that is why those definitions are not
>> > getting flagged? I'm not seeing anything in ndctl build options that
>> > would explicitly disable this warning, but I'm not sure if the ndctl
>> > build environment is missing this build warning by accident.
>>
>> I tried building ndctl master with clang-10 with CC=clang and
>> CFLAGS="". Seeing the same warning messages reported for all command
>> package struct for existing command families.
>>
>> ./hpe1.h:334:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>>         struct nd_cmd_pkg gen;
>>                           ^
>> ./msft.h:59:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>>         struct nd_cmd_pkg       gen;
>>                                 ^
>> ./hyperv.h:34:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>>         struct nd_cmd_pkg       gen;
>>                                 ^
>
[.]
> Good to know, but ugh now I'm just realizing this warning is only
> coming from clang and not gcc. Frankly I'm not as concerned about
> clang warnings and I should have been more careful looking at the
> source of this warning.
Thanks for acknowledging this.

I digged deeper into this today and it seems that with clang, kernel code
is compiled with diagnostic flag '-Wno-gnu' [1][2] which implicitly implies
'-Wno-gnu-variable-sized-type-not-at-end'. Hence the structures with
flexible arrays not the end of containing struct are not flagged in
kernel code.

[1] https://github.com/torvalds/linux/blob/b29482fde649c72441d5478a4ea2c52c56d97a5e/Makefile#L788
[2] https://clang.llvm.org/docs/DiagnosticsReference.html#wgnu

However this dignostic flag is not used for uapi header test hence
build robot emmited this warning while trying to test compile
'papr_pdsm.h' uapi header.

>
>> >
>> > Those variable size payloads are also not being used in any code paths
>> > that would look at the size of the command payload, like the kernel
>> > ioctl() path. The payload validation code needs static sizes and the
>> > payload parsing code wants to cast the payload to a known type. I
>> > don't think you can use the same struct definition for both those
>> > cases which is why the ndctl parsing code uses the union layout, but
>> > the kernel command marshaling code does strict layering.
>> Even if I switch to union layout and replacing the flexible array 'payload'
>> at end to a fixed size array something like below, I still see
>> '-Wgnu-variable-sized-type-not-at-end' warning reported by clang:
>>
>> union nd_pdsm_cmd_payload {
>>         struct nd_papr_pdsm_health health;
>>         __u8 buf[ND_PDSM_PAYLOAD_MAX_SIZE];
>> };
>>
>> struct nd_pdsm_cmd_pkg {
>>         struct nd_cmd_pkg hdr;  /* Package header containing sub-cmd */
>>         __s32 cmd_status;       /* Out: Sub-cmd status returned back */
>>         __u16 reserved[2];      /* Ignored and to be used in future */
>>         union nd_pdsm_cmd_payload payload;
>> } __attribute__((packed));
>
> Even though this is a clang warning, I'm still not sure it's a good
> idea to copy the ndctl approach into the kernel. Could you perhaps
> handle this the way that drivers/acpi/nfit/intel.c handles submitting
> commands through the ND_CMD_CALL interface, i.e. by just defining the
> command locally like this (from intel_security_flags()):
>
>         struct {
>                 struct nd_cmd_pkg pkg;
>                 struct nd_intel_get_security_state cmd;
>         } nd_cmd = {
>                 .pkg = {
>                         .nd_command = NVDIMM_INTEL_GET_SECURITY_STATE,
>                         .nd_family = NVDIMM_FAMILY_INTEL,
>                         .nd_size_out =
>                                 sizeof(struct nd_intel_get_security_state),
>                         .nd_fw_size =
>                                 sizeof(struct nd_intel_get_security_state),
>                 },
>         };
>
> That way it's clear that the payload is 'struct
> nd_intel_get_security_state' without needing to have a pre-existing
> definition. For parsing in the ioctl path I think it's clearer to cast
> the payload to the local pdsm structure for the command.
>
In userspace libndctl code doesnt use '-Wno-gnu' (yet) hence this would
still be reported as a warning. Also for each pdsm I want a consistent
way to report errors back. Above would force me to define a 'status'
field to report error in every pdsm payload struct.

I have two possible solutions to work around the clang and 'status'
field issue:

1. I remove instance of 'struct nd_cmd_pkg' from 'nd_pdsm_cmd_pkg' like
below. This should make the clang warning go away and I still keep the
'cmd_status' field.

struct nd_pdsm_cmd_pkg {
 	__s32 cmd_status;	/* Out: Sub-cmd status returned back */
 	__u16 reserved[2];	/* Ignored and to be used in future */
 	__u8 payload[];		/* In/Out: Sub-cmd data buffer */
} __packed;

When sending CMD_CALL allocate and populate an envelop large enough to
hold generic nd_cmd header, pdsm header and the payload like below:

0             64                72                            255
+------------+-----------------+--------------------------------+
|nd_cmd_pkg  | nd_pdsm_cmd_pkg |    payload                     |
+------------+-----------------+--------------------------------+

pdsm handling code introduced in this patchset already uses helpers to
convert nd_cmd_pkg -> nd_pdsm_cmd_pkg and nd_pdsm_cmd_pkg -> payload. So
the impact to the patchset should be contained to these helper
functions. There are places in pdsm service functions that directly
access members of nd_cmd_pkg which may need some tweaking.


2. I open-code members of 'struct nd_cmd_pkg' at start of 'struct
nd_pdsm_cmd_pkg' except the nd_payload field like below. This struct
should ensure ABI compatibility with 'struct nd_cmd_pkg'.

struct nd_pdsm_cmd_pkg {
	__u64   nd_family;		/* family of commands */
	__u64   nd_command;
	__u32   nd_size_in;		/* INPUT: size of input args */
	__u32   nd_size_out;		/* INPUT: size of payload */
	__u32   nd_reserved2[9];	/* reserved must be zero */
	__u32   nd_fw_size;		/* OUTPUT: size fw wants to return */
 	__s32 cmd_status;	/* Out: Sub-cmd status returned back */
 	__u16 reserved[2];	/* Ignored and to be used in future */
 	__u8 payload[];		/* In/Out: Sub-cmd data buffer */
 } __packed;

BULD_BUG_ON((sizeof(struct nd_cmd_pkg) + 8) > sizeof(struct nd_pdsm_cmd_pkg))

>>
>>
>> >
>> >> };
>> >>
>> >> Even though other command families implement similar command-package
>> >> layout they were not flagged (yet) as they are (I am guessing) serviced
>> >> in vendor acpi drivers rather than in kernel like in case of papr-scm
>> >> command family.
>> >
>> > I sincerely hope there are no vendor acpi kernel drivers outside of
>> > the upstream one.
>> Apologies if I was not clear. Was referring to nvdimm vendor uefi
>> drivers which ultimately service the DSM commands. Since CMD_CALL serves
>> as a conduit to send the command payload to these vendor drivers,
>> libnvdimm never needs to peek into the nd_cmd_pkg.payload
>> field. Consequently nfit module never hit this warning in kernel before.
>
> Ah, understood, and no, that's not the root reason this problem is not
> present in the kernel. The expectation is that any payload that the
> kernel would need to consider should probably have a kernel specific
> translation defined. For example,
>
>         ND_CMD_GET_CONFIG_SIZE
>         ND_CMD_GET_CONFIG_DATA
>         ND_CMD_SET_CONFIG_DATA
>
> ...are payloads that the kernel needs to understand. However instead
> of supporting each way to read / write the label area the expectation
> is that all drivers just parse this common kernel payload and
> translate it if necessary. For example ND_CMD_{GET,SET}_CONFIG_DATA is
> optionally translated to the Intel DSMs, generic ACPI _LSR/_LSW, or
> papr_scm_meta_{get,set}.
>
> Outside of validating command numbers the expectation is that the
> kernel does not validate/consume the contents of the ND_CMD_CALL
> payload, it passes it to the backend where ACPI DSM or pdsm protocol
> takes over.
Right, but arent those independent IOCTLs to libnvdimm with a fixed
predefined struct thats exchanged with libndctl. Not sure how can that
help with exchanging pdsms with papr_scm that are variable in length and
can only rely on CMD_CALL ioctl.

Patch

diff --git a/arch/powerpc/include/uapi/asm/papr_pdsm.h b/arch/powerpc/include/uapi/asm/papr_pdsm.h
new file mode 100644
index 000000000000..df2447455cfe
--- /dev/null
+++ b/arch/powerpc/include/uapi/asm/papr_pdsm.h
@@ -0,0 +1,84 @@ 
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * PAPR nvDimm Specific Methods (PDSM) and structs for libndctl
+ *
+ * (C) Copyright IBM 2020
+ *
+ * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
+ */
+
+#ifndef _UAPI_ASM_POWERPC_PAPR_PDSM_H_
+#define _UAPI_ASM_POWERPC_PAPR_PDSM_H_
+
+#include <linux/types.h>
+#include <linux/ndctl.h>
+
+/*
+ * PDSM Envelope:
+ *
+ * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
+ * envelope which consists of a header and user-defined payload sections.
+ * The header is described by 'struct nd_pdsm_cmd_pkg' which expects a
+ * payload following it and accessible via 'nd_pdsm_cmd_pkg.payload' field.
+ * There is reserved field that can used to introduce new fields to the
+ * structure in future. It also tries to ensure that 'nd_pdsm_cmd_pkg.payload'
+ * lies at a 8-byte boundary.
+ *
+ *  +-------------+---------------------+---------------------------+
+ *  |   64-Bytes  |       8-Bytes       |       Max 184-Bytes       |
+ *  +-------------+---------------------+---------------------------+
+ *  |               nd_pdsm_cmd_pkg     |                           |
+ *  |-------------+                     |                           |
+ *  |  nd_cmd_pkg |                     |                           |
+ *  +-------------+---------------------+---------------------------+
+ *  | nd_family   |                     |                           |
+ *  | nd_size_out | cmd_status          |                           |
+ *  | nd_size_in  | reserved            |     payload               |
+ *  | nd_command  |                     |                           |
+ *  | nd_fw_size  |                     |                           |
+ *  +-------------+---------------------+---------------------------+
+ *
+ * PDSM Header:
+ *
+ * The header is defined as 'struct nd_pdsm_cmd_pkg' which embeds a
+ * 'struct nd_cmd_pkg' instance. The PDSM command is assigned to member
+ * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelope which is
+ * contained in 'struct nd_cmd_pkg', the header also has members following
+ * members:
+ *
+ * 'cmd_status'		: (Out) Errors if any encountered while servicing PDSM.
+ * 'reserved'		: Not used, reserved for future and should be set to 0.
+ *
+ * PDSM Payload:
+ *
+ * The layout of the PDSM Payload is defined by various structs shared between
+ * papr_scm and libndctl so that contents of payload can be interpreted. During
+ * servicing of a PDSM the papr_scm module will read input args from the payload
+ * field by casting its contents to an appropriate struct pointer based on the
+ * PDSM command. Similarly the output of servicing the PDSM command will be
+ * copied to the payload field using the same struct.
+ *
+ * 'libnvdimm' enforces a hard limit of 256 bytes on the envelope size, which
+ * leaves around 184 bytes for the envelope payload (ignoring any padding that
+ * the compiler may silently introduce).
+ *
+ */
+
+/* PDSM-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
+struct nd_pdsm_cmd_pkg {
+	struct nd_cmd_pkg hdr;	/* Package header containing sub-cmd */
+	__s32 cmd_status;	/* Out: Sub-cmd status returned back */
+	__u16 reserved[2];	/* Ignored and to be used in future */
+	__u8 payload[];		/* In/Out: Sub-cmd data buffer */
+} __packed;
+
+/*
+ * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
+ * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
+ */
+enum papr_pdsm {
+	PAPR_PDSM_MIN = 0x0,
+	PAPR_PDSM_MAX,
+};
+
+#endif /* _UAPI_ASM_POWERPC_PAPR_PDSM_H_ */
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 692ad3d79826..167fcf0e249d 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -15,13 +15,15 @@ 
 #include <linux/seq_buf.h>
 
 #include <asm/plpar_wrappers.h>
+#include <asm/papr_pdsm.h>
 
 #define BIND_ANY_ADDR (~0ul)
 
 #define PAPR_SCM_DIMM_CMD_MASK \
 	((1ul << ND_CMD_GET_CONFIG_SIZE) | \
 	 (1ul << ND_CMD_GET_CONFIG_DATA) | \
-	 (1ul << ND_CMD_SET_CONFIG_DATA))
+	 (1ul << ND_CMD_SET_CONFIG_DATA) | \
+	 (1ul << ND_CMD_CALL))
 
 /* DIMM health bitmap bitmap indicators */
 /* SCM device is unable to persist memory contents */
@@ -89,6 +91,21 @@  struct papr_scm_priv {
 	u64 health_bitmap;
 };
 
+/* Convert a libnvdimm nd_cmd_pkg to pdsm specific pkg */
+static inline struct nd_pdsm_cmd_pkg *nd_to_pdsm_cmd_pkg(struct nd_cmd_pkg *cmd)
+{
+	return (struct nd_pdsm_cmd_pkg *)cmd;
+}
+
+/* Return the payload pointer for a given pcmd */
+static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
+{
+	if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
+		return NULL;
+	else
+		return (void *)(pcmd->payload);
+}
+
 static int drc_pmem_bind(struct papr_scm_priv *p)
 {
 	unsigned long ret[PLPAR_HCALL_BUFSIZE];
@@ -349,17 +366,113 @@  static int papr_scm_meta_set(struct papr_scm_priv *p,
 	return 0;
 }
 
+/*
+ * Do a sanity checks on the inputs args to dimm-control function and return
+ * '0' if valid. This also does validation on ND_CMD_CALL sub-command packages.
+ */
+static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
+			unsigned int buf_len)
+{
+	unsigned long cmd_mask = PAPR_SCM_DIMM_CMD_MASK;
+	struct nd_pdsm_cmd_pkg *pkg;
+	struct nd_cmd_pkg *nd_cmd;
+	struct papr_scm_priv *p;
+	enum papr_pdsm pdsm;
+
+	/* Only dimm-specific calls are supported atm */
+	if (!nvdimm)
+		return -EINVAL;
+
+	/* get the provider data from struct nvdimm */
+	p = nvdimm_provider_data(nvdimm);
+
+	if (!test_bit(cmd, &cmd_mask)) {
+		dev_dbg(&p->pdev->dev, "Unsupported cmd=%u\n", cmd);
+		return -EINVAL;
+	}
+
+	/* For CMD_CALL verify pdsm request */
+	if (cmd == ND_CMD_CALL) {
+		/* Verify the envelope package */
+		if (!buf || buf_len < sizeof(struct nd_pdsm_cmd_pkg)) {
+			dev_dbg(&p->pdev->dev, "Invalid pkg size=%u\n",
+				buf_len);
+			return -EINVAL;
+		}
+
+		/* Verify that the nd_cmd_pkg.nd_family is correct */
+		nd_cmd = (struct nd_cmd_pkg *)buf;
+		if (nd_cmd->nd_family != NVDIMM_FAMILY_PAPR) {
+			dev_dbg(&p->pdev->dev, "Invalid pkg family=0x%llx\n",
+				nd_cmd->nd_family);
+			return -EINVAL;
+		}
+
+		/* Get the pdsm request package and the command */
+		pkg = nd_to_pdsm_cmd_pkg(nd_cmd);
+		pdsm = pkg->hdr.nd_command;
+
+		/* Verify if the psdm command is valid */
+		if (pdsm <= PAPR_PDSM_MIN || pdsm >= PAPR_PDSM_MAX) {
+			dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Invalid PDSM\n", pdsm);
+			return -EINVAL;
+		}
+
+		/* We except a payload with all PDSM commands */
+		if (!pdsm_cmd_to_payload(pkg)) {
+			dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Empty payload\n", pdsm);
+			return -EINVAL;
+		}
+
+		/* Ensure reserved fields of the pdsm header are set to 0 */
+		if (pkg->reserved[0] || pkg->reserved[1]) {
+			dev_dbg(&p->pdev->dev,
+				"PDSM[0x%x]: Invalid reserved field usage\n", pdsm);
+			return -EINVAL;
+		}
+	}
+
+	/* Let the command be further processed */
+	return 0;
+}
+
+/*
+ * For a given pdsm request call an appropriate service function.
+ * Returns errors if any while handling the pdsm command package.
+ */
+static int papr_scm_service_pdsm(struct papr_scm_priv *p,
+				 struct nd_pdsm_cmd_pkg *pkg)
+{
+	const enum papr_pdsm pdsm = pkg->hdr.nd_command;
+
+	dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Servicing..\n", pdsm);
+
+	/* Call pdsm service function */
+	switch (pdsm) {
+	default:
+		dev_dbg(&p->pdev->dev, "PDSM[0x%x]: Unsupported PDSM request\n",
+			pdsm);
+		pkg->cmd_status = -ENOENT;
+		break;
+	}
+
+	return pkg->cmd_status;
+}
+
 static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
 			  struct nvdimm *nvdimm, unsigned int cmd, void *buf,
 			  unsigned int buf_len, int *cmd_rc)
 {
 	struct nd_cmd_get_config_size *get_size_hdr;
+	struct nd_pdsm_cmd_pkg *call_pkg = NULL;
 	struct papr_scm_priv *p;
 	int rc;
 
-	/* Only dimm-specific calls are supported atm */
-	if (!nvdimm)
-		return -EINVAL;
+	rc = is_cmd_valid(nvdimm, cmd, buf, buf_len);
+	if (rc) {
+		pr_debug("Invalid cmd=0x%x. Err=%d\n", cmd, rc);
+		return rc;
+	}
 
 	/* Use a local variable in case cmd_rc pointer is NULL */
 	if (!cmd_rc)
@@ -385,6 +498,11 @@  static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
 		*cmd_rc = papr_scm_meta_set(p, buf);
 		break;
 
+	case ND_CMD_CALL:
+		call_pkg = nd_to_pdsm_cmd_pkg(buf);
+		*cmd_rc = papr_scm_service_pdsm(p, call_pkg);
+		break;
+
 	default:
 		dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
 		return -EINVAL;
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index de5d90212409..0e09dc5cec19 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -244,6 +244,7 @@  struct nd_cmd_pkg {
 #define NVDIMM_FAMILY_HPE2 2
 #define NVDIMM_FAMILY_MSFT 3
 #define NVDIMM_FAMILY_HYPERV 4
+#define NVDIMM_FAMILY_PAPR 5
 
 #define ND_IOCTL_CALL			_IOWR(ND_IOCTL, ND_CMD_CALL,\
 					struct nd_cmd_pkg)