On Thu, 6 Oct 2022 19:37:02 -0400
Gregory Price <gourry.memve...@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.pr...@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;
> }
>
