[libvirt] [PATCH v3 16/17] qemu: Allow nvdimm in devices CGroups

[John Ferlan]

On 03/09/2017 11:06 AM, Michal Privoznik wrote:
> Some users might want to pass a blockdev or a chardev as a
> backend for NVDIMM. In fact, this is expected to be the mostly
> used configuration. Therefore libvirt should allow the device in
> devices CGroup then.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_cgroup.c  | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_cgroup.h  |  4 ++++
>  src/qemu/qemu_hotplug.c | 10 ++++++++++
>  3 files changed, 63 insertions(+)
> 
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 42a47a798..36762d4f6 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -348,6 +348,50 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
>  }
>  
>  
> +int
> +qemuSetupMemoryDevicesCgroup(virDomainObjPtr vm,
> +                             virDomainMemoryDefPtr mem)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    int rv;
> +
> +    if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM)
> +        return 0;
> +
> +    if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
> +        return 0;
> +
> +    VIR_DEBUG("Setting devices Cgroup for NVDIMM device: %s", mem->nvdimmPath);
> +    rv = virCgroupAllowDevicePath(priv->cgroup, mem->nvdimmPath,
> +                                  VIR_CGROUP_DEVICE_RW, false);
> +    virDomainAuditCgroupPath(vm, priv->cgroup, "allow",
> +                             mem->nvdimmPath, "rw", rv == 0);
> +
> +    return rv;
> +}
> +
> +
> +int
> +qemuTeardownMemoryDevicesCgroup(virDomainObjPtr vm,
> +                                virDomainMemoryDefPtr mem)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    int rv;
> +
> +    if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM)
> +        return 0;
> +
> +    if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
> +        return 0;
> +
> +    rv = virCgroupDenyDevicePath(priv->cgroup, mem->nvdimmPath,
> +                                 VIR_CGROUP_DEVICE_RWM, false);
> +    virDomainAuditCgroupPath(vm, priv->cgroup,
> +                             "deny", mem->nvdimmPath, "rwm", rv == 0);
> +    return rv;
> +}
> +
> +
>  static int
>  qemuSetupGraphicsCgroup(virDomainObjPtr vm,
>                          virDomainGraphicsDefPtr gfx)
> @@ -647,6 +691,11 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver,
>              goto cleanup;
>      }
>  
> +    for (i = 0; i < vm->def->nmems; i++) {
> +        if (qemuSetupMemoryDevicesCgroup(vm, vm->def->mems[i]) < 0)
> +            goto cleanup;
> +    }
> +
>      for (i = 0; i < vm->def->ngraphics; i++) {
>          if (qemuSetupGraphicsCgroup(vm, vm->def->graphics[i]) < 0)
>              goto cleanup;
> diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h
> index 8ae4a72ab..d016ce29d 100644
> --- a/src/qemu/qemu_cgroup.h
> +++ b/src/qemu/qemu_cgroup.h
> @@ -43,6 +43,10 @@ int qemuSetupHostdevCgroup(virDomainObjPtr vm,
>  int qemuTeardownHostdevCgroup(virDomainObjPtr vm,
>                                virDomainHostdevDefPtr dev)
>     ATTRIBUTE_RETURN_CHECK;
> +int qemuSetupMemoryDevicesCgroup(virDomainObjPtr vm,
> +                                 virDomainMemoryDefPtr mem);
> +int qemuTeardownMemoryDevicesCgroup(virDomainObjPtr vm,
> +                                    virDomainMemoryDefPtr mem);
>  int qemuSetupRNGCgroup(virDomainObjPtr vm,
>                         virDomainRNGDefPtr rng);
>  int qemuTeardownRNGCgroup(virDomainObjPtr vm,
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 7e19d2f82..829b40258 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2216,6 +2216,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>      const char *backendType;
>      bool objAdded = false;
>      bool teardownlabel = false;
> +    bool teardowncgroup = false;
>      virJSONValuePtr props = NULL;
>      virObjectEventPtr event;
>      int id;
> @@ -2245,6 +2246,10 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>                                    priv->qemuCaps, vm->def, mem, NULL, true) < 0)
>          goto cleanup;
>  
> +    if (qemuSetupMemoryDevicesCgroup(vm, mem) < 0)
> +        goto cleanup;
> +    teardowncgroup = true;
> +

This has the same @props leak as Patch15

>      if (qemuSecuritySetMemoryLabel(driver, vm, mem) < 0)
>          goto cleanup;
>      teardownlabel = true;
> @@ -2294,6 +2299,8 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>      virDomainAuditMemory(vm, oldmem, newmem, "update", ret == 0);
>   cleanup:
>      if (mem && ret < 0) {
> +        if (teardowncgroup && qemuTeardownMemoryDevicesCgroup(vm, mem) < 0)
> +            VIR_WARN("Unable to remove memory device cgroup ACL on hotplug fail");

FWIW: based on patch15 comments this would move too

>          if (teardownlabel && qemuSecurityRestoreMemoryLabel(driver, vm, mem) < 0)
>              VIR_WARN("Unable to restore security label on memdev");
>      }
> @@ -3761,6 +3768,9 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
>      if (qemuSecurityRestoreMemoryLabel(driver, vm, mem) < 0)
>          VIR_WARN("Unable to restore security label on memdev");
>  
> +    if (qemuTeardownMemoryDevicesCgroup(vm, mem) < 0)
> +        VIR_WARN("Unable to remove memory device cgroup ACL");
> +

The order here is different than attach failure path of teardown cgroup,
restore label

If there's a relationship between them, then order could be important.

ACK in principle and assuming you're following patch15 adjustments anyway.

John

>      virDomainMemoryDefFree(mem);
>  
>      /* fix the balloon size */
>