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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Thu,  6 Oct 2022 19:37:02 -0400
Gregory Price <gourry.memverge@gmail.com> wrote:

> This commit enables setting one memory region for a type-3 device, but
> that region may now be either a persistent region or volatile region.
> 
> Future work may enable setting both regions simultaneously, as this is
> a possible configuration on a real type-3 device.  The scaffolding was
> put in for this, but is left for further work.
> 
> The existing [memdev] property has been deprecated and will default the
> memory region to a persistent memory region (although a user may assign
> the region to a ram or file backed region).
> 
> There is presently no interface with which to expose a MemoryRegion's
> real backing type (HostMemoryBackendRam/File), otherwise we could have
> used File to imply pmem (or inspected HostMemoryBackendFile->is_pmem) to
> deterine whether the backing device supported pmem mode.  This should be
> considered for future work, as it would make creating an array of
> HostMemory devices to represent DIMMs on a Single-Logical-Device
> MemoryExpander fairly straightforward.
> 
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> ---
Looks good to me, though I haven't tested it yet.

A few trivial comments inline.

>      *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.

TODO maybe?  FIXME tends to lead to reviewers asking why we haven't
fixed it yet!

> +     * 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.");

I'd suggest we don't warn on this yet. There is limited advantage in
doing so given it's easy to carry on supporting by just treating
it as another name for persistent-memdev


> +        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;
>      }
>