Re: [RESEND PATCH v7 4/5] ndctl/papr_scm, uapi: Add support for PAPR nvdimm specific methods
Vaibhav Jain writes:
> Hi Ira, Mpe and Aneesh,
>
> Vaibhav Jain writes:
>
>> Michael Ellerman writes:
>>
>>> Ira Weiny writes:
On Wed, May 20, 2020 at 12:30:57AM +0530, Vaibhav Jain wrote:
> Introduce support for Papr nvDimm Specific Methods (PDSM) in papr_scm
> modules and add the command family to the white list of NVDIMM command
> sets. Also advertise support for ND_CMD_CALL for the dimm
> command mask and implement necessary scaffolding in the module to
> handle ND_CMD_CALL ioctl and PDSM requests that we receive.
>>> ...
> + *
> + * Payload Version:
> + *
> + * A 'payload_version' field is present in PDSM header that indicates a
> specific
> + * version of the structure present in PDSM Payload for a given PDSM
> command.
> + * This provides backward compatibility in case the PDSM Payload
> structure
> + * evolves and different structures are supported by 'papr_scm' and
> 'libndctl'.
> + *
> + * When sending a PDSM Payload to 'papr_scm', 'libndctl' should send the
> version
> + * of the payload struct it supports via 'payload_version' field. The
> 'papr_scm'
> + * module when servicing the PDSM envelope checks the 'payload_version'
> and then
> + * uses 'payload struct version' == MIN('payload_version field',
> + * 'max payload-struct-version supported by papr_scm') to service the
> PDSM.
> + * After servicing the PDSM, 'papr_scm' put the negotiated version of
> payload
> + * struct in returned 'payload_version' field.
FWIW many people believe using a size rather than version is more
sustainable.
It is expected that new payload structures are larger (more features) than
the
previous payload structure.
I can't find references at the moment through.
>>>
>>> I think clone_args is a good modern example:
>>>
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/sched.h#n88
>>
>> Thank Ira and Mpe for pointing this out. I looked into how clone3 sycall
>> handles clone_args and few differences came out:
>>
>> * Unlike clone_args that are always transferred in one direction from
>> user-space to kernel, payload contents of pdsms are transferred in both
>> directions. Having a single version number makes it easier for
>> user-space and kernel to determine what data will be exchanged.
>>
>> * For PDSMs, the version number is negotiated between libndctl and
>> kernel. For example in case kernel only supports an older version of
>> a structure, its free to send a lower version number back to
>> libndctl. Such negotiations doesnt happen with clone3 syscall.
>
> If you are ok with the explaination above please let me know. I will
> quickly spin off a v8 addressing your review comments.
I don't have strong opinions about the user API, it's really up to the
nvdimm folks.
cheers
Re: [RESEND PATCH v7 4/5] ndctl/papr_scm, uapi: Add support for PAPR nvdimm specific methods
Hi Ira, Mpe and Aneesh,
Vaibhav Jain writes:
> Michael Ellerman writes:
>
>> Ira Weiny writes:
>>> On Wed, May 20, 2020 at 12:30:57AM +0530, Vaibhav Jain wrote:
Introduce support for Papr nvDimm Specific Methods (PDSM) in papr_scm
modules and add the command family to the white list of NVDIMM command
sets. Also advertise support for ND_CMD_CALL for the dimm
command mask and implement necessary scaffolding in the module to
handle ND_CMD_CALL ioctl and PDSM requests that we receive.
>> ...
+ *
+ * Payload Version:
+ *
+ * A 'payload_version' field is present in PDSM header that indicates a
specific
+ * version of the structure present in PDSM Payload for a given PDSM
command.
+ * This provides backward compatibility in case the PDSM Payload structure
+ * evolves and different structures are supported by 'papr_scm' and
'libndctl'.
+ *
+ * When sending a PDSM Payload to 'papr_scm', 'libndctl' should send the
version
+ * of the payload struct it supports via 'payload_version' field. The
'papr_scm'
+ * module when servicing the PDSM envelope checks the 'payload_version'
and then
+ * uses 'payload struct version' == MIN('payload_version field',
+ * 'max payload-struct-version supported by papr_scm') to service the
PDSM.
+ * After servicing the PDSM, 'papr_scm' put the negotiated version of
payload
+ * struct in returned 'payload_version' field.
>>>
>>> FWIW many people believe using a size rather than version is more
>>> sustainable.
>>> It is expected that new payload structures are larger (more features) than
>>> the
>>> previous payload structure.
>>>
>>> I can't find references at the moment through.
>>
>> I think clone_args is a good modern example:
>>
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/sched.h#n88
>>
>> cheers
>
> Thank Ira and Mpe for pointing this out. I looked into how clone3 sycall
> handles clone_args and few differences came out:
>
> * Unlike clone_args that are always transferred in one direction from
> user-space to kernel, payload contents of pdsms are transferred in both
> directions. Having a single version number makes it easier for
> user-space and kernel to determine what data will be exchanged.
>
> * For PDSMs, the version number is negotiated between libndctl and
> kernel. For example in case kernel only supports an older version of
> a structure, its free to send a lower version number back to
> libndctl. Such negotiations doesnt happen with clone3 syscall.
If you are ok with the explaination above please let me know. I will
quickly spin off a v8 addressing your review comments.
Thanks,
--
Cheers
~ Vaibhav
Re: [RESEND PATCH v7 4/5] ndctl/papr_scm, uapi: Add support for PAPR nvdimm specific methods
Michael Ellerman writes:
> Ira Weiny writes:
>> On Wed, May 20, 2020 at 12:30:57AM +0530, Vaibhav Jain wrote:
>>> Introduce support for Papr nvDimm Specific Methods (PDSM) in papr_scm
>>> modules and add the command family to the white list of NVDIMM command
>>> sets. Also advertise support for ND_CMD_CALL for the dimm
>>> command mask and implement necessary scaffolding in the module to
>>> handle ND_CMD_CALL ioctl and PDSM requests that we receive.
> ...
>>> + *
>>> + * Payload Version:
>>> + *
>>> + * A 'payload_version' field is present in PDSM header that indicates a
>>> specific
>>> + * version of the structure present in PDSM Payload for a given PDSM
>>> command.
>>> + * This provides backward compatibility in case the PDSM Payload structure
>>> + * evolves and different structures are supported by 'papr_scm' and
>>> 'libndctl'.
>>> + *
>>> + * When sending a PDSM Payload to 'papr_scm', 'libndctl' should send the
>>> version
>>> + * of the payload struct it supports via 'payload_version' field. The
>>> 'papr_scm'
>>> + * module when servicing the PDSM envelope checks the 'payload_version'
>>> and then
>>> + * uses 'payload struct version' == MIN('payload_version field',
>>> + * 'max payload-struct-version supported by papr_scm') to service the PDSM.
>>> + * After servicing the PDSM, 'papr_scm' put the negotiated version of
>>> payload
>>> + * struct in returned 'payload_version' field.
>>
>> FWIW many people believe using a size rather than version is more
>> sustainable.
>> It is expected that new payload structures are larger (more features) than
>> the
>> previous payload structure.
>>
>> I can't find references at the moment through.
>
> I think clone_args is a good modern example:
>
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/sched.h#n88
>
> cheers
Thank Ira and Mpe for pointing this out. I looked into how clone3 sycall
handles clone_args and few differences came out:
* Unlike clone_args that are always transferred in one direction from
user-space to kernel, payload contents of pdsms are transferred in both
directions. Having a single version number makes it easier for
user-space and kernel to determine what data will be exchanged.
* For PDSMs, the version number is negotiated between libndctl and
kernel. For example in case kernel only supports an older version of
a structure, its free to send a lower version number back to
libndctl. Such negotiations doesnt happen with clone3 syscall.
--
Cheers
~ Vaibhav
Re: [RESEND PATCH v7 4/5] ndctl/papr_scm, uapi: Add support for PAPR nvdimm specific methods
Ira Weiny writes:
> On Wed, May 20, 2020 at 12:30:57AM +0530, Vaibhav Jain wrote:
>> Introduce support for Papr nvDimm Specific Methods (PDSM) in papr_scm
>> modules and add the command family to the white list of NVDIMM command
>> sets. Also advertise support for ND_CMD_CALL for the dimm
>> command mask and implement necessary scaffolding in the module to
>> handle ND_CMD_CALL ioctl and PDSM requests that we receive.
...
>> + *
>> + * Payload Version:
>> + *
>> + * A 'payload_version' field is present in PDSM header that indicates a
>> specific
>> + * version of the structure present in PDSM Payload for a given PDSM
>> command.
>> + * This provides backward compatibility in case the PDSM Payload structure
>> + * evolves and different structures are supported by 'papr_scm' and
>> 'libndctl'.
>> + *
>> + * When sending a PDSM Payload to 'papr_scm', 'libndctl' should send the
>> version
>> + * of the payload struct it supports via 'payload_version' field. The
>> 'papr_scm'
>> + * module when servicing the PDSM envelope checks the 'payload_version' and
>> then
>> + * uses 'payload struct version' == MIN('payload_version field',
>> + * 'max payload-struct-version supported by papr_scm') to service the PDSM.
>> + * After servicing the PDSM, 'papr_scm' put the negotiated version of
>> payload
>> + * struct in returned 'payload_version' field.
>
> FWIW many people believe using a size rather than version is more sustainable.
> It is expected that new payload structures are larger (more features) than the
> previous payload structure.
>
> I can't find references at the moment through.
I think clone_args is a good modern example:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/sched.h#n88
cheers
Re: [RESEND PATCH v7 4/5] ndctl/papr_scm, uapi: Add support for PAPR nvdimm specific methods
Thanks for reviewing this patch Ira. My responses below: Ira Weiny writes: > On Wed, May 20, 2020 at 12:30:57AM +0530, Vaibhav Jain wrote: >> Introduce support for Papr nvDimm Specific Methods (PDSM) in papr_scm >> modules and add the command family to the white list of NVDIMM command >> sets. Also advertise support for ND_CMD_CALL for the dimm >> 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_scm_pdsm.h' which >> defines a new 'struct nd_pdsm_cmd_pkg' header. This header is used >> to communicate the PDSM request via member >> 'nd_pkg_papr_scm->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: Dan Williams >> Cc: Michael Ellerman >> Cc: "Aneesh Kumar K . V" >> Signed-off-by: Vaibhav Jain >> --- >> Changelog: >> >> 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 0x1 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_scm_pdsm.h | 134 ++ >> arch/powerpc/platforms/pseries/papr_scm.c | 101 - >> include/uapi/linux/ndctl.h| 1 + >> 3 files changed, 230 insertions(+), 6 deletions(-) >> create mode 100644 arch/powerpc/include/uapi/asm/papr_scm_pdsm.h >> >> diff --git a/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h >> b/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h >> new file mode 100644 >> index ..671693439c1c >> --- /dev/null >> +++ b/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h >> @@ -0,0 +1,134 @@ >> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */ >> +/* >> + * PAPR-SCM Dimm specific methods (PDSM) and structs for libndctl >> + * >> + * (C) Copyright IBM 2020 >> + * >> + * Author: Vaibhav Jain >> + */ >> + >> +#ifndef _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_ >> +#define _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_ >> + >> +#include >> + >> +/* >> + * PDSM Envelope: >> + * >> + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via >> + * 'envelopes' 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 offset of which relative to the struct is >> provided >> + * by 'nd_pdsm_cmd_pkg.payload_offset'. * >> + * >> + * +-+-+---+ >> + * | 64-Bytes | 8-Bytes | Max 184-Bytes | >> + * +-+-+---+ >> + * | nd_pdsm_cmd_pkg | | >> + * |-+ | | >> + * | nd_cmd_pkg | | | >> + * +-+-+---+ >> + * | nd_family | | | >> + * | nd_size_out | cmd_status | | >> + * | nd_size_in | payload_version | PAYLOAD | >> + * | nd_command | payload_offset -> | >> + * | nd_fw_size | | | >> + * +-+-+---+ >> + * >> + * PDSM
Re: [RESEND PATCH v7 4/5] ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods
On Wed, May 20, 2020 at 12:30:57AM +0530, Vaibhav Jain wrote: > Introduce support for Papr nvDimm Specific Methods (PDSM) in papr_scm > modules and add the command family to the white list of NVDIMM command > sets. Also advertise support for ND_CMD_CALL for the dimm > 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_scm_pdsm.h' which > defines a new 'struct nd_pdsm_cmd_pkg' header. This header is used > to communicate the PDSM request via member > 'nd_pkg_papr_scm->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: Dan Williams > Cc: Michael Ellerman > Cc: "Aneesh Kumar K . V" > Signed-off-by: Vaibhav Jain > --- > Changelog: > > 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 0x1 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_scm_pdsm.h | 134 ++ > arch/powerpc/platforms/pseries/papr_scm.c | 101 - > include/uapi/linux/ndctl.h| 1 + > 3 files changed, 230 insertions(+), 6 deletions(-) > create mode 100644 arch/powerpc/include/uapi/asm/papr_scm_pdsm.h > > diff --git a/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h > b/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h > new file mode 100644 > index ..671693439c1c > --- /dev/null > +++ b/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h > @@ -0,0 +1,134 @@ > +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */ > +/* > + * PAPR-SCM Dimm specific methods (PDSM) and structs for libndctl > + * > + * (C) Copyright IBM 2020 > + * > + * Author: Vaibhav Jain > + */ > + > +#ifndef _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_ > +#define _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_ > + > +#include > + > +/* > + * PDSM Envelope: > + * > + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via > + * 'envelopes' 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 offset of which relative to the struct is > provided > + * by 'nd_pdsm_cmd_pkg.payload_offset'. * > + * > + * +-+-+---+ > + * | 64-Bytes | 8-Bytes | Max 184-Bytes | > + * +-+-+---+ > + * | nd_pdsm_cmd_pkg | | > + * |-+ | | > + * | nd_cmd_pkg | | | > + * +-+-+---+ > + * | nd_family | | | > + * | nd_size_out | cmd_status || > + * | nd_size_in | payload_version | PAYLOAD | > + * | nd_command | payload_offset ->| > + * | 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
Re: [RESEND PATCH v7 4/5] ndctl/papr_scm, uapi: Add support for PAPR nvdimm specific methods
Thanks for reviewing this patch Aneesh.
"Aneesh Kumar K.V" writes:
> Vaibhav Jain writes:
>
>
>
> +
>> +/* Papr-scm-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 payload_offset; /* In: offset from start of struct */
>> +__u16 payload_version; /* In/Out: version of the payload */
>> +__u8 payload[]; /* In/Out: Sub-cmd data buffer */
>> +} __packed;
>
> that payload_offset can be avoided if we prevent userspace to user a
> different variant of nd_pdsm_cmd_pkg which different header. We can keep
> things simpler if we can always find payload at
> nd_pdsm_cmd_pkg->payload.
Had introduced this member to handle case where new fields are added to
'struct nd_pdsm_cmd_pkg' without having to break the ABI. But agree with
the point that you made later that this can be simplified by replacing
'payload_offset' with a set of reserved variables. Will address this in
next iteration of this patchset.
>
>> +
>> +/*
>> + * 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_scm_pdsm {
>> +PAPR_SCM_PDSM_MIN = 0x0,
>> +PAPR_SCM_PDSM_MAX,
>> +};
>> +
>> +/* 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 *)((__u8 *) pcmd + pcmd->payload_offset);
>> +}
>> +
>
> we need to make sure userspace is not passing a wrong payload_offset.
Agree, that this function should have more strict checking for
payload_offset field. However will be getting rid of
'payload_offset' all together in the next iteration as you previously
suggested.
> and in the next patch you do
>
> + /* Copy the health struct to the payload */
> + memcpy(pdsm_cmd_to_payload(pkg), >health, copysize);
> + pkg->hdr.nd_fw_size = copysize;
> +
Yes this is already being done in the patchset and changes proposed to
this pdsm_cmd_to_payload() should not impact other patches as
pdsm_cmd_to_payload() abstracts rest of the code from how to access the
payload.
> All this can be simplified if you can keep payload at
> nd_pdsm_cmd_pkg->payload.
>
> If you still want to have the ability to extend the header, then added a
> reserved field similar to nd_cmd_pkg.
>
Agree to this and will address this in V8.
>
> -aneesh
--
Cheers
~ Vaibhav
Re: [RESEND PATCH v7 4/5] ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods
Vaibhav Jain writes:
+
> +/* Papr-scm-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 payload_offset; /* In: offset from start of struct */
> + __u16 payload_version; /* In/Out: version of the payload */
> + __u8 payload[]; /* In/Out: Sub-cmd data buffer */
> +} __packed;
that payload_offset can be avoided if we prevent userspace to user a
different variant of nd_pdsm_cmd_pkg which different header. We can keep
things simpler if we can always find payload at
nd_pdsm_cmd_pkg->payload.
> +
> +/*
> + * 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_scm_pdsm {
> + PAPR_SCM_PDSM_MIN = 0x0,
> + PAPR_SCM_PDSM_MAX,
> +};
> +
> +/* 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 *)((__u8 *) pcmd + pcmd->payload_offset);
> +}
> +
we need to make sure userspace is not passing a wrong payload_offset.
and in the next patch you do
+ /* Copy the health struct to the payload */
+ memcpy(pdsm_cmd_to_payload(pkg), >health, copysize);
+ pkg->hdr.nd_fw_size = copysize;
+
All this can be simplified if you can keep payload at
nd_pdsm_cmd_pkg->payload.
If you still want to have the ability to extend the header, then added a
reserved field similar to nd_cmd_pkg.
-aneesh
[RESEND PATCH v7 4/5] ndctl/papr_scm, uapi: Add support for PAPR nvdimm specific methods
Introduce support for Papr nvDimm Specific Methods (PDSM) in papr_scm modules and add the command family to the white list of NVDIMM command sets. Also advertise support for ND_CMD_CALL for the dimm 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_scm_pdsm.h' which defines a new 'struct nd_pdsm_cmd_pkg' header. This header is used to communicate the PDSM request via member 'nd_pkg_papr_scm->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: Dan Williams Cc: Michael Ellerman Cc: "Aneesh Kumar K . V" Signed-off-by: Vaibhav Jain --- Changelog: 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 0x1 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_scm_pdsm.h | 134 ++ arch/powerpc/platforms/pseries/papr_scm.c | 101 - include/uapi/linux/ndctl.h| 1 + 3 files changed, 230 insertions(+), 6 deletions(-) create mode 100644 arch/powerpc/include/uapi/asm/papr_scm_pdsm.h diff --git a/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h b/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h new file mode 100644 index ..671693439c1c --- /dev/null +++ b/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h @@ -0,0 +1,134 @@ +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */ +/* + * PAPR-SCM Dimm specific methods (PDSM) and structs for libndctl + * + * (C) Copyright IBM 2020 + * + * Author: Vaibhav Jain + */ + +#ifndef _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_ +#define _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_ + +#include + +/* + * PDSM Envelope: + * + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via + * 'envelopes' 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 offset of which relative to the struct is provided + * by 'nd_pdsm_cmd_pkg.payload_offset'. * + * + * +-+-+---+ + * | 64-Bytes | 8-Bytes | Max 184-Bytes | + * +-+-+---+ + * | nd_pdsm_cmd_pkg | | + * |-+ | | + * | nd_cmd_pkg | | | + * +-+-+---+ + * | nd_family || | + * | nd_size_out | cmd_status | | + * | nd_size_in | payload_version | PAYLOAD | + * | nd_command | payload_offset -> | + * | 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. + * 'payload_version' : (In/Out) Version number associated with the
