Re: [ndctl PATCH v3 5/6] libcxl: add interfaces for SET_PARTITION_INFO mailbox command

[Dan Williams <dan.j.williams@intel.com>]

On Thu, Jan 27, 2022 at 12:46 PM Alison Schofield
<alison.schofield@intel.com> wrote:
>
> Hi Dan,
> Thanks for the review. I'm still working thru this, but a clarifying
> question below...
>
> On Wed, Jan 26, 2022 at 03:41:14PM -0800, Dan Williams wrote:
> > On Tue, Jan 18, 2022 at 12:20 PM <alison.schofield@intel.com> wrote:
> > >
> > > From: Alison Schofield <alison.schofield@intel.com>
> > >
> > > Users may want the ability to change the partition layout of a CXL
> > > memory device.
> > >
> > > Add interfaces to libcxl to allocate and send a SET_PARTITION_INFO
> > > mailbox as defined in the CXL 2.0 specification.
> > >
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > > ---
> > >  cxl/lib/libcxl.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  cxl/lib/libcxl.sym |  5 +++++
> > >  cxl/lib/private.h  |  8 ++++++++
> > >  cxl/libcxl.h       |  5 +++++
> > >  4 files changed, 68 insertions(+)
> > >
> snip
> >
> >
> > I don't understand what this is for?
> >
> > Let's back up. In order to future proof against spec changes, and
> > endianness, struct packing and all other weird things that make struct
> > ABIs hard to maintain compatibility the ndctl project adopts the
> > libabc template of just not letting library consumers see any raw data
> > structures or bit fields by default [1]. For a situation like this
> > since the command only has one flag that affects the mode of operation
> > I would just go ahead and define an enum for that explicitly.
> >
> > enum cxl_setpartition_mode {
> >     CXL_SETPART_NONE,
> >     CXL_SETPART_NEXTBOOT,
> >     CXL_SETPART_IMMEDIATE,
> > };
> >
> > Then the main function prototype becomes:
> >
> > int cxl_cmd_new_setpartition(struct cxl_memdev *memdev, unsigned long
> > long volatile_capacity);
> >
> > ...with a new:
> >
> > int cxl_cmd_setpartition_set_mode(struct cxl_cmd *cmd, enum
> > cxl_setpartition_mode mode);
> >
>
> I don't understand setting of the mode separately. Can it be:
>
> int cxl_cmd_new_setpartition(struct cxl_memdev *memdev,
>                              unsigned long long volatile_capacity,
>                              enum cxl_setpartition_mode mode);

It could be, but what happens when the specification defines a new
flag for this command? Then we would have cxl_cmd_new_setpartition()
and cxl_cmd_new_setpartition2()  to add the new parameters. A helper
function after establishing the cxl_cmd context lets you have
flexibility to extend the base command by as many new flags and modes
that come along... hopefully none, but you never know.