diff mbox series

[ndctl,1/7] libcxl: add GET_PARTITION_INFO mailbox command and accessors

Message ID 9d3c55cbd36efb6eabec075cc8596a6382f1f145.1641233076.git.alison.schofield@intel.com
State Superseded
Headers show
Series Add partitioning support for CXL memdevs | expand

Commit Message

Alison Schofield Jan. 3, 2022, 8:16 p.m. UTC
From: Alison Schofield <alison.schofield@intel.com>

Add libcxl APIs to create a new GET_PARTITION_INFO mailbox command, the
command output data structure (privately), and accessor APIs to return
the different fields in the partition info output.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 cxl/lib/private.h  | 10 ++++++++++
 cxl/libcxl.h       |  5 +++++
 util/size.h        |  1 +
 cxl/lib/libcxl.c   | 41 +++++++++++++++++++++++++++++++++++++++++
 cxl/lib/libcxl.sym |  5 +++++
 5 files changed, 62 insertions(+)

Comments

Ira Weiny Jan. 6, 2022, 8:19 p.m. UTC | #1
On Mon, Jan 03, 2022 at 12:16:12PM -0800, Schofield, Alison wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Add libcxl APIs to create a new GET_PARTITION_INFO mailbox command, the
> command output data structure (privately), and accessor APIs to return
> the different fields in the partition info output.

I would rephrase this:

libcxl provides functions for C code to issue cxl mailbox commands as well as
parse the output returned.  Get partition info should be part of this API.

Add the libcxl get partition info mailbox support as well as an API to parse
the fields of the command returned.

All fields are specified in multiples of 256MB so also define a capacity
multiplier to convert the raw data into bytes.

> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  cxl/lib/private.h  | 10 ++++++++++
>  cxl/libcxl.h       |  5 +++++
>  util/size.h        |  1 +
>  cxl/lib/libcxl.c   | 41 +++++++++++++++++++++++++++++++++++++++++
>  cxl/lib/libcxl.sym |  5 +++++
>  5 files changed, 62 insertions(+)
> 
> diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> index a1b8b50..dd9234f 100644
> --- a/cxl/lib/private.h
> +++ b/cxl/lib/private.h
> @@ -7,6 +7,7 @@
>  #include <cxl/cxl_mem.h>
>  #include <ccan/endian/endian.h>
>  #include <ccan/short_types/short_types.h>
> +#include <util/size.h>
>  
>  #define CXL_EXPORT __attribute__ ((visibility("default")))
>  
> @@ -104,6 +105,15 @@ struct cxl_cmd_get_health_info {
>  	le32 pmem_errors;
>  } __attribute__((packed));
>  
> +struct cxl_cmd_get_partition_info {
> +	le64 active_volatile_cap;
> +	le64 active_persistent_cap;
> +	le64 next_volatile_cap;
> +	le64 next_persistent_cap;
> +} __attribute__((packed));
> +
> +#define CXL_CAPACITY_MULTIPLIER		SZ_256M
> +
>  /* CXL 2.0 8.2.9.5.3 Byte 0 Health Status */
>  #define CXL_CMD_HEALTH_INFO_STATUS_MAINTENANCE_NEEDED_MASK		BIT(0)
>  #define CXL_CMD_HEALTH_INFO_STATUS_PERFORMANCE_DEGRADED_MASK		BIT(1)
> diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> index 89d35ba..7cf9061 100644
> --- a/cxl/libcxl.h
> +++ b/cxl/libcxl.h
> @@ -109,6 +109,11 @@ ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd, void *buf,
>  		unsigned int length);
>  struct cxl_cmd *cxl_cmd_new_write_label(struct cxl_memdev *memdev,
>  		void *buf, unsigned int offset, unsigned int length);
> +struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev);

why 'new'?  Why not:

cxl_cmd_get_partition_info()

?

> +unsigned long long cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd);
> +unsigned long long cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd);
> +unsigned long long cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd);
> +unsigned long long cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd);

These are pretty long function names.  :-/

