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

[Alison Schofield]

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.

>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? 

Alison

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

snip