On Fri, Apr 23, 2021 at 15:24:27 +0200, Michal Privoznik wrote:
[...]
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 1b9b221611..44e478c88a 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
[...]
> @@ -7793,7 +7807,8 @@ Example: usage of the memory devices
> added memory from the perspective of the guest.
>
> The mandatory ``size`` subelement configures the size of the added memory as
> - a scaled integer.
> + a scaled integer. For ``virtio-mem`` this represents the maximum possible
> + size exposed to the guest.
>
> The ``node`` subelement configures the guest NUMA node to attach the memory
> to. The element shall be used only if the guest has NUMA nodes configured.
> @@ -7820,6 +7835,17 @@ Example: usage of the memory devices
> so other backend types should use the ``readonly`` element. :since:`Since
> 5.0.0`
>
> + ``block``
> + For ``virtio-mem`` only.
> + The size of an individual block, granularity of division of memory module.
I'd refrain from using 'memory module' as this is specifically
paravirtualized -> no modules.
> + Must be power of two and at least equal to size of a transparent hugepage
> + (2MiB on x84_64). The default is hypervisor dependent.
> +
> + ``requested``
> + For ``virtio-mem`` only.
> + The total size exposed to the guest. Must respect ``block`` granularity
> + and be smaller or equal to ``size``.
> +
> :anchor:`<a id="elementsIommu"/>`
>
> IOMMU devices
[...]
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 686b9e8d16..16fa65bc6b 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
[...]
> @@ -1896,6 +1899,42 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem,
> _("virtio-pmem does not support NUMA nodes"));
> return -1;
> }
> + break;
> +
> + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
> + if (mem->requestedsize > mem->size) {
> + virReportError(VIR_ERR_XML_DETAIL, "%s",
> + _("requested size must be smaller than @size"));
> + return -1;
> + }
I'd expect that 'mem->requestedsize == mem->size' is also okay otherwise
it'll rob you from using the last block.
> +
> + if (!VIR_IS_POW2(mem->blocksize)) {
> + virReportError(VIR_ERR_XML_DETAIL, "%s",
> + _("block size must be a power of two"));
> + return -1;
> + }
> +
> + if (virHostMemGetTHPSize(&thpSize) < 0) {
> + /* We failed to get THP size, fall back to a sane default. On
> + * almost every architecture the size will be 2MiB, except for some
> + * funky arches like sparc and m68k. Use 2MiB and refine later if
> + * somebody complains. */
> + thpSize = 2048;
> + }
[...]
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index 63638b1402..e1caeec006 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -523,6 +523,7 @@ qemuAssignDeviceMemoryAlias(virDomainDef *def,
> case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM:
> prefix = "virtiopmem";
> break;
> + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
> case VIR_DOMAIN_MEMORY_MODEL_NONE:
> case VIR_DOMAIN_MEMORY_MODEL_LAST:
> default:
In this hunk you refrain from adding qemuisms and they are added
later, but all the following hunks add qemu-specific details.
Specifically the last one adding qemuCaps check is questionable.
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 6e3e3555c7..c182b47f38 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8968,6 +8968,16 @@ qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem,
> needsNuma = false;
> break;
>
> + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
> + if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> + mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("only 'pci' addresses are supported for the %s device"),
> + virDomainMemoryModelTypeToString(mem->model));
> + return -1;
> + }
> + break;
> +
> case VIR_DOMAIN_MEMORY_MODEL_NONE:
> case VIR_DOMAIN_MEMORY_MODEL_LAST:
> return -1;
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 1ee75b8f2e..c632b055f1 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -380,9 +380,18 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDef *def,
> }
>
> for (i = 0; i < def->nmems; i++) {
> - if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM &&
> - def->mems[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> - def->mems[i]->info.type = type;
> + switch (def->mems[i]->model) {
> + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM:
> + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
> + if (def->mems[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> + def->mems[i]->info.type = type;
> + break;
> + case VIR_DOMAIN_MEMORY_MODEL_NONE:
> + case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> + case VIR_DOMAIN_MEMORY_MODEL_LAST:
> + break;
> + }
> }
>
> if (type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
[...]
> @@ -2439,12 +2449,19 @@ qemuDomainAssignDevicePCISlots(virDomainDef *def,
> for (i = 0; i < def->nmems; i++) {
> virDomainMemoryDef *mem = def->mems[i];
>
> - if (mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM ||
> - !virDeviceInfoPCIAddressIsWanted(&mem->info))
> - continue;
> -
> - if (qemuDomainPCIAddressReserveNextAddr(addrs, &mem->info) < 0)
> - return -1;
> + switch (mem->model) {
> + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM:
> + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
> + if (virDeviceInfoPCIAddressIsWanted(&mem->info) &&
> + qemuDomainPCIAddressReserveNextAddr(addrs, &mem->info) < 0)
> + return -1;
> + break;
> + case VIR_DOMAIN_MEMORY_MODEL_NONE:
> + case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> + case VIR_DOMAIN_MEMORY_MODEL_LAST:
> + break;
> + }
> }
>
> return 0;
[...]
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 255d653118..b02b33a0f1 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -4895,6 +4895,14 @@ qemuValidateDomainDeviceDefMemory(virDomainMemoryDef *mem,
> }
> break;
>
> + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("virtio-mem isn't supported by this QEMU binary"));
> + return -1;
> + }
> + break;
> +
> case VIR_DOMAIN_MEMORY_MODEL_NONE:
> case VIR_DOMAIN_MEMORY_MODEL_LAST:
> break;
With the nits addressed:
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