I also wonder if the conversion to bytes should be reflected in the function
name.  Because returning 'cap' might imply to someone they are getting the raw
data field.

Ira

>  
>  #ifdef __cplusplus
>  } /* extern "C" */
> diff --git a/util/size.h b/util/size.h
> index a0f3593..e72467f 100644
> --- a/util/size.h
> +++ b/util/size.h
> @@ -15,6 +15,7 @@
>  #define SZ_4M     0x00400000
>  #define SZ_16M    0x01000000
>  #define SZ_64M    0x04000000
> +#define SZ_256M	  0x10000000
>  #define SZ_1G     0x40000000
>  #define SZ_1T 0x10000000000ULL
>  
> diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> index f0664be..f3d4022 100644
> --- a/cxl/lib/libcxl.c
> +++ b/cxl/lib/libcxl.c
> @@ -1157,6 +1157,47 @@ CXL_EXPORT ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd,
>  	return length;
>  }
>  
> +CXL_EXPORT struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev)
> +{
> +	return cxl_cmd_new_generic(memdev,
> +				   CXL_MEM_COMMAND_ID_GET_PARTITION_INFO);
> +}
> +
> +#define cmd_partition_get_capacity_field(cmd, field)			\
> +do {										\
> +	struct cxl_cmd_get_partition_info *c =					\
> +		(struct cxl_cmd_get_partition_info *)cmd->send_cmd->out.payload;\
> +	int rc = cxl_cmd_validate_status(cmd,					\
> +			CXL_MEM_COMMAND_ID_GET_PARTITION_INFO);			\
> +	if (rc)									\
> +		return ULLONG_MAX;							\
> +	return le64_to_cpu(c->field) * CXL_CAPACITY_MULTIPLIER;			\
> +} while (0)
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd)
> +{
> +	cmd_partition_get_capacity_field(cmd, active_volatile_cap);
> +}
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd)
> +{
> +	cmd_partition_get_capacity_field(cmd, active_persistent_cap);
> +}
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd)
> +{
> +	cmd_partition_get_capacity_field(cmd, next_volatile_cap);
> +}
> +
> +CXL_EXPORT unsigned long long
> +cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd)
> +{
> +	cmd_partition_get_capacity_field(cmd, next_persistent_cap);
> +}
> +
>  CXL_EXPORT int cxl_cmd_submit(struct cxl_cmd *cmd)
>  {
>  	struct cxl_memdev *memdev = cmd->memdev;
> diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
> index 077d104..09d6d94 100644
> --- a/cxl/lib/libcxl.sym
> +++ b/cxl/lib/libcxl.sym
> @@ -70,6 +70,11 @@ global:
>  	cxl_memdev_zero_label;
>  	cxl_memdev_write_label;
>  	cxl_memdev_read_label;
> +	cxl_cmd_new_get_partition_info;
> +	cxl_cmd_get_partition_info_get_active_volatile_cap;
> +	cxl_cmd_get_partition_info_get_active_persistent_cap;
> +	cxl_cmd_get_partition_info_get_next_volatile_cap;
> +	cxl_cmd_get_partition_info_get_next_persistent_cap;
>  local:
>          *;
>  };
> -- 
> 2.31.1
>
Dan Williams Jan. 6, 2022, 9:21 p.m. UTC | #2
On Thu, Jan 6, 2022 at 12:19 PM Ira Weiny <ira.weiny@intel.com> wrote:
>
> On Mon, Jan 03, 2022 at 12:16:12PM -0800, Schofield, Alison wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > Add libcxl APIs to create a new GET_PARTITION_INFO mailbox command, the
> > command output data structure (privately), and accessor APIs to return
> > the different fields in the partition info output.
>
> I would rephrase this:
>
> libcxl provides functions for C code to issue cxl mailbox commands as well as
> parse the output returned.  Get partition info should be part of this API.
>
> Add the libcxl get partition info mailbox support as well as an API to parse
> the fields of the command returned.
>
> All fields are specified in multiples of 256MB so also define a capacity
> multiplier to convert the raw data into bytes.
>
> >
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  cxl/lib/private.h  | 10 ++++++++++
> >  cxl/libcxl.h       |  5 +++++
> >  util/size.h        |  1 +
> >  cxl/lib/libcxl.c   | 41 +++++++++++++++++++++++++++++++++++++++++
> >  cxl/lib/libcxl.sym |  5 +++++
> >  5 files changed, 62 insertions(+)
> >
> > diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> > index a1b8b50..dd9234f 100644
> > --- a/cxl/lib/private.h
> > +++ b/cxl/lib/private.h
> > @@ -7,6 +7,7 @@
> >  #include <cxl/cxl_mem.h>
> >  #include <ccan/endian/endian.h>
> >  #include <ccan/short_types/short_types.h>
> > +#include <util/size.h>
> >
> >  #define CXL_EXPORT __attribute__ ((visibility("default")))
> >
> > @@ -104,6 +105,15 @@ struct cxl_cmd_get_health_info {
> >       le32 pmem_errors;
> >  } __attribute__((packed));
> >
> > +struct cxl_cmd_get_partition_info {
> > +     le64 active_volatile_cap;
> > +     le64 active_persistent_cap;
> > +     le64 next_volatile_cap;
> > +     le64 next_persistent_cap;
> > +} __attribute__((packed));
> > +
> > +#define CXL_CAPACITY_MULTIPLIER              SZ_256M
> > +
> >  /* CXL 2.0 8.2.9.5.3 Byte 0 Health Status */
> >  #define CXL_CMD_HEALTH_INFO_STATUS_MAINTENANCE_NEEDED_MASK           BIT(0)
> >  #define CXL_CMD_HEALTH_INFO_STATUS_PERFORMANCE_DEGRADED_MASK         BIT(1)
> > diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> > index 89d35ba..7cf9061 100644
> > --- a/cxl/libcxl.h
> > +++ b/cxl/libcxl.h
> > @@ -109,6 +109,11 @@ ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd, void *buf,
> >               unsigned int length);
> >  struct cxl_cmd *cxl_cmd_new_write_label(struct cxl_memdev *memdev,
> >               void *buf, unsigned int offset, unsigned int length);
> > +struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev);
>
> why 'new'?  Why not:
>
> cxl_cmd_get_partition_info()
>
> ?

