linux-cxl.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Verma, Vishal L" <vishal.l.verma@intel.com>
To: "Williams, Dan J" <dan.j.williams@intel.com>
Cc: "Widawsky, Ben" <ben.widawsky@intel.com>,
	"linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
	"nvdimm@lists.linux.dev" <nvdimm@lists.linux.dev>
Subject: Re: [ndctl PATCH v4 12/17] libcxl: add interfaces for label operations
Date: Thu, 14 Oct 2021 22:18:10 +0000	[thread overview]
Message-ID: <0b63b6e4d89d13b3f52d2d6e94331da94a0370af.camel@intel.com> (raw)
In-Reply-To: <CAPcyv4h49Cei27qLAL8oUmcpa=Su_VArrAEzGwt3VSbpCoxYTw@mail.gmail.com>

On Thu, 2021-10-14 at 14:27 -0700, Dan Williams wrote:
> On Thu, Oct 7, 2021 at 1:22 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > 
> > Add libcxl interfaces to allow performinfg label (LSA) manipulations.
> > Add a 'cxl_cmd_new_set_lsa' interface to create a 'Set LSA' mailbox
> > command payload, and interfaces to read, write, and zero the LSA area on
> > a memdev.
> > 
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  cxl/lib/private.h  |   6 ++
> >  cxl/lib/libcxl.c   | 137 +++++++++++++++++++++++++++++++++++++++++++++
> >  cxl/libcxl.h       |   7 +++
> >  cxl/lib/libcxl.sym |   4 ++
> >  4 files changed, 154 insertions(+)
> > 
> > diff --git a/cxl/lib/private.h b/cxl/lib/private.h
> > index 671f12f..89212df 100644
> > --- a/cxl/lib/private.h
> > +++ b/cxl/lib/private.h
> > @@ -79,6 +79,12 @@ struct cxl_cmd_get_lsa_in {
> >         le32 length;
> >  } __attribute__((packed));
> > 
> > +struct cxl_cmd_set_lsa {
> > +       le32 offset;
> > +       le32 rsvd;
> > +       unsigned char lsa_data[0];
> > +} __attribute__ ((packed));
> > +
> >  struct cxl_cmd_get_health_info {
> >         u8 health_status;
> >         u8 media_status;
> > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> > index 59d091c..8dd69cf 100644
> > --- a/cxl/lib/libcxl.c
> > +++ b/cxl/lib/libcxl.c
> > @@ -1126,3 +1126,140 @@ CXL_EXPORT int cxl_cmd_get_out_size(struct cxl_cmd *cmd)
> >  {
> >         return cmd->send_cmd->out.size;
> >  }
> > +
> > +CXL_EXPORT struct cxl_cmd *cxl_cmd_new_write_label(struct cxl_memdev *memdev,
> > +               void *lsa_buf, unsigned int offset, unsigned int length)
> > +{
> > +       struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev);
> > +       struct cxl_cmd_set_lsa *set_lsa;
> > +       struct cxl_cmd *cmd;
> > +       int rc;
> > +
> > +       cmd = cxl_cmd_new_generic(memdev, CXL_MEM_COMMAND_ID_SET_LSA);
> > +       if (!cmd)
> > +               return NULL;
> > +
> > +       /* this will allocate 'in.payload' */
> > +       rc = cxl_cmd_set_input_payload(cmd, NULL, sizeof(*set_lsa) + length);
> > +       if (rc) {
> > +               err(ctx, "%s: cmd setup failed: %s\n",
> > +                       cxl_memdev_get_devname(memdev), strerror(-rc));
> > +               goto out_fail;
> > +       }
> > +       set_lsa = (void *)cmd->send_cmd->in.payload;
> 
> ...the cast is still nagging at me especially when this knows what the
> payload is supposed to be. What about a helper per command type of the
> form:
> 
> struct cxl_cmd_$name *to_cxl_cmd_$name(struct cxl_cmd *cmd)
> {
>     if (cmd->send_cmd->id != CXL_MEM_COMMAND_ID_$NAME)
>         return NULL;
>     return (struct cxl_cmd_$name *) cmd->send_cmd->in.payload;
> }
> 
> > +       set_lsa->offset = cpu_to_le32(offset);
> > +       memcpy(set_lsa->lsa_data, lsa_buf, length);
> > +
> > +       return cmd;
> > +
> > +out_fail:
> > +       cxl_cmd_unref(cmd);
> > +       return NULL;
> > +}
> > +
> > +enum lsa_op {
> > +       LSA_OP_GET,
> > +       LSA_OP_SET,
> > +       LSA_OP_ZERO,
> > +};
> > +
> > +static int lsa_op(struct cxl_memdev *memdev, int op, void **buf,
> > +               size_t length, size_t offset)
> > +{
> > +       const char *devname = cxl_memdev_get_devname(memdev);
> > +       struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev);
> > +       struct cxl_cmd *cmd;
> > +       void *zero_buf = NULL;
> > +       ssize_t ret_len;
> > +       int rc = 0;
> > +
> > +       if (op != LSA_OP_ZERO && (buf == NULL || *buf == NULL)) {
> > +               err(ctx, "%s: LSA buffer cannot be NULL\n", devname);
> > +               return -EINVAL;
> > +       }
> > +
> > +       /* TODO: handle the case for offset + len > mailbox payload size */
> > +       switch (op) {
> > +       case LSA_OP_GET:
> > +               if (length == 0)
> > +                       length = memdev->lsa_size;
> 
> What's the use case to support a default size for get? If someone
> wants to do a zero length lsa_op shouldn't that just return immediate
> success just like 0 length read(2)?

I was going for a convenience shortcut where a user can get the whole
thing without needing to worry about what the memdev's lsa_size is, but
in hindsight, this doesn't make much sense. The API can take length to
be literal - and not do any work for length == 0, and the cli can have
the convenience function of implying a full label read if no length
option was supplied.

> 
> > +               cmd = cxl_cmd_new_read_label(memdev, offset, length);
> > +               if (!cmd)
> > +                       return -ENOMEM;
> > +               rc = cxl_cmd_set_output_payload(cmd, *buf, length);
> > +               if (rc) {
> > +                       err(ctx, "%s: cmd setup failed: %s\n",
> > +                           cxl_memdev_get_devname(memdev), strerror(-rc));
> > +                       goto out;
> > +               }
> > +               break;
> > +       case LSA_OP_ZERO:
> > +               if (length == 0)
> 
> This one makes sense because the caller just wants the whole area zeroed.

I think the API between all three of these should be consistent in how
'length' is treated. It would make sense to zero everything if the
length were removed from the API entirely, but if it is part of it,
then we should just do what read/write do.

> 
> > +                       length = memdev->lsa_size;
> > +               zero_buf = calloc(1, length);
> > +               if (!zero_buf)
> > +                       return -ENOMEM;
> > +               buf = &zero_buf;
> 
> 
> > +               /* fall through */
> > +       case LSA_OP_SET:
> 
> ...and if length == 0 here there's no need to go any further, we're done.

Yep will change.

> 
> > +               cmd = cxl_cmd_new_write_label(memdev, *buf, offset, length);
> > +               if (!cmd) {
> > +                       rc = -ENOMEM;
> > +                       goto out_free;
> > +               }
> > +               break;
> > +       default:
> > +               return -EOPNOTSUPP;
> > +       }
> > +
> > +       rc = cxl_cmd_submit(cmd);
> > +       if (rc < 0) {
> > +               err(ctx, "%s: cmd submission failed: %s\n",
> > +                       devname, strerror(-rc));
> > +               goto out;
> > +       }
> > +
> > +       rc = cxl_cmd_get_mbox_status(cmd);
> > +       if (rc != 0) {
> > +               err(ctx, "%s: firmware status: %d\n",
> > +                       devname, rc);
> > +               rc = -ENXIO;
> > +               goto out;
> > +       }
> > +
> > +       if (op == LSA_OP_GET) {
> > +               ret_len = cxl_cmd_read_label_get_payload(cmd, *buf, length);
> > +               if (ret_len < 0) {
> > +                       rc = ret_len;
> > +                       goto out;
> > +               }
> > +       }
> > +
> > +       /*
> > +        * TODO: If writing, the memdev may need to be disabled/re-enabled to
> > +        * refresh any cached LSA data in the kernel.
> > +        */
> 
> I think we're sufficiently protected by the nvdimm-bridge up/down
> dependency. I.e. if the above commands actually worked then nothing
> should have had the labels cached in the kernel.

