Re: [PATCH 2/2] hw/cxl: Allow CXL type-3 devices to be persistent or volatile

[Davidlohr Bueso]

On Thu, 06 Oct 2022, Gregory Price wrote:

> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index bc1bb18844..dfec11a1b5 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -138,7 +138,7 @@ static ret_code cmd_firmware_update_get_info(struct cxl_cmd 
> *cmd,
>     } QEMU_PACKED *fw_info;
>     QEMU_BUILD_BUG_ON(sizeof(*fw_info) != 0x50);
>
> -    if (cxl_dstate->pmem_size < (256 << 20)) {
> +    if (cxl_dstate->mem_size < (256 << 20)) {

Nit but we probably want to abstract this out (in a pre-patch), just like in the
kernel side. Ie:

#define CXL_CAPACITY_MULTIPLIER   0x10000000 /* SZ_256M */

>         return CXL_MBOX_INTERNAL_ERROR;
>     }
>
> @@ -281,9 +281,10 @@ static ret_code cmd_identify_memory_device(struct cxl_cmd 
> *cmd,
>
>     CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
>     CXLType3Class *cvc = CXL_TYPE3_GET_CLASS(ct3d);
> -    uint64_t size = cxl_dstate->pmem_size;
>
> -    if (!QEMU_IS_ALIGNED(size, 256 << 20)) {
> +    if ((!QEMU_IS_ALIGNED(cxl_dstate->mem_size, 256 << 20)) ||

is the full mem_size check here really needed?

> +        (!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, 256 << 20)) ||
> +        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, 256 << 20))) {
>         return CXL_MBOX_INTERNAL_ERROR;
>     }
>
> @@ -293,8 +294,9 @@ static ret_code cmd_identify_memory_device(struct cxl_cmd 
> *cmd,
>     /* PMEM only */

This comment wants removed.

>     snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
>
> -    id->total_capacity = size / (256 << 20);
> -    id->persistent_capacity = size / (256 << 20);
> +    id->total_capacity = cxl_dstate->mem_size / (256 << 20);
> +    id->persistent_capacity = cxl_dstate->pmem_size / (256 << 20);
> +    id->volatile_capacity = cxl_dstate->vmem_size / (256 << 20);
>     id->lsa_size = cvc->get_lsa_size(ct3d);
>
>     *len = sizeof(*id);
> @@ -312,16 +314,16 @@ static ret_code cmd_ccls_get_partition_info(struct 
> cxl_cmd *cmd,
>         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)) {
> +    if ((!QEMU_IS_ALIGNED(cxl_dstate->mem_size, 256 << 20)) ||
> +        (!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, 256 << 20)) ||
> +        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, 256 << 20))) {
>         return CXL_MBOX_INTERNAL_ERROR;
>     }
>
> -    /* PMEM only */
> -    part_info->active_vmem = 0;
> +    part_info->active_vmem = cxl_dstate->vmem_size / (256 << 20);
>     part_info->next_vmem = 0;
> -    part_info->active_pmem = size / (256 << 20);
> +    part_info->active_pmem = cxl_dstate->pmem_size / (256 << 20);
>     part_info->next_pmem = 0;
>
>     *len = sizeof(*part_info);
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 1837c1c83a..998461dac1 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -100,18 +100,47 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error 
> **errp)
>     DeviceState *ds = DEVICE(ct3d);
>     MemoryRegion *mr;
>     char *name;
> +    bool is_pmem = false;
>
> -    if (!ct3d->hostmem) {
> -        error_setg(errp, "memdev property must be set");
> +    /*
> +     * FIXME: For now we only allow a single host memory region.
> +     * Handle the deprecated memdev property usage cases
> +     */
> +    if (!ct3d->hostmem && !ct3d->host_vmem && !ct3d->host_pmem) {
> +        error_setg(errp, "at least one memdev property must be set");
>         return false;
> +    } else if (ct3d->hostmem && (ct3d->host_vmem || ct3d->host_pmem)) {
> +        error_setg(errp, "deprecated [memdev] cannot be used with new "
> +                         "persistent and volatile memdev properties");
> +        return false;
> +    } else if (ct3d->hostmem) {
> +        warn_report("memdev is deprecated and defaults to pmem. "
> +                    "Use (persistent|volatile)-memdev instead.");
> +        is_pmem = true;
> +    } else {
> +        if (ct3d->host_vmem && ct3d->host_pmem) {
> +            error_setg(errp, "Multiple memory devices not supported yet");
> +            return false;
> +        }
> +        is_pmem = !!ct3d->host_pmem;
> +        ct3d->hostmem = ct3d->host_pmem ? ct3d->host_pmem : ct3d->host_vmem;

This hides requirement details as to the necessary changes that are needed for
volatile support - for example, build_dvsecs(). Imo using two backends (without
breaking current configs, of course) should be the initial version, not 
something
to leave pending.

Thanks,
Davidlohr