Re: [PATCH v2 19/27] qemu: Build command line for virtio-pmem

[Peter Krempa <pkrempa@xxxxxxxxxx>]

On Thu, Dec 03, 2020 at 13:36:22 +0100, Michal Privoznik wrote:
> Now we have everything prepared for generating the command line.
> The device alias prefix was chosen to be 'virtiopmem'.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1735375
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
>  src/qemu/qemu_alias.c                         | 44 ++++++----
>  src/qemu/qemu_command.c                       | 82 ++++++++++---------
>  ...ory-hotplug-virtio-pmem.x86_64-latest.args | 45 ++++++++++
>  tests/qemuxml2argvtest.c                      |  1 +
>  4 files changed, 120 insertions(+), 52 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/memory-hotplug-virtio-pmem.x86_64-latest.args
> 
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index 931dbd4e84..68d4a7b480 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -466,6 +466,28 @@ qemuAssignDeviceRNGAlias(virDomainDefPtr def,
>  }
>  
>  
> +static int
> +qemuDeviceMemoryGetAliasID(virDomainDefPtr def,
> +                           virDomainMemoryDefPtr mem,
> +                           bool oldAlias,
> +                           const char *prefix)
> +{
> +    size_t i;
> +    int maxidx = 0;
> +
> +    if (!oldAlias)
> +        return mem->info.addr.dimm.slot;

If oldAlias is false this returns 'info.addr.dimm.slot' ...

> +
> +    for (i = 0; i < def->nmems; i++) {
> +        int idx;
> +        if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, prefix)) >= maxidx)
> +            maxidx = idx + 1;
> +    }
> +
> +    return maxidx;
> +}
> +
> +
>  /**
>   * qemuAssignDeviceMemoryAlias:
>   * @def: domain definition. Necessary only if @oldAlias is true.
> @@ -483,10 +505,8 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def,
>                              virDomainMemoryDefPtr mem,
>                              bool oldAlias)
>  {
> -    size_t i;
> -    int maxidx = 0;
> -    int idx;
>      const char *prefix = NULL;
> +    int idx = 0;
>  
>      if (mem->info.alias)
>          return 0;
> @@ -494,26 +514,22 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def,
>      switch (mem->model) {
>      case VIR_DOMAIN_MEMORY_MODEL_DIMM:
>          prefix = "dimm";
> +        idx = qemuDeviceMemoryGetAliasID(def, mem, oldAlias, prefix);
>          break;
>      case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
>          prefix = "nvdimm";
> +        idx = qemuDeviceMemoryGetAliasID(def, mem, oldAlias, prefix);
>          break;
>      case VIR_DOMAIN_MEMORY_MODEL_VIRTIO:
> +        prefix = "virtiopmem";
> +        idx = qemuDeviceMemoryGetAliasID(def, mem, true, prefix);

... and you use it like that here, but virtio-pmem is a PCI device.

> +        break;
>      case VIR_DOMAIN_MEMORY_MODEL_NONE:
>      case VIR_DOMAIN_MEMORY_MODEL_LAST:

[...]

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 49241fc507..501deff1ee 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c

[...]

> @@ -3143,8 +3149,8 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
>          backendType = "memory-backend-ram";
>      }
>  
> -    if (!priv->memPrealloc &&
> -        virJSONValueObjectAdd(props, "B:prealloc", prealloc, NULL) < 0)
> +    if (allowPrealloc &&
> +        virJSONValueObjectAdd(props, "B:prealloc", wantPrealloc, NULL) < 0)
>          return -1;

We can theoretically use a virTristate* and use 'T' type here instead of
having two variables.


[...]

> @@ -3308,46 +3314,46 @@ qemuBuildMemoryDeviceStr(const virDomainDef *def,
>      }
>  
>      switch (mem->model) {
> -    case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
>      case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> -
> -        if (mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM)
> -            device = "pc-dimm";
> -        else
> -            device = "nvdimm";
> -
> -        virBufferAsprintf(&buf, "%s,", device);
> -
> -        if (mem->targetNode >= 0)
> -            virBufferAsprintf(&buf, "node=%d,", mem->targetNode);
> -
> -        if (mem->labelsize)
> -            virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024);
> -
> -        if (virUUIDIsValid(mem->uuid)) {
> -            char uuidstr[VIR_UUID_STRING_BUFLEN];
> -
> -            virUUIDFormat(mem->uuid, uuidstr);
> -            virBufferAsprintf(&buf, "uuid=%s,", uuidstr);
> -        }
> -
> -        if (mem->readonly) {
> -            virBufferAddLit(&buf, "unarmed=on,");
> -        }
> -
> -        virBufferAsprintf(&buf, "memdev=mem%s,id=%s",
> -                          mem->info.alias, mem->info.alias);
> -
> -        qemuBuildDeviceAddressStr(&buf, def, &mem->info, qemuCaps);
> +        device = "pc-dimm";
> +        break;
> +    case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> +        device = "nvdimm";
>          break;
>  
>      case VIR_DOMAIN_MEMORY_MODEL_VIRTIO:
> +        device = "virtio-pmem-pci";
> +
>      case VIR_DOMAIN_MEMORY_MODEL_NONE:
>      case VIR_DOMAIN_MEMORY_MODEL_LAST:
>          break;
> +    }
> +
> +    virBufferAsprintf(&buf, "%s,", device);
> +
> +    if (mem->targetNode >= 0)
> +        virBufferAsprintf(&buf, "node=%d,", mem->targetNode);
> +
> +    if (mem->labelsize)
> +        virBufferAsprintf(&buf, "label-size=%llu,", mem->labelsize * 1024);
> +
> +    if (virUUIDIsValid(mem->uuid)) {
> +        char uuidstr[VIR_UUID_STRING_BUFLEN];
>  
> +        virUUIDFormat(mem->uuid, uuidstr);
> +        virBufferAsprintf(&buf, "uuid=%s,", uuidstr);
>      }
>  
> +    if (mem->readonly) {
> +        virBufferAddLit(&buf, "unarmed=on,");
> +    }
> +
> +    virBufferAsprintf(&buf, "memdev=mem%s,id=%s",
> +                      mem->info.alias, mem->info.alias);
> +
> +    qemuBuildDeviceAddressStr(&buf, def, &mem->info, qemuCaps);
> +
> +
>      return virBufferContentAndReset(&buf);
>  }

Some of these seem to be specific for some devices (such as unarmed is
relevant only for nvdimm)