Re: [Qemu-devel] [PATCH v2 3/4] nvdimm: add a boolean option "restrict"

[Michael S. Tsirkin]

On Tue, Jun 06, 2017 at 03:22:28PM +0800, Haozhong Zhang wrote:
> If a vNVDIMM device is not backed by a DAX device and its "restrict"
> option is enabled, bit 3 of state flags in its region mapping
> structure will be set, in order to notify the guest of the lack of
> write persistence guarantee. Once this bit is set, the guest OS may
> mark the vNVDIMM device as read-only.
> 
> This option is disabled by default for backwards compatibility. It's
> recommended to enable for the formal usage.
> 
> Signed-off-by: Haozhong Zhang <address@hidden>

Seems wrong to me. E.g. it won't work in a nested
virt setup. What if backend is dax but is not armed?
Can't the armed bit of the backing device be tested?
Name "restrict" is also confusing. Can we reuse cache=
options? E.g. cache=unsafe etc.

> ---
>  hw/acpi/nvdimm.c        | 16 ++++++++++++++++
>  hw/mem/nvdimm.c         | 38 +++++++++++++++++++++++++++++++++++++-
>  include/hw/mem/nvdimm.h |  5 +++++
>  3 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 8e7d6ec034..fd1ef6dc65 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -138,6 +138,8 @@ struct NvdimmNfitMemDev {
>  } QEMU_PACKED;
>  typedef struct NvdimmNfitMemDev NvdimmNfitMemDev;
>  
> +#define ACPI_NFIT_MEM_NOT_ARMED    (1 << 3)
> +
>  /*
>   * NVDIMM Control Region Structure
>   *
> @@ -289,6 +291,10 @@ nvdimm_build_structure_memdev(GArray *structures, 
> DeviceState *dev)
>      int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
>                                              NULL);
>      uint32_t handle = nvdimm_slot_to_handle(slot);
> +    bool dev_dax = object_property_get_bool(OBJECT(dev), NVDIMM_DEV_DAX_PROP,
> +                                            NULL);
> +    bool restrict_mode = object_property_get_bool(OBJECT(dev),
> +                                                  NVDIMM_RESTRICT_PROP, 
> NULL);
>  
>      nfit_memdev = acpi_data_push(structures, sizeof(*nfit_memdev));
>  
> @@ -312,6 +318,16 @@ nvdimm_build_structure_memdev(GArray *structures, 
> DeviceState *dev)
>  
>      /* Only one interleave for PMEM. */
>      nfit_memdev->interleave_ways = cpu_to_le16(1);
> +
> +    /*
> +     * If a vNVDIMM device in the restrict mode and is not backed by a
> +     * DAX device, QEMU will set ACPI_NFIT_MEM_NOT_ARMED bit of state
> +     * flags in its region mapping structure, in order to notify the
> +     * guest of the lack of write persistence guarantee.
> +     */
> +    if (!dev_dax && restrict_mode) {
> +        nfit_memdev->flags = cpu_to_le16(ACPI_NFIT_MEM_NOT_ARMED);
> +    }
>  }
>  
>  /*
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index b23542fbdf..cda416e5c8 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -65,11 +65,46 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> +static bool nvdimm_get_backend_dev_dax(Object *obj, Error **errp)
> +{
> +    NVDIMMDevice *nvdimm = NVDIMM(obj);
> +
> +    return nvdimm->backend_dev_dax;
> +}
> +
> +static bool nvdimm_get_restrict(Object *obj, Error **errp)
> +{
> +    NVDIMMDevice *nvdimm = NVDIMM(obj);
> +
> +    return nvdimm->restrict_mode;
> +}
> +
> +static void nvdimm_set_restrict(Object *obj, bool val, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    NVDIMMDevice *nvdimm = NVDIMM(obj);
> +    Error *local_err = NULL;
> +
> +    if (dev->realized) {
> +        error_setg(&local_err, "cannot change property value");
> +        goto out;
> +    }
> +
> +    nvdimm->restrict_mode = val;
> +
> + out:
> +    error_propagate(errp, local_err);
> +}
> +
>  static void nvdimm_init(Object *obj)
>  {
>      object_property_add(obj, NVDIMM_LABEL_SIZE_PROP, "int",
>                          nvdimm_get_label_size, nvdimm_set_label_size, NULL,
>                          NULL, NULL);
> +    object_property_add_bool(obj, NVDIMM_DEV_DAX_PROP,
> +                             nvdimm_get_backend_dev_dax, NULL, NULL);
> +    object_property_add_bool(obj, NVDIMM_RESTRICT_PROP,
> +                             nvdimm_get_restrict, nvdimm_set_restrict, NULL);
>  }
>  
>  static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
> @@ -85,7 +120,8 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error 
> **errp)
>      NVDIMMDevice *nvdimm = NVDIMM(dimm);
>      uint64_t align, pmem_size, size = memory_region_size(mr);
>  
> -    if (!qemu_fd_is_dev_dax(memory_region_get_fd(mr))) {
> +    nvdimm->backend_dev_dax = qemu_fd_is_dev_dax(memory_region_get_fd(mr));
> +    if (!nvdimm->backend_dev_dax) {
>          error_report("warning: nvdimm backend does not look like a DAX 
> device, "
>                       "unable to guarantee persistence of guest writes");
>      }
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index f1f3987055..2fbe0d7858 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -49,6 +49,8 @@
>                                                 TYPE_NVDIMM)
>  
>  #define NVDIMM_LABEL_SIZE_PROP "label-size"
> +#define NVDIMM_DEV_DAX_PROP    "dev-dax"
> +#define NVDIMM_RESTRICT_PROP   "restrict"
>  
>  struct NVDIMMDevice {
>      /* private */
> @@ -74,6 +76,9 @@ struct NVDIMMDevice {
>       * guest via ACPI NFIT and _FIT method if NVDIMM hotplug is supported.
>       */
>      MemoryRegion nvdimm_mr;
> +
> +    bool backend_dev_dax;
> +    bool restrict_mode;
>  };
>  typedef struct NVDIMMDevice NVDIMMDevice;
>  
> -- 
> 2.11.0