| Message ID | 167105522584.3034751.8329537593759406601.stgit@djiang5-desk3.ch.intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ndctl: Add security test for cxl devices through nvdimm | expand |
Dave Jiang wrote: > Add a CXL bus type, and detect whether a 'dimm' is backed by the CXL > subsystem. > > Reviewed-by: Alison Schofield <alison.schofield@intel.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > --- > v2: > - Improve commit log. (Vishal) > --- > ndctl/lib/libndctl.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ > ndctl/lib/libndctl.sym | 1 + > ndctl/lib/private.h | 1 + > ndctl/libndctl.h | 1 + > 4 files changed, 56 insertions(+) > > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c > index ad54f0626510..10422e24d38b 100644 > --- a/ndctl/lib/libndctl.c > +++ b/ndctl/lib/libndctl.c > @@ -12,6 +12,7 @@ > #include <ctype.h> > #include <fcntl.h> > #include <dirent.h> > +#include <libgen.h> This new include had me looking for why below... > #include <sys/stat.h> > #include <sys/types.h> > #include <sys/ioctl.h> > @@ -876,6 +877,48 @@ static enum ndctl_fwa_method fwa_method_to_method(const char *fwa_method) > return NDCTL_FWA_METHOD_RESET; > } > > +static int is_ndbus_cxl(const char *ctl_base) > +{ > + char *path, *ppath, *subsys; > + char tmp_path[PATH_MAX]; > + int rc; > + > + /* get the real path of ctl_base */ > + path = realpath(ctl_base, NULL); > + if (!path) > + return -errno; > + > + /* setup to get the nd bridge device backing the ctl */ > + sprintf(tmp_path, "%s/device", path); > + free(path); > + > + path = realpath(tmp_path, NULL); > + if (!path) > + return -errno; > + > + /* get the parent dir of the ndbus, which should be the nvdimm-bridge */ > + ppath = dirname(path); > + > + /* setup to get the subsystem of the nvdimm-bridge */ > + sprintf(tmp_path, "%s/%s", ppath, "subsystem"); > + free(path); > + > + path = realpath(tmp_path, NULL); > + if (!path) > + return -errno; > + > + subsys = basename(path); > + > + /* check if subsystem is cxl */ > + if (!strcmp(subsys, "cxl")) > + rc = 1; > + else > + rc = 0; > + > + free(path); > + return rc; > +} > + > static void *add_bus(void *parent, int id, const char *ctl_base) > { > char buf[SYSFS_ATTR_SIZE]; > @@ -919,6 +962,11 @@ static void *add_bus(void *parent, int id, const char *ctl_base) > else > bus->has_of_node = 1; > > + if (is_ndbus_cxl(ctl_base)) > + bus->has_cxl = 1; > + else > + bus->has_cxl = 0; > + I think you can drop is_ndbus_cxl() and just do this: @@ -981,6 +976,11 @@ static void *add_bus(void *parent, int id, const char *ctl_base) if (!bus->provider) goto err_read; + if (strcasestr("cxl", provider)) + bus->has_cxl = 1; + else + bus->has_cxl = 0; + sprintf(path, "%s/device/wait_probe", ctl_base); bus->wait_probe_path = strdup(path); if (!bus->wait_probe_path)
Dan Williams <dan.j.williams@intel.com> writes: > Dave Jiang wrote: >> Add a CXL bus type, and detect whether a 'dimm' is backed by the CXL >> subsystem. >> >> Reviewed-by: Alison Schofield <alison.schofield@intel.com> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> >> --- >> v2: >> - Improve commit log. (Vishal) >> --- >> ndctl/lib/libndctl.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ >> ndctl/lib/libndctl.sym | 1 + >> ndctl/lib/private.h | 1 + >> ndctl/libndctl.h | 1 + >> 4 files changed, 56 insertions(+) >> >> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c >> index ad54f0626510..10422e24d38b 100644 >> --- a/ndctl/lib/libndctl.c >> +++ b/ndctl/lib/libndctl.c >> @@ -12,6 +12,7 @@ >> #include <ctype.h> >> #include <fcntl.h> >> #include <dirent.h> >> +#include <libgen.h> > > This new include had me looking for why below... man 3 basename >> #include <sys/stat.h> >> #include <sys/types.h> >> #include <sys/ioctl.h> >> @@ -876,6 +877,48 @@ static enum ndctl_fwa_method fwa_method_to_method(const char *fwa_method) >> return NDCTL_FWA_METHOD_RESET; >> } >> >> +static int is_ndbus_cxl(const char *ctl_base) >> +{ >> + char *path, *ppath, *subsys; >> + char tmp_path[PATH_MAX]; >> + int rc; >> + >> + /* get the real path of ctl_base */ >> + path = realpath(ctl_base, NULL); >> + if (!path) >> + return -errno; >> + >> + /* setup to get the nd bridge device backing the ctl */ >> + sprintf(tmp_path, "%s/device", path); >> + free(path); >> + >> + path = realpath(tmp_path, NULL); >> + if (!path) >> + return -errno; >> + >> + /* get the parent dir of the ndbus, which should be the nvdimm-bridge */ >> + ppath = dirname(path); >> + >> + /* setup to get the subsystem of the nvdimm-bridge */ >> + sprintf(tmp_path, "%s/%s", ppath, "subsystem"); >> + free(path); >> + >> + path = realpath(tmp_path, NULL); >> + if (!path) >> + return -errno; >> + >> + subsys = basename(path); >> + >> + /* check if subsystem is cxl */ >> + if (!strcmp(subsys, "cxl")) >> + rc = 1; >> + else >> + rc = 0; >> + >> + free(path); >> + return rc; >> +} >> + >> static void *add_bus(void *parent, int id, const char *ctl_base) >> { >> char buf[SYSFS_ATTR_SIZE]; >> @@ -919,6 +962,11 @@ static void *add_bus(void *parent, int id, const char *ctl_base) >> else >> bus->has_of_node = 1; >> >> + if (is_ndbus_cxl(ctl_base)) >> + bus->has_cxl = 1; >> + else >> + bus->has_cxl = 0; >> + > > I think you can drop is_ndbus_cxl() and just do this: > > @@ -981,6 +976,11 @@ static void *add_bus(void *parent, int id, const char *ctl_base) > if (!bus->provider) > goto err_read; > > + if (strcasestr("cxl", provider)) > + bus->has_cxl = 1; > + else > + bus->has_cxl = 0; > + Can you explain why this is preferred? Cheers, Jeff
Jeff Moyer wrote: > Dan Williams <dan.j.williams@intel.com> writes: > > > Dave Jiang wrote: > >> Add a CXL bus type, and detect whether a 'dimm' is backed by the CXL > >> subsystem. > >> > >> Reviewed-by: Alison Schofield <alison.schofield@intel.com> > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > >> > >> --- > >> v2: > >> - Improve commit log. (Vishal) > >> --- > >> ndctl/lib/libndctl.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ > >> ndctl/lib/libndctl.sym | 1 + > >> ndctl/lib/private.h | 1 + > >> ndctl/libndctl.h | 1 + > >> 4 files changed, 56 insertions(+) > >> > >> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c > >> index ad54f0626510..10422e24d38b 100644 > >> --- a/ndctl/lib/libndctl.c > >> +++ b/ndctl/lib/libndctl.c > >> @@ -12,6 +12,7 @@ > >> #include <ctype.h> > >> #include <fcntl.h> > >> #include <dirent.h> > >> +#include <libgen.h> > > > > This new include had me looking for why below... > > man 3 basename > > >> #include <sys/stat.h> > >> #include <sys/types.h> > >> #include <sys/ioctl.h> > >> @@ -876,6 +877,48 @@ static enum ndctl_fwa_method fwa_method_to_method(const char *fwa_method) > >> return NDCTL_FWA_METHOD_RESET; > >> } > >> > >> +static int is_ndbus_cxl(const char *ctl_base) > >> +{ > >> + char *path, *ppath, *subsys; > >> + char tmp_path[PATH_MAX]; > >> + int rc; > >> + > >> + /* get the real path of ctl_base */ > >> + path = realpath(ctl_base, NULL); > >> + if (!path) > >> + return -errno; > >> + > >> + /* setup to get the nd bridge device backing the ctl */ > >> + sprintf(tmp_path, "%s/device", path); > >> + free(path); > >> + > >> + path = realpath(tmp_path, NULL); > >> + if (!path) > >> + return -errno; > >> + > >> + /* get the parent dir of the ndbus, which should be the nvdimm-bridge */ > >> + ppath = dirname(path); > >> + > >> + /* setup to get the subsystem of the nvdimm-bridge */ > >> + sprintf(tmp_path, "%s/%s", ppath, "subsystem"); > >> + free(path); > >> + > >> + path = realpath(tmp_path, NULL); > >> + if (!path) > >> + return -errno; > >> + > >> + subsys = basename(path); > >> + > >> + /* check if subsystem is cxl */ > >> + if (!strcmp(subsys, "cxl")) > >> + rc = 1; > >> + else > >> + rc = 0; > >> + > >> + free(path); > >> + return rc; > >> +} > >> + > >> static void *add_bus(void *parent, int id, const char *ctl_base) > >> { > >> char buf[SYSFS_ATTR_SIZE]; > >> @@ -919,6 +962,11 @@ static void *add_bus(void *parent, int id, const char *ctl_base) > >> else > >> bus->has_of_node = 1; > >> > >> + if (is_ndbus_cxl(ctl_base)) > >> + bus->has_cxl = 1; > >> + else > >> + bus->has_cxl = 0; > >> + > > > > I think you can drop is_ndbus_cxl() and just do this: > > > > @@ -981,6 +976,11 @@ static void *add_bus(void *parent, int id, const char *ctl_base) > > if (!bus->provider) > > goto err_read; > > > > + if (strcasestr("cxl", provider)) > > + bus->has_cxl = 1; > > + else > > + bus->has_cxl = 0; > > + > > Can you explain why this is preferred? Just less code to achieve a similar result. I do like the precision of looking at the subsytem of bus device parent, just not the multiple calls to realpath() with dirname() and basename() thrown in which struck me as unnecessary. How about this: diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index c569178b9a3a..76bd7167bc70 100644 --- a/ndctl/lib/libndctl.c +++ b/ndctl/lib/libndctl.c @@ -877,40 +877,16 @@ static enum ndctl_fwa_method fwa_method_to_method(const char *fwa_method) return NDCTL_FWA_METHOD_RESET; } -static int is_ndbus_cxl(const char *ctl_base) +static int is_subsys_cxl(const char *subsys) { - char *path, *ppath, *subsys; - char tmp_path[PATH_MAX]; + char *path; int rc; - /* get the real path of ctl_base */ - path = realpath(ctl_base, NULL); + path = realpath(subsys, NULL); if (!path) return -errno; - /* setup to get the nd bridge device backing the ctl */ - sprintf(tmp_path, "%s/device", path); - free(path); - - path = realpath(tmp_path, NULL); - if (!path) - return -errno; - - /* get the parent dir of the ndbus, which should be the nvdimm-bridge */ - ppath = dirname(path); - - /* setup to get the subsystem of the nvdimm-bridge */ - sprintf(tmp_path, "%s/%s", ppath, "subsystem"); - free(path); - - path = realpath(tmp_path, NULL); - if (!path) - return -errno; - - subsys = basename(path); - - /* check if subsystem is cxl */ - if (!strcmp(subsys, "cxl")) + if (!strcmp(subsys, "/sys/bus/cxl")) rc = 1; else rc = 0; @@ -962,7 +938,8 @@ static void *add_bus(void *parent, int id, const char *ctl_base) else bus->has_of_node = 1; - if (is_ndbus_cxl(ctl_base)) + sprintf(path, "%s/device/../subsys", ctl_base); + if (is_subsys_cxl(path)) bus->has_cxl = 1; else bus->has_cxl = 0;
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index ad54f0626510..10422e24d38b 100644 --- a/ndctl/lib/libndctl.c +++ b/ndctl/lib/libndctl.c @@ -12,6 +12,7 @@ #include <ctype.h> #include <fcntl.h> #include <dirent.h> +#include <libgen.h> #include <sys/stat.h> #include <sys/types.h> #include <sys/ioctl.h> @@ -876,6 +877,48 @@ static enum ndctl_fwa_method fwa_method_to_method(const char *fwa_method) return NDCTL_FWA_METHOD_RESET; } +static int is_ndbus_cxl(const char *ctl_base) +{ + char *path, *ppath, *subsys; + char tmp_path[PATH_MAX]; + int rc; + + /* get the real path of ctl_base */ + path = realpath(ctl_base, NULL); + if (!path) + return -errno; + + /* setup to get the nd bridge device backing the ctl */ + sprintf(tmp_path, "%s/device", path); + free(path); + + path = realpath(tmp_path, NULL); + if (!path) + return -errno; + + /* get the parent dir of the ndbus, which should be the nvdimm-bridge */ + ppath = dirname(path); + + /* setup to get the subsystem of the nvdimm-bridge */ + sprintf(tmp_path, "%s/%s", ppath, "subsystem"); + free(path); + + path = realpath(tmp_path, NULL); + if (!path) + return -errno; + + subsys = basename(path); + + /* check if subsystem is cxl */ + if (!strcmp(subsys, "cxl")) + rc = 1; + else + rc = 0; + + free(path); + return rc; +} + static void *add_bus(void *parent, int id, const char *ctl_base) { char buf[SYSFS_ATTR_SIZE]; @@ -919,6 +962,11 @@ static void *add_bus(void *parent, int id, const char *ctl_base) else bus->has_of_node = 1; + if (is_ndbus_cxl(ctl_base)) + bus->has_cxl = 1; + else + bus->has_cxl = 0; + sprintf(path, "%s/device/nfit/dsm_mask", ctl_base); if (sysfs_read_attr(ctx, path, buf) < 0) bus->nfit_dsm_mask = 0; @@ -1050,6 +1098,11 @@ NDCTL_EXPORT int ndctl_bus_has_of_node(struct ndctl_bus *bus) return bus->has_of_node; } +NDCTL_EXPORT int ndctl_bus_has_cxl(struct ndctl_bus *bus) +{ + return bus->has_cxl; +} + NDCTL_EXPORT int ndctl_bus_is_papr_scm(struct ndctl_bus *bus) { char buf[SYSFS_ATTR_SIZE]; diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym index 75c32b9d4967..2892544d1985 100644 --- a/ndctl/lib/libndctl.sym +++ b/ndctl/lib/libndctl.sym @@ -464,4 +464,5 @@ LIBNDCTL_27 { } LIBNDCTL_26; LIBNDCTL_28 { ndctl_dimm_disable_master_passphrase; + ndctl_bus_has_cxl; } LIBNDCTL_27; diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h index e5c56295556d..46bc8908bd90 100644 --- a/ndctl/lib/private.h +++ b/ndctl/lib/private.h @@ -163,6 +163,7 @@ struct ndctl_bus { int regions_init; int has_nfit; int has_of_node; + int has_cxl; char *bus_path; char *bus_buf; size_t buf_len; diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h index c52e82a6f826..91ef0f42f654 100644 --- a/ndctl/libndctl.h +++ b/ndctl/libndctl.h @@ -133,6 +133,7 @@ struct ndctl_bus *ndctl_bus_get_next(struct ndctl_bus *bus); struct ndctl_ctx *ndctl_bus_get_ctx(struct ndctl_bus *bus); int ndctl_bus_has_nfit(struct ndctl_bus *bus); int ndctl_bus_has_of_node(struct ndctl_bus *bus); +int ndctl_bus_has_cxl(struct ndctl_bus *bus); int ndctl_bus_is_papr_scm(struct ndctl_bus *bus); unsigned int ndctl_bus_get_major(struct ndctl_bus *bus); unsigned int ndctl_bus_get_minor(struct ndctl_bus *bus);