Re: [PATCH v2 3/3] nvdimm: honor -object memory-backend-file, readonly=on option

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 16 Sep 2020 10:51:50 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Make it possible to present read-only files to the guest as "unarmed"
> NVDIMMs. The Linux NVDIMM device (/dev/pmemX) is read-only.
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/nvdimm.txt | 8 +++++++-
>  hw/mem/nvdimm.c | 4 ++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
> index c2c6e441b3..06c2008107 100644
> --- a/docs/nvdimm.txt
> +++ b/docs/nvdimm.txt
> @@ -17,7 +17,7 @@ following command line options:
>  
>   -machine pc,nvdimm
>   -m $RAM_SIZE,slots=$N,maxmem=$MAX_SIZE
> - -object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE
> + -object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE,readonly=off
>   -device nvdimm,id=nvdimm1,memdev=mem1
>  
>  Where,
> @@ -42,6 +42,12 @@ Where,
>     "share=off", then guest writes won't be applied to the backend
>     file and thus will be invisible to other guests.
>  
> +   "readonly=on/off" controls whether the file $PATH is opened read-only or
> +   read/write (default). "readonly=on" sets the ACPI NFIT NVDIMM Region Mapping
> +   Structure "NVDIMM State Flags" Bit 3 indicating that the device is "unarmed"
> +   and cannot accept persistent writes. Linux guest drivers set the device to
> +   read-only when this bit is present.
> +
>   - "device nvdimm,id=nvdimm1,memdev=mem1" creates a virtual NVDIMM
>     device whose storage is provided by above memory backend device.
>  
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index e1574bc07c..848cd65917 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -146,6 +146,10 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
>          return;
>      }
>  
> +    if (memory_region_is_rom(mr)) {
> +        nvdimm->unarmed = true; /* this device is read-only */
> +    }
we probably should error out in case 'backend,readonly=true + nvdimm,unarmed=false'
instead of silently ignoring user's input

>      nvdimm->nvdimm_mr = g_new(MemoryRegion, 1);
>      memory_region_init_alias(nvdimm->nvdimm_mr, OBJECT(dimm),
>                               "nvdimm-memory", mr, 0, pmem_size);