The "new" is the naming convention inherited from ndctl indicating the
allocation of a new command object. The motivation is to have a verb /
action in all of the APIs.

>
> > +unsigned long long cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd);
> > +unsigned long long cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd);
> > +unsigned long long cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd);
> > +unsigned long long cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd);
>
> These are pretty long function names.  :-/

If you think those are long, how about:

cxl_cmd_health_info_get_media_powerloss_persistence_loss

The motivation here is to keep data structure layouts hidden behind
APIs to ease the maintenance of binary compatibility from one library
release and specification release to the next. The side effect though
is some long function names in places.

> I also wonder if the conversion to bytes should be reflected in the function
> name.  Because returning 'cap' might imply to someone they are getting the raw
> data field.

Makes sense.
Ira Weiny Jan. 6, 2022, 9:30 p.m. UTC | #3
On Thu, Jan 06, 2022 at 01:21:45PM -0800, Dan Williams wrote:
> On Thu, Jan 6, 2022 at 12:19 PM Ira Weiny <ira.weiny@intel.com> wrote:
> >
> > On Mon, Jan 03, 2022 at 12:16:12PM -0800, Schofield, Alison wrote:
> > > From: Alison Schofield <alison.schofield@intel.com>
> > >
> > > diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> > > index 89d35ba..7cf9061 100644
> > > --- a/cxl/libcxl.h
> > > +++ b/cxl/libcxl.h
> > > @@ -109,6 +109,11 @@ ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd, void *buf,
> > >               unsigned int length);
> > >  struct cxl_cmd *cxl_cmd_new_write_label(struct cxl_memdev *memdev,
> > >               void *buf, unsigned int offset, unsigned int length);
> > > +struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev);
> >
> > why 'new'?  Why not:
> >
> > cxl_cmd_get_partition_info()
> >
> > ?
> 
> The "new" is the naming convention inherited from ndctl indicating the
> allocation of a new command object. The motivation is to have a verb /
> action in all of the APIs.

