Re: [PATCH v8 19/46] hw/cxl/device: Add some trivial commands

[Jonathan Cameron]

On Fri, 18 Mar 2022 09:56:22 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> On Fri, Mar 18, 2022 at 03:06:08PM +0000, Jonathan Cameron wrote:
> > From: Ben Widawsky <ben.widawsky@intel.com>
> > 
> > GET_FW_INFO and GET_PARTITION_INFO, for this emulation, is equivalent to
> > info already returned in the IDENTIFY command. To have a more robust
> > implementation, add those.
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  hw/cxl/cxl-mailbox-utils.c | 69 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >   
> 
> snip
> 
> >  
> > +static ret_code cmd_ccls_get_partition_info(struct cxl_cmd *cmd,
> > +                                           CXLDeviceState *cxl_dstate,
> > +                                           uint16_t *len)
> > +{
> > +    struct {
> > +        uint64_t active_vmem;
> > +        uint64_t active_pmem;
> > +        uint64_t next_vmem;
> > +        uint64_t next_pmem;
> > +    } QEMU_PACKED *part_info = (void *)cmd->payload;
> > +    QEMU_BUILD_BUG_ON(sizeof(*part_info) != 0x20);
> > +    uint64_t size = cxl_dstate->pmem_size;
> > +
> > +    if (!QEMU_IS_ALIGNED(size, 256 << 20)) {
> > +        return CXL_MBOX_INTERNAL_ERROR;
> > +    }
> > +
> > +    /* PMEM only */
> > +    part_info->active_vmem = 0;
> > +    part_info->next_vmem = 0;
> > +    part_info->active_pmem = size / (256 << 20);
> > +    part_info->next_pmem = part_info->active_pmem;  
> 
> Setting next like this is logical, but it's not per the CXL spec:
> 
> 8.2.9.5.2.1
> "Next Persistent Capacity: If non-zero, this value shall become the
> Active Persistent Capacity on the next cold reset. If both this field and the
> Next Volatile Capacity field are zero, there is no pending change to the
> partitioning."
> 
> next_(vmem|pmem) should start as zero and only change as the result
> of a successful set_partition_info command.

Ah.  Good point and now fixed up.

> 
> From your cover letter:
> * Volatile memory devices (easy but it's more code so left for now).
> Wondering if this is something I could do, and follow that with
> set_partition support. Does that sound reasonable?

Sure, that would be great.

It raises an interesting question for what the volatile / non volatile
backend will look like.  So what is the command line?

So far I'd been assuming we'd do it as separate memdev devices
for volatile from those for non volatile because chances are
someone testing would back the volatile with RAM the non volatile with
a file.  If we enable the partitioning control that approach won't
make much sense.  Various options come to mind.
1) Just back with one memdev and present that as persistent or not
   but actually always have it persistent under the hood.
2) Allow backing with 2 memdevs and don't support the set_partition
   command. Lazy approach I was planing on doing eventualy.
3) Support 3 memdevs.  The middle one of which is the part we
   allow to be repartitioned if it is provided

Also, to actually enable this we'd probably want more HDM decoders
than currently supported and flags for CFMWS which aren't there
yet (Ben asked for them but I've left it for a future patch set).

Lots still to do once the initial patch set is moving forwards.
I don't mind queuing stuff up it gitlab though and then we can
send suitable series to the list once the earlier part has been
applied.  In the meantime we would have a moderately stable
based to build on top of.

Thanks,

Jonathan



 
> 
> Alison
> 
> > +
> > +    *len = sizeof(*part_info);
> > +    return CXL_MBOX_SUCCESS;
> > +}
> > +  
> 
> snip
> 
>