Re: [libvirt PATCHv3 08/12] qemu: validate virtiofs filesystems

[Masayoshi Mizuma <msys.mizuma@xxxxxxxxx>]

On Thu, Jan 30, 2020 at 06:06:24PM +0100, Ján Tomko wrote:
> Reject unsupported configurations.
> 
> Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx>
> ---
>  src/qemu/qemu_domain.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 6367f5394e..b5d5812ff8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8314,8 +8314,32 @@ qemuDomainDeviceDefValidateFS(virDomainFSDefPtr fs,
>          return -1;
>  
>      case VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS:
> -        /* TODO: vhost-user-fs-pci */
> -        return 0;
> +        if (fs->accessmode != VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("virtiofs only supports passthrough accessmode"));
> +            return -1;
> +        }
> +        if (fs->wrpolicy != VIR_DOMAIN_FS_WRPOLICY_DEFAULT) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("virtiofs does not support wrpolicy"));
> +            return -1;
> +        }
> +        if (fs->model != VIR_DOMAIN_FS_MODEL_DEFAULT) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("virtiofs does not support model"));
> +            return -1;
> +        }
> +        if (fs->format != VIR_STORAGE_FILE_NONE) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("virtiofs does not support format"));
> +            return -1;
> +        }

> +        if (def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_SHARED) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("virtiofs requires shared memory"));
> +            return -1;
> +        }

This requires the following <memoryBacking> config, right?

  <memoryBacking>
    <access mode='shared'/>
  </memoryBacking>

I think the <memoryBacking> isn't mandatory because the memory access policy
can be over written by <numa> config like as:

    <numa>
      <cell id='0' cpus='0-31' memory='8912896' unit='KiB' memAccess='shared'/>
    </numa>

And, I suppose virtiofsd requires one or more numa nodes.

How about adding following function to check the mem
config instead of the if statement?

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 493864b854..b3b5cb2054 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8203,6 +8203,39 @@ qemuDomainDeviceDefValidateIOMMU(const virDomainIOMMUDef *iommu,
     return 0;
 }

+static int
+memconfIsSufficientForVirtiofs(const virDomainDef *def)
+{
+    virDomainNumaPtr numa = def->numa;
+    virDomainMemoryAccess memAccess = def->mem.access;
+    virDomainMemoryAccess NodememAccess;
+    size_t nodes;
+    size_t i;
+
+    nodes = virDomainNumaGetNodeCount(numa);
+    if (!nodes) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("virtiofs requires one or more numa nodes"));
+        return 0;
+    }
+
+    for (i = 0; i < nodes; i++) {
+        NodememAccess = virDomainNumaGetNodeMemoryAccessMode(numa, i);
+        if (NodememAccess == VIR_DOMAIN_MEMORY_ACCESS_PRIVATE) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("virtiofs requires shared memory for node[%ld]"), i);
+	    return 0;
+        } else if ((NodememAccess == VIR_DOMAIN_MEMORY_ACCESS_DEFAULT) &&
+                   (memAccess != VIR_DOMAIN_MEMORY_ACCESS_SHARED)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("virtiofs requires shared memory for node[%ld]"), i);
+            return 0;
+	}
+    }
+
+    return 1;
+}
+

 static int
 qemuDomainDeviceDefValidateFS(virDomainFSDefPtr fs,
@@ -8256,9 +8289,7 @@ qemuDomainDeviceDefValidateFS(virDomainFSDefPtr fs,
                            _("virtiofs does not support format"));
             return -1;
         }
-        if (def->mem.access != VIR_DOMAIN_MEMORY_ACCESS_SHARED) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("virtiofs requires shared memory"));
+        if (!memconfIsSufficientForVirtiofs(def)) {
             return -1;
         }
         break;
---

Thanks,
Masa