Yea my bad.  I realized that later on.  Sorry.

> 
> >
> > > +unsigned long long cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd);
> >
> > These are pretty long function names.  :-/
> 
> If you think those are long, how about:
> 
> cxl_cmd_health_info_get_media_powerloss_persistence_loss
> 
> The motivation here is to keep data structure layouts hidden behind
> APIs to ease the maintenance of binary compatibility from one library
> release and specification release to the next. The side effect though
> is some long function names in places.

Sure.  I'm ok with that.

Ira

> 
> > I also wonder if the conversion to bytes should be reflected in the function
> > name.  Because returning 'cap' might imply to someone they are getting the raw
> > data field.
> 
> Makes sense.
Verma, Vishal L Jan. 6, 2022, 9:57 p.m. UTC | #4
On Thu, 2022-01-06 at 13:21 -0800, Dan Williams wrote:
> On Thu, Jan 6, 2022 at 12:19 PM Ira Weiny <ira.weiny@intel.com> wrote:
> > 

[..]

> > > +struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev);
> > 
> > why 'new'?  Why not:
> > 
> > cxl_cmd_get_partition_info()
> > 
> > ?
> 
> The "new" is the naming convention inherited from ndctl indicating the
> allocation of a new command object. The motivation is to have a verb /
> action in all of the APIs.
> 
> > 
> > > +unsigned long long cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd);

Just one nit here about the double verb 'get'. In such cases,
get_partition_info can just become 'partition_info'

e.g.: cxl_cmd_partition_info_get_active_volatile_cap(...

>
Alison Schofield Jan. 7, 2022, 7:56 p.m. UTC | #5
Thanks for the review Ira!

On Thu, Jan 06, 2022 at 12:19:07PM -0800, Ira Weiny wrote:
> On Mon, Jan 03, 2022 at 12:16:12PM -0800, Schofield, Alison wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Add libcxl APIs to create a new GET_PARTITION_INFO mailbox command, the
> > command output data structure (privately), and accessor APIs to return
> > the different fields in the partition info output.
> 
> I would rephrase this:
> 
> libcxl provides functions for C code to issue cxl mailbox commands as well as
> parse the output returned.  Get partition info should be part of this API.
> 
> Add the libcxl get partition info mailbox support as well as an API to parse
> the fields of the command returned.
> 
> All fields are specified in multiples of 256MB so also define a capacity
> multiplier to convert the raw data into bytes.
> 
Will do. Thanks.

> > 

snip.
 
> I also wonder if the conversion to bytes should be reflected in the function
> name.  Because returning 'cap' might imply to someone they are getting the raw
> data field.

Agree. Will s/cap/bytes

>

Some additional comments were addressed by Dan & Vishal responses.
Alison Schofield Jan. 7, 2022, 8:27 p.m. UTC | #6
On Thu, Jan 06, 2022 at 01:57:59PM -0800, Vishal Verma wrote:
> 
> Just one nit here about the double verb 'get'. In such cases,
> get_partition_info can just become 'partition_info'
> 
> e.g.: cxl_cmd_partition_info_get_active_volatile_cap(...
> 

Will do - thanks Vishal!

Combiningg w Ira's feedback, it'll be:
cxl_cmd_partition_info_get_active_volatile_bytes(...

> >
>
diff mbox series

Patch

diff --git a/cxl/lib/private.h b/cxl/lib/private.h
index a1b8b50..dd9234f 100644
--- a/cxl/lib/private.h
+++ b/cxl/lib/private.h
@@ -7,6 +7,7 @@ 
 #include <cxl/cxl_mem.h>
 #include <ccan/endian/endian.h>
 #include <ccan/short_types/short_types.h>
+#include <util/size.h>
 
 #define CXL_EXPORT __attribute__ ((visibility("default")))
 
@@ -104,6 +105,15 @@  struct cxl_cmd_get_health_info {
 	le32 pmem_errors;
 } __attribute__((packed));
 
+struct cxl_cmd_get_partition_info {
+	le64 active_volatile_cap;
+	le64 active_persistent_cap;
+	le64 next_volatile_cap;
+	le64 next_persistent_cap;
+} __attribute__((packed));
+
+#define CXL_CAPACITY_MULTIPLIER		SZ_256M
+
 /* CXL 2.0 8.2.9.5.3 Byte 0 Health Status */
 #define CXL_CMD_HEALTH_INFO_STATUS_MAINTENANCE_NEEDED_MASK		BIT(0)
 #define CXL_CMD_HEALTH_INFO_STATUS_PERFORMANCE_DEGRADED_MASK		BIT(1)
diff --git a/cxl/libcxl.h b/cxl/libcxl.h
index 89d35ba..7cf9061 100644
--- a/cxl/libcxl.h
+++ b/cxl/libcxl.h
@@ -109,6 +109,11 @@  ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd, void *buf,
 		unsigned int length);
 struct cxl_cmd *cxl_cmd_new_write_label(struct cxl_memdev *memdev,
 		void *buf, unsigned int offset, unsigned int length);
+struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev);
+unsigned long long cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd);
+unsigned long long cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd);
 
 #ifdef __cplusplus
 } /* extern "C" */
diff --git a/util/size.h b/util/size.h
index a0f3593..e72467f 100644
--- a/util/size.h
+++ b/util/size.h
@@ -15,6 +15,7 @@ 
 #define SZ_4M     0x00400000
 #define SZ_16M    0x01000000
 #define SZ_64M    0x04000000
+#define SZ_256M	  0x10000000
 #define SZ_1G     0x40000000
 #define SZ_1T 0x10000000000ULL
 
diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
index f0664be..f3d4022 100644
--- a/cxl/lib/libcxl.c
+++ b/cxl/lib/libcxl.c
@@ -1157,6 +1157,47 @@  CXL_EXPORT ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd,
 	return length;
 }
 
+CXL_EXPORT struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev)
+{
+	return cxl_cmd_new_generic(memdev,
+				   CXL_MEM_COMMAND_ID_GET_PARTITION_INFO);
+}
+
+#define cmd_partition_get_capacity_field(cmd, field)			\
+do {										\
+	struct cxl_cmd_get_partition_info *c =					\
+		(struct cxl_cmd_get_partition_info *)cmd->send_cmd->out.payload;\
+	int rc = cxl_cmd_validate_status(cmd,					\
+			CXL_MEM_COMMAND_ID_GET_PARTITION_INFO);			\
+	if (rc)									\
+		return ULLONG_MAX;							\
+	return le64_to_cpu(c->field) * CXL_CAPACITY_MULTIPLIER;			\
+} while (0)
+
+CXL_EXPORT unsigned long long
+cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd)
+{
+	cmd_partition_get_capacity_field(cmd, active_volatile_cap);
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd)
+{
+	cmd_partition_get_capacity_field(cmd, active_persistent_cap);
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd)
+{
+	cmd_partition_get_capacity_field(cmd, next_volatile_cap);
+}
+
+CXL_EXPORT unsigned long long
+cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd)
+{
+	cmd_partition_get_capacity_field(cmd, next_persistent_cap);
+}
+
 CXL_EXPORT int cxl_cmd_submit(struct cxl_cmd *cmd)
 {
 	struct cxl_memdev *memdev = cmd->memdev;
diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym
index 077d104..09d6d94 100644
--- a/cxl/lib/libcxl.sym
+++ b/cxl/lib/libcxl.sym
@@ -70,6 +70,11 @@  global:
 	cxl_memdev_zero_label;
 	cxl_memdev_write_label;
 	cxl_memdev_read_label;
+	cxl_cmd_new_get_partition_info;
+	cxl_cmd_get_partition_info_get_active_volatile_cap;
+	cxl_cmd_get_partition_info_get_active_persistent_cap;
+	cxl_cmd_get_partition_info_get_next_volatile_cap;
+	cxl_cmd_get_partition_info_get_next_persistent_cap;
 local:
         *;
 };