Re: [PATCH v3 05/14] conf: Introduce virtio-mem <memory/> model

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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>




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux