| Message ID | 150393513271.10003.253995897844648312.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 28, 2017 at 08:45:32AM -0700, Dan Williams wrote: > Remove the command payloads that do not have an associated libnvdimm > ioctl. I.e. remove the payloads that would only ever be carried in the > ND_CMD_CALL envelope. This prevents userspace from growing unnecessary > dependencies on this kernel header when userspace already has everything > it needs to craft and send these commands. Userspace needs to include linux/ndctl.h to make the call as that is where nd_cmd_pkg is defined. So you want to have some structures defined in ndctl.h and other defined in the to be created libndctl-nfit.h? Plus a third header file for the HPE non-root calls? Will libndctl-nfit.h be generally available and installed? Will it be clean so that other applications can use it to get these definitions? Or will it be loaded w/ a bunch of stuff only useful to your ndctl command? > > Cc: Jerry Hoemann <jerry.hoemann@hpe.com> > Reported-by: Yasunori Goto <y-goto@jp.fujitsu.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > include/uapi/linux/ndctl.h | 37 ------------------------------------- > 1 file changed, 37 deletions(-) > > diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h > index 6d3c54264d8e..3f03567631cb 100644 > --- a/include/uapi/linux/ndctl.h > +++ b/include/uapi/linux/ndctl.h > @@ -145,43 +145,6 @@ struct nd_cmd_clear_error { > __u64 cleared; > } __packed; > > -struct nd_cmd_trans_spa { > - __u64 spa; > - __u32 status; > - __u8 flags; > - __u8 _reserved[3]; > - __u64 trans_length; > - __u32 num_nvdimms; > - struct nd_nvdimm_device { > - __u32 nfit_device_handle; > - __u32 _reserved; > - __u64 dpa; > - } __packed devices[0]; > - > -} __packed; > - > -struct nd_cmd_ars_err_inj { > - __u64 err_inj_spa_range_base; > - __u64 err_inj_spa_range_length; > - __u8 err_inj_options; > - __u32 status; > -} __packed; > - > -struct nd_cmd_ars_err_inj_clr { > - __u64 err_inj_clr_spa_range_base; > - __u64 err_inj_clr_spa_range_length; > - __u32 status; > -} __packed; > - > -struct nd_cmd_ars_err_inj_stat { > - __u32 status; > - __u32 inj_err_rec_count; > - struct nd_error_stat_query_record { > - __u64 err_inj_stat_spa_range_base; > - __u64 err_inj_stat_spa_range_length; > - } __packed record[0]; > -} __packed; > - > enum { > ND_CMD_IMPLEMENTED = 0, >
On Mon, Aug 28, 2017 at 1:50 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > > On Mon, Aug 28, 2017 at 08:45:32AM -0700, Dan Williams wrote: >> Remove the command payloads that do not have an associated libnvdimm >> ioctl. I.e. remove the payloads that would only ever be carried in the >> ND_CMD_CALL envelope. This prevents userspace from growing unnecessary >> dependencies on this kernel header when userspace already has everything >> it needs to craft and send these commands. > > Userspace needs to include linux/ndctl.h to make the call as > that is where nd_cmd_pkg is defined. > > So you want to have some structures defined in ndctl.h and other > defined in the to be created libndctl-nfit.h? Plus a third header > file for the HPE non-root calls? Yes. ndctl.h exports the ioctl command payloads, everything that goes inside of ND_CMD_CALL is defined by userspace headers. The libndctl-nfit.h header is proposed as a place to land vendor agnostic NFIT-defined payloads, and any vendor specific definitions would remain internal to libndctl as they are today. > Will libndctl-nfit.h be generally available and installed? Yes, that's the plan. > Will it be clean so that other applications can use it to get these > definitions? Or will it be loaded w/ a bunch of stuff only useful > to your ndctl command? Yes, that's the plan. It's a bug if libndctl-nfit.h is not generically clean for issuing the NFIT root device commands via some ND_CMD_CALL helpers from the base libndctl library. In other words libndctl-nfit.h defines the payload and libndctl defines some general helpers for issuing commands.
> On Mon, Aug 28, 2017 at 1:50 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > > > > On Mon, Aug 28, 2017 at 08:45:32AM -0700, Dan Williams wrote: > >> Remove the command payloads that do not have an associated libnvdimm > >> ioctl. I.e. remove the payloads that would only ever be carried in the > >> ND_CMD_CALL envelope. This prevents userspace from growing unnecessary > >> dependencies on this kernel header when userspace already has everything > >> it needs to craft and send these commands. > > > > Userspace needs to include linux/ndctl.h to make the call as > > that is where nd_cmd_pkg is defined. > > > > So you want to have some structures defined in ndctl.h and other > > defined in the to be created libndctl-nfit.h? Plus a third header > > file for the HPE non-root calls? > > Yes. ndctl.h exports the ioctl command payloads, everything that goes > inside of ND_CMD_CALL is defined by userspace headers. The > libndctl-nfit.h header is proposed as a place to land vendor agnostic > NFIT-defined payloads, and any vendor specific definitions would > remain internal to libndctl as they are today. > > > Will libndctl-nfit.h be generally available and installed? > > Yes, that's the plan. > > > Will it be clean so that other applications can use it to get these > > definitions? Or will it be loaded w/ a bunch of stuff only useful > > to your ndctl command? > > Yes, that's the plan. It's a bug if libndctl-nfit.h is not generically > clean for issuing the NFIT root device commands via some ND_CMD_CALL > helpers from the base libndctl library. > > In other words libndctl-nfit.h defines the payload and libndctl > defines some general helpers for issuing commands. Maybe I don't understand your idea yet, let me confirm it. Certainly, current acpi driver does not need these definitions. But, I think nfit_test.ko will need them to emulate these features. Do you intend that libndctl-nfit.h should be defined at "include/uapi/linux/" directory? Otherwise, it should be defined at "tools/testing/nvdimm/" or "tools/testing/nvdimm/test" ? Thanks, --- Yasunori Goto
On Mon, Aug 28, 2017 at 6:03 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: >> On Mon, Aug 28, 2017 at 1:50 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >> > >> > On Mon, Aug 28, 2017 at 08:45:32AM -0700, Dan Williams wrote: >> >> Remove the command payloads that do not have an associated libnvdimm >> >> ioctl. I.e. remove the payloads that would only ever be carried in the >> >> ND_CMD_CALL envelope. This prevents userspace from growing unnecessary >> >> dependencies on this kernel header when userspace already has everything >> >> it needs to craft and send these commands. >> > >> > Userspace needs to include linux/ndctl.h to make the call as >> > that is where nd_cmd_pkg is defined. >> > >> > So you want to have some structures defined in ndctl.h and other >> > defined in the to be created libndctl-nfit.h? Plus a third header >> > file for the HPE non-root calls? >> >> Yes. ndctl.h exports the ioctl command payloads, everything that goes >> inside of ND_CMD_CALL is defined by userspace headers. The >> libndctl-nfit.h header is proposed as a place to land vendor agnostic >> NFIT-defined payloads, and any vendor specific definitions would >> remain internal to libndctl as they are today. >> >> > Will libndctl-nfit.h be generally available and installed? >> >> Yes, that's the plan. >> >> > Will it be clean so that other applications can use it to get these >> > definitions? Or will it be loaded w/ a bunch of stuff only useful >> > to your ndctl command? >> >> Yes, that's the plan. It's a bug if libndctl-nfit.h is not generically >> clean for issuing the NFIT root device commands via some ND_CMD_CALL >> helpers from the base libndctl library. >> >> In other words libndctl-nfit.h defines the payload and libndctl >> defines some general helpers for issuing commands. > > Maybe I don't understand your idea yet, let me confirm it. > > Certainly, current acpi driver does not need these definitions. > But, I think nfit_test.ko will need them to emulate these features. > > Do you intend that libndctl-nfit.h should be defined at "include/uapi/linux/" > directory? > Otherwise, it should be defined at "tools/testing/nvdimm/" or > "tools/testing/nvdimm/test" ? nfit_test will need its own internal / private copy of these payloads in tools/testing/nvdimm/test so it can emulate how the bios behaves. The include/uapi/linux directory is for user to kernel interface definitions and these command payloads are purely an interface to bios / firmware.
> On Mon, Aug 28, 2017 at 6:03 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: > >> On Mon, Aug 28, 2017 at 1:50 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > >> > > >> > On Mon, Aug 28, 2017 at 08:45:32AM -0700, Dan Williams wrote: > >> >> Remove the command payloads that do not have an associated libnvdimm > >> >> ioctl. I.e. remove the payloads that would only ever be carried in the > >> >> ND_CMD_CALL envelope. This prevents userspace from growing unnecessary > >> >> dependencies on this kernel header when userspace already has everything > >> >> it needs to craft and send these commands. > >> > > >> > Userspace needs to include linux/ndctl.h to make the call as > >> > that is where nd_cmd_pkg is defined. > >> > > >> > So you want to have some structures defined in ndctl.h and other > >> > defined in the to be created libndctl-nfit.h? Plus a third header > >> > file for the HPE non-root calls? > >> > >> Yes. ndctl.h exports the ioctl command payloads, everything that goes > >> inside of ND_CMD_CALL is defined by userspace headers. The > >> libndctl-nfit.h header is proposed as a place to land vendor agnostic > >> NFIT-defined payloads, and any vendor specific definitions would > >> remain internal to libndctl as they are today. > >> > >> > Will libndctl-nfit.h be generally available and installed? > >> > >> Yes, that's the plan. > >> > >> > Will it be clean so that other applications can use it to get these > >> > definitions? Or will it be loaded w/ a bunch of stuff only useful > >> > to your ndctl command? > >> > >> Yes, that's the plan. It's a bug if libndctl-nfit.h is not generically > >> clean for issuing the NFIT root device commands via some ND_CMD_CALL > >> helpers from the base libndctl library. > >> > >> In other words libndctl-nfit.h defines the payload and libndctl > >> defines some general helpers for issuing commands. > > > > Maybe I don't understand your idea yet, let me confirm it. > > > > Certainly, current acpi driver does not need these definitions. > > But, I think nfit_test.ko will need them to emulate these features. > > > > Do you intend that libndctl-nfit.h should be defined at "include/uapi/linux/" > > directory? > > Otherwise, it should be defined at "tools/testing/nvdimm/" or > > "tools/testing/nvdimm/test" ? > > nfit_test will need its own internal / private copy of these payloads > in tools/testing/nvdimm/test so it can emulate how the bios behaves. > The include/uapi/linux directory is for user to kernel interface > definitions and these command payloads are purely an interface to bios > / firmware. > Hmm, I'm trying to make libndctl-nfit.h under tools/testing/nvdimm/nfit/, and trying to move NFIT_CMD_TRANSLATE_SPA from drivers/acpi/nfit/core.c to it. But I noticed that drivers/acpi/nfit/core.c still uses the definition in acpi_nfit_init_dsms(). ---- static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) { struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc; const guid_t *guid = to_nfit_uuid(NFIT_DEV_BUS); : dsm_mask = (1 << ND_CMD_ARS_CAP) | (1 << ND_CMD_ARS_START) | (1 << ND_CMD_ARS_STATUS) | (1 << ND_CMD_CLEAR_ERROR) | (1 << NFIT_CMD_TRANSLATE_SPA) | (1 << NFIT_CMD_ARS_INJECT_SET) | (1 << NFIT_CMD_ARS_INJECT_CLEAR) | (1 << NFIT_CMD_ARS_INJECT_GET); for_each_set_bit(i, &dsm_mask, BITS_PER_LONG) if (acpi_check_dsm(adev->handle, guid, 1, 1ULL << i)) set_bit(i, &nd_desc->bus_dsm_mask); --- Though it seems to be necessary yet, do you have any idea to remove it? Bye,
On Tue, Aug 29, 2017 at 3:09 AM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: >> On Mon, Aug 28, 2017 at 6:03 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: >> >> On Mon, Aug 28, 2017 at 1:50 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >> >> > >> >> > On Mon, Aug 28, 2017 at 08:45:32AM -0700, Dan Williams wrote: >> >> >> Remove the command payloads that do not have an associated libnvdimm >> >> >> ioctl. I.e. remove the payloads that would only ever be carried in the >> >> >> ND_CMD_CALL envelope. This prevents userspace from growing unnecessary >> >> >> dependencies on this kernel header when userspace already has everything >> >> >> it needs to craft and send these commands. >> >> > >> >> > Userspace needs to include linux/ndctl.h to make the call as >> >> > that is where nd_cmd_pkg is defined. >> >> > >> >> > So you want to have some structures defined in ndctl.h and other >> >> > defined in the to be created libndctl-nfit.h? Plus a third header >> >> > file for the HPE non-root calls? >> >> >> >> Yes. ndctl.h exports the ioctl command payloads, everything that goes >> >> inside of ND_CMD_CALL is defined by userspace headers. The >> >> libndctl-nfit.h header is proposed as a place to land vendor agnostic >> >> NFIT-defined payloads, and any vendor specific definitions would >> >> remain internal to libndctl as they are today. >> >> >> >> > Will libndctl-nfit.h be generally available and installed? >> >> >> >> Yes, that's the plan. >> >> >> >> > Will it be clean so that other applications can use it to get these >> >> > definitions? Or will it be loaded w/ a bunch of stuff only useful >> >> > to your ndctl command? >> >> >> >> Yes, that's the plan. It's a bug if libndctl-nfit.h is not generically >> >> clean for issuing the NFIT root device commands via some ND_CMD_CALL >> >> helpers from the base libndctl library. >> >> >> >> In other words libndctl-nfit.h defines the payload and libndctl >> >> defines some general helpers for issuing commands. >> > >> > Maybe I don't understand your idea yet, let me confirm it. >> > >> > Certainly, current acpi driver does not need these definitions. >> > But, I think nfit_test.ko will need them to emulate these features. >> > >> > Do you intend that libndctl-nfit.h should be defined at "include/uapi/linux/" >> > directory? >> > Otherwise, it should be defined at "tools/testing/nvdimm/" or >> > "tools/testing/nvdimm/test" ? >> >> nfit_test will need its own internal / private copy of these payloads >> in tools/testing/nvdimm/test so it can emulate how the bios behaves. >> The include/uapi/linux directory is for user to kernel interface >> definitions and these command payloads are purely an interface to bios >> / firmware. >> > > Hmm, > > I'm trying to make libndctl-nfit.h under tools/testing/nvdimm/nfit/, > and trying to move NFIT_CMD_TRANSLATE_SPA from drivers/acpi/nfit/core.c to it. > > But I noticed that drivers/acpi/nfit/core.c still uses the definition > in acpi_nfit_init_dsms(). > > ---- > static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) > { > struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc; > const guid_t *guid = to_nfit_uuid(NFIT_DEV_BUS); > > : > > dsm_mask = > (1 << ND_CMD_ARS_CAP) | > (1 << ND_CMD_ARS_START) | > (1 << ND_CMD_ARS_STATUS) | > (1 << ND_CMD_CLEAR_ERROR) | > (1 << NFIT_CMD_TRANSLATE_SPA) | > (1 << NFIT_CMD_ARS_INJECT_SET) | > (1 << NFIT_CMD_ARS_INJECT_CLEAR) | > (1 << NFIT_CMD_ARS_INJECT_GET); > for_each_set_bit(i, &dsm_mask, BITS_PER_LONG) > if (acpi_check_dsm(adev->handle, guid, 1, 1ULL << i)) > set_bit(i, &nd_desc->bus_dsm_mask); > --- > > Though it seems to be necessary yet, > do you have any idea to remove it? Here's the end state I'm looking for... 1/ drivers/acpi/nfit/core.c keeps its current definitions of the NFIT_CMD_ numbers since it needs to validate the DSM command number mask 2/ tools/testing/nvdimm/test/nfit.h grows private definitions of only the command payloads that it emulates 3/ ndctl in userspace grows a new ndctl/libndctl-nfit.h with all the payloads defined in ACPI. Ideally we can deprecate all the payload definitions in ndctl.h and only use the ND_CMD_CALL ioctl plus the ndctl/libndctl-nfit.h definitions. Although, since label reading has moved away from DSMs we'll always need the separate ND_CMD_{GET,SET}_CONFIG_DATA ioctls. The libndctl-nfit.h header should be listed in pkginclude_HEADERS.
> On Tue, Aug 29, 2017 at 3:09 AM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: > >> On Mon, Aug 28, 2017 at 6:03 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: > >> >> On Mon, Aug 28, 2017 at 1:50 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > >> >> > > >> >> > On Mon, Aug 28, 2017 at 08:45:32AM -0700, Dan Williams wrote: > >> >> >> Remove the command payloads that do not have an associated libnvdimm > >> >> >> ioctl. I.e. remove the payloads that would only ever be carried in the > >> >> >> ND_CMD_CALL envelope. This prevents userspace from growing unnecessary > >> >> >> dependencies on this kernel header when userspace already has everything > >> >> >> it needs to craft and send these commands. > >> >> > > >> >> > Userspace needs to include linux/ndctl.h to make the call as > >> >> > that is where nd_cmd_pkg is defined. > >> >> > > >> >> > So you want to have some structures defined in ndctl.h and other > >> >> > defined in the to be created libndctl-nfit.h? Plus a third header > >> >> > file for the HPE non-root calls? > >> >> > >> >> Yes. ndctl.h exports the ioctl command payloads, everything that goes > >> >> inside of ND_CMD_CALL is defined by userspace headers. The > >> >> libndctl-nfit.h header is proposed as a place to land vendor agnostic > >> >> NFIT-defined payloads, and any vendor specific definitions would > >> >> remain internal to libndctl as they are today. > >> >> > >> >> > Will libndctl-nfit.h be generally available and installed? > >> >> > >> >> Yes, that's the plan. > >> >> > >> >> > Will it be clean so that other applications can use it to get these > >> >> > definitions? Or will it be loaded w/ a bunch of stuff only useful > >> >> > to your ndctl command? > >> >> > >> >> Yes, that's the plan. It's a bug if libndctl-nfit.h is not generically > >> >> clean for issuing the NFIT root device commands via some ND_CMD_CALL > >> >> helpers from the base libndctl library. > >> >> > >> >> In other words libndctl-nfit.h defines the payload and libndctl > >> >> defines some general helpers for issuing commands. > >> > > >> > Maybe I don't understand your idea yet, let me confirm it. > >> > > >> > Certainly, current acpi driver does not need these definitions. > >> > But, I think nfit_test.ko will need them to emulate these features. > >> > > >> > Do you intend that libndctl-nfit.h should be defined at "include/uapi/linux/" > >> > directory? > >> > Otherwise, it should be defined at "tools/testing/nvdimm/" or > >> > "tools/testing/nvdimm/test" ? > >> > >> nfit_test will need its own internal / private copy of these payloads > >> in tools/testing/nvdimm/test so it can emulate how the bios behaves. > >> The include/uapi/linux directory is for user to kernel interface > >> definitions and these command payloads are purely an interface to bios > >> / firmware. > >> > > > > Hmm, > > > > I'm trying to make libndctl-nfit.h under tools/testing/nvdimm/nfit/, > > and trying to move NFIT_CMD_TRANSLATE_SPA from drivers/acpi/nfit/core.c to it. > > > > But I noticed that drivers/acpi/nfit/core.c still uses the definition > > in acpi_nfit_init_dsms(). > > > > ---- > > static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) > > { > > struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc; > > const guid_t *guid = to_nfit_uuid(NFIT_DEV_BUS); > > > > : > > > > dsm_mask = > > (1 << ND_CMD_ARS_CAP) | > > (1 << ND_CMD_ARS_START) | > > (1 << ND_CMD_ARS_STATUS) | > > (1 << ND_CMD_CLEAR_ERROR) | > > (1 << NFIT_CMD_TRANSLATE_SPA) | > > (1 << NFIT_CMD_ARS_INJECT_SET) | > > (1 << NFIT_CMD_ARS_INJECT_CLEAR) | > > (1 << NFIT_CMD_ARS_INJECT_GET); > > for_each_set_bit(i, &dsm_mask, BITS_PER_LONG) > > if (acpi_check_dsm(adev->handle, guid, 1, 1ULL << i)) > > set_bit(i, &nd_desc->bus_dsm_mask); > > --- > > > > Though it seems to be necessary yet, > > do you have any idea to remove it? > > Here's the end state I'm looking for... > > 1/ drivers/acpi/nfit/core.c keeps its current definitions of the > NFIT_CMD_ numbers since it needs to validate the DSM command number > mask > > 2/ tools/testing/nvdimm/test/nfit.h grows private definitions of only > the command payloads that it emulates > > 3/ ndctl in userspace grows a new ndctl/libndctl-nfit.h with all the > payloads defined in ACPI. Ideally we can deprecate all the payload > definitions in ndctl.h and only use the ND_CMD_CALL ioctl plus the > ndctl/libndctl-nfit.h definitions. Although, since label reading has > moved away from DSMs we'll always need the separate > ND_CMD_{GET,SET}_CONFIG_DATA ioctls. The libndctl-nfit.h header should > be listed in pkginclude_HEADERS. Hmm, Ok. I'll try. Thanks for your explanation.
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h index 6d3c54264d8e..3f03567631cb 100644 --- a/include/uapi/linux/ndctl.h +++ b/include/uapi/linux/ndctl.h @@ -145,43 +145,6 @@ struct nd_cmd_clear_error { __u64 cleared; } __packed; -struct nd_cmd_trans_spa { - __u64 spa; - __u32 status; - __u8 flags; - __u8 _reserved[3]; - __u64 trans_length; - __u32 num_nvdimms; - struct nd_nvdimm_device { - __u32 nfit_device_handle; - __u32 _reserved; - __u64 dpa; - } __packed devices[0]; - -} __packed; - -struct nd_cmd_ars_err_inj { - __u64 err_inj_spa_range_base; - __u64 err_inj_spa_range_length; - __u8 err_inj_options; - __u32 status; -} __packed; - -struct nd_cmd_ars_err_inj_clr { - __u64 err_inj_clr_spa_range_base; - __u64 err_inj_clr_spa_range_length; - __u32 status; -} __packed; - -struct nd_cmd_ars_err_inj_stat { - __u32 status; - __u32 inj_err_rec_count; - struct nd_error_stat_query_record { - __u64 err_inj_stat_spa_range_base; - __u64 err_inj_stat_spa_range_length; - } __packed record[0]; -} __packed; - enum { ND_CMD_IMPLEMENTED = 0,
Remove the command payloads that do not have an associated libnvdimm ioctl. I.e. remove the payloads that would only ever be carried in the ND_CMD_CALL envelope. This prevents userspace from growing unnecessary dependencies on this kernel header when userspace already has everything it needs to craft and send these commands. Cc: Jerry Hoemann <jerry.hoemann@hpe.com> Reported-by: Yasunori Goto <y-goto@jp.fujitsu.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- include/uapi/linux/ndctl.h | 37 ------------------------------------- 1 file changed, 37 deletions(-)