Yep makes sense, I'll drop this.

> 
> After fixing the the length == 0 case you can add:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>


  reply	other threads:[~2021-10-14 22:18 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-07  8:21 [ndctl PATCH v4 00/17] Initial CXL support Vishal Verma
2021-10-07  8:21 ` [ndctl PATCH v4 01/17] ndctl: add .clang-format Vishal Verma
2021-10-07  8:21 ` [ndctl PATCH v4 02/17] cxl: add a cxl utility and libcxl library Vishal Verma
2021-10-07  8:21 ` [ndctl PATCH v4 03/17] cxl: add a local copy of the cxl_mem UAPI header Vishal Verma
2021-10-07  8:21 ` [ndctl PATCH v4 04/17] util: add the struct_size() helper from the kernel Vishal Verma
2021-10-14  2:40   ` Dan Williams
2021-10-07  8:21 ` [ndctl PATCH v4 05/17] libcxl: add support for command query and submission Vishal Verma
2021-10-14  2:53   ` Dan Williams
2021-10-07  8:21 ` [ndctl PATCH v4 06/17] libcxl: add support for the 'Identify Device' command Vishal Verma
2021-10-07  8:21 ` [ndctl PATCH v4 07/17] libcxl: add GET_HEALTH_INFO mailbox command and accessors Vishal Verma
2021-10-14 16:01   ` Dan Williams
2021-11-02 20:22     ` Verma, Vishal L
2021-11-02 20:27       ` Dan Williams
2021-10-07  8:21 ` [ndctl PATCH v4 08/17] libcxl: add support for the 'GET_LSA' command Vishal Verma
2021-10-14 16:35   ` Dan Williams
2021-10-14 20:06     ` Verma, Vishal L
2021-10-14 20:55       ` Dan Williams
2021-10-07  8:21 ` [ndctl PATCH v4 09/17] util/hexdump: Add a util helper to print a buffer in hex Vishal Verma
2021-10-14 16:48   ` Dan Williams
2021-10-14 20:33     ` Verma, Vishal L
2021-10-14 22:39       ` Dan Williams
2021-11-02 20:25         ` Verma, Vishal L
2021-10-07  8:21 ` [ndctl PATCH v4 10/17] libcxl: add label_size to cxl_memdev, and an API to retrieve it Vishal Verma
2021-10-14 18:24   ` Dan Williams
2021-10-14 21:50     ` Verma, Vishal L
2021-10-07  8:21 ` [ndctl PATCH v4 11/17] libcxl: add a stub interface to determine whether a memdev is active Vishal Verma
2021-10-14 19:59   ` Dan Williams
2021-10-07  8:21 ` [ndctl PATCH v4 12/17] libcxl: add interfaces for label operations Vishal Verma
2021-10-14 21:27   ` Dan Williams
2021-10-14 22:18     ` Verma, Vishal L [this message]
2021-10-14 22:24     ` Verma, Vishal L
2021-10-14 22:45       ` Dan Williams
2021-10-07  8:21 ` [ndctl PATCH v4 13/17] cxl: add commands to read, write, and zero labels Vishal Verma
2021-10-14 22:34   ` Dan Williams
2021-10-07  8:21 ` [ndctl PATCH v4 14/17] Documentation/cxl: add library API documentation Vishal Verma
2021-10-14 23:31   ` Dan Williams
2021-11-05 18:58   ` Dan Williams
2021-10-07  8:21 ` [ndctl PATCH v4 15/17] ndctl: Add CXL packages to the RPM spec Vishal Verma
2021-10-14 23:33   ` Dan Williams
2021-10-07  8:21 ` [ndctl PATCH v4 16/17] cxl-cli: add bash completion Vishal Verma
2021-10-14 23:34   ` Dan Williams
2021-10-07  8:21 ` [ndctl PATCH v4 17/17] cxl: add health information to cxl-list Vishal Verma
2021-10-11 22:07   ` Verma, Vishal L
2021-10-15  0:09     ` Dan Williams
2021-10-14 23:42   ` Verma, Vishal L
2021-10-15 21:15     ` Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0b63b6e4d89d13b3f52d2d6e94331da94a0370af.camel@intel.com \
    --to=vishal.l.verma@intel.com \
    --cc=ben.widawsky@intel.com \
    --cc=dan.j.williams@intel.com \
    --cc=linux-cxl@vger.kernel.org \
    --cc=nvdimm@lists.linux.dev \
    --subject='Re: [ndctl PATCH v4 12/17] libcxl: add interfaces for label operations' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).