[Qemu-devel] [PATCH 0/2] add "nopin" option in the memory-backend-file

[Qemu-devel] [PATCH 0/2] add "nopin" option in the memory-backend-file

[Yang Zhong]

Qemu does not need pin NVDIMM memory for VFIO device during VFIO
hotplug, what's more, if there is no NVDIMM hw in the test machine,
the VFIO hotplug operation will need at least 10 minutes to pin RAM
as the NVDIMM, this time is not accepted. So we add "nopin=on" option
in the memory-backed-file, which can avoid to pin RAM memory for NVDIMM.

The new command like below:
-object memory-backend-file,id=mem0,share,nopin=on,mem-path=kvm.img,size=9161408512 

The default of "nopin" still "off" value, which is same with previous value.

Yang Zhong (2):
  hostmem-file: Add "nopin" option for memory-backend-file
  nvdimm: Add "nopin" for related documents

 backends/hostmem-file.c | 23 +++++++++++++++++++++++
 docs/nvdimm.txt         | 10 ++++++++--
 hw/vfio/common.c        | 12 +++++++++++-
 qemu-options.hx         |  6 +++++-
 4 files changed, 47 insertions(+), 4 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] hostmem-file: Add "nopin" option for memory-backend-file

[Yang Zhong]

Since qemu does not need pin nvdimm memory during the VFIO
hotplug, the new option can be used to avoid pin whole nvdimm
memory. The default value is still "nopin=off" as previous.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 backends/hostmem-file.c | 23 +++++++++++++++++++++++
 hw/vfio/common.c        | 12 +++++++++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index e44c319..e402077 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -33,6 +33,7 @@ struct HostMemoryBackendFile {
 
     bool share;
     bool discard_data;
+    bool nopin;
     char *mem_path;
 };
 
@@ -128,6 +129,25 @@ static void file_backend_unparent(Object *obj)
     }
 }
 
+static bool file_memory_backend_get_nopin(Object *o, Error **errp)
+{
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
+
+    return fb->nopin;
+}
+
+static void file_memory_backend_set_nopin(Object *o, bool value, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(o);
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
+
+    if (memory_region_size(&backend->mr)) {
+        error_setg(errp, "cannot change property value");
+        return;
+    }
+    fb->nopin = value;
+}
+
 static void
 file_backend_class_init(ObjectClass *oc, void *data)
 {
@@ -142,6 +162,9 @@ file_backend_class_init(ObjectClass *oc, void *data)
     object_class_property_add_bool(oc, "discard-data",
         file_memory_backend_get_discard_data, file_memory_backend_set_discard_data,
         &error_abort);
+    object_class_property_add_bool(oc, "nopin",
+        file_memory_backend_get_nopin, file_memory_backend_set_nopin,
+        &error_abort);
     object_class_property_add_str(oc, "mem-path",
         get_mem_path, set_mem_path,
         &error_abort);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 7b2924c..f36ff24 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -408,7 +408,8 @@ static void vfio_listener_region_add(MemoryListener *listener,
     void *vaddr;
     int ret;
     VFIOHostDMAWindow *hostwin;
-    bool hostwin_found;
+    bool hostwin_found, nopin;
+    Object *obj = section->mr->owner;
 
     if (vfio_listener_skipped_section(section)) {
         trace_vfio_listener_region_add_skip(
@@ -424,6 +425,15 @@ static void vfio_listener_region_add(MemoryListener *listener,
         return;
     }
 
+    if (obj && object_dynamic_cast(obj, "memory-backend-file")) {
+        nopin = object_property_get_bool(obj, "nopin", NULL);
+        if (nopin) {
+            error_report("warning: If VFIO DMA still map to NVDIMM memory, "
+                         "the VM will crash");
+            return;
+        }
+    }
+
     iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
     llend = int128_make64(section->offset_within_address_space);
     llend = int128_add(llend, section->size);
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] nvdimm: Add "nopin" for related documents

[Yang Zhong]

Added the "nopin" related changes in nvdimm.txt and
qemu-options.hx.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 docs/nvdimm.txt | 10 ++++++++--
 qemu-options.hx |  6 +++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
index 2d9f8c0..41ac1c2 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,nopin=on,mem-path=$PATH,size=$NVDIMM_SIZE
  -device nvdimm,id=nvdimm1,memdev=mem1
 
 Where,
@@ -31,7 +31,7 @@ Where,
    of normal RAM devices and vNVDIMM devices, e.g. $MAX_SIZE should be
    >= $RAM_SIZE + $NVDIMM_SIZE here.
 
- - "object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE"
+ - "object memory-backend-file,id=mem1,share=on,nopin=on,mem-path=$PATH,size=$NVDIMM_SIZE"
    creates a backend storage of size $NVDIMM_SIZE on a file $PATH. All
    accesses to the virtual NVDIMM device go to the file $PATH.
 
@@ -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.
 
+   "nopin=on/off" controls the memory pining of memory backend file
+    during the VFIO device hotplug. If "nopin=on", then VFIO device
+    hotplug can skip the NVDIMM memory pining because qemu does not
+    need pining NVDIMM memory for devices. If "nopin=off", the VFIO
+    device hotplug need NVDIMM memory pining.
+
  - "device nvdimm,id=nvdimm1,memdev=mem1" creates a virtual NVDIMM
    device whose storage is provided by above memory backend device.
 
diff --git a/qemu-options.hx b/qemu-options.hx
index 981742d..d21ce2e 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4173,7 +4173,7 @@ property must be set.  These objects are placed in the
 
 @table @option
 
-@item -object memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off},discard-data=@var{on|off}
+@item -object memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off},discard-data=@var{on|off},nopin=@var{on|off}
 
 Creates a memory file backend object, which can be used to back
 the guest RAM with huge pages. The @option{id} parameter is a
@@ -4191,6 +4191,10 @@ to avoid unnecessarily flushing data to the backing file.  Note
 that @option{discard-data} is only an optimization, and QEMU
 might not discard file contents if it aborts unexpectedly or is
 terminated using SIGKILL.
+Setting the @option{nopin} boolean option to @var{on} indicates
+that if the memory-backend-file is nvdimm or else, the memory
+pining can be disable during VFIO device hotplug. The default
+nopin is still off, which is same with previous value.
 
 @item -object rng-random,id=@var{id},filename=@var{/dev/random}
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/2] add "nopin" option in the memory-backend-file

[Xiao Guangrong]

On 10/16/2017 04:56 PM, Yang Zhong wrote:
> Qemu does not need pin NVDIMM memory for VFIO device during VFIO
> hotplug, what's more, if there is no NVDIMM hw in the test machine,
> the VFIO hotplug operation will need at least 10 minutes to pin RAM
> as the NVDIMM, this time is not accepted. So we add "nopin=on" option
> in the memory-backed-file, which can avoid to pin RAM memory for NVDIMM.

No.

memory-backed-file does not dedicate for nvdimm only, it can be mapped
as normal memory as well. Rather more, this is no way to stop guest to
use it as DMA.

Re: [Qemu-devel] [PATCH 0/2] add "nopin" option in the memory-backend-file

[Paolo Bonzini]

On 16/10/2017 12:20, Xiao Guangrong wrote:
> 
>> Qemu does not need pin NVDIMM memory for VFIO device during VFIO
>> hotplug, what's more, if there is no NVDIMM hw in the test machine,
>> the VFIO hotplug operation will need at least 10 minutes to pin RAM
>> as the NVDIMM, this time is not accepted. So we add "nopin=on" option
>> in the memory-backed-file, which can avoid to pin RAM memory for NVDIMM.
> 
> No.
> 
> memory-backed-file does not dedicate for nvdimm only, it can be mapped
> as normal memory as well. Rather more, this is no way to stop guest to
> use it as DMA.

Right, so a better name for the object property could be "dma" rather
than "nopin".  I'll let others comment on whether MemoryBackend (not
just memory-backend-file) is the right place for the option.

I am also not sure whether VFIO is not the right place for the "other
side" of the hook.  If you add the memory region to the CPU address
space and not the PCI address space, you can hide it from all PCI devices.

Paolo

Re: [Qemu-devel] [PATCH 0/2] add "nopin" option in the memory-backend-file

[Alex Williamson]

On Mon, 16 Oct 2017 16:56:21 +0800
Yang Zhong <yang.zhong@intel.com> wrote:

> Qemu does not need pin NVDIMM memory for VFIO device during VFIO
> hotplug, what's more, if there is no NVDIMM hw in the test machine,
> the VFIO hotplug operation will need at least 10 minutes to pin RAM
> as the NVDIMM, this time is not accepted. So we add "nopin=on" option
> in the memory-backed-file, which can avoid to pin RAM memory for NVDIMM.
> 
> The new command like below:
> -object memory-backend-file,id=mem0,share,nopin=on,mem-path=kvm.img,size=9161408512 
> 
> The default of "nopin" still "off" value, which is same with previous value.


If an NVDIMM is not a possible DMA target for a VFIO assigned device
then it should be in a different AddressSpace from the device.  If an
NVDIMM can be a DMA target then it's the correct thing to do to pin it
through the IOMMU for a VFIO device even if it might take considerable
time to do so.  Thanks,

Alex

Re: [Qemu-devel] [PATCH 0/2] add "nopin" option in the memory-backend-file

[Zhong Yang]

On Mon, Oct 16, 2017 at 06:20:32PM +0800, Xiao Guangrong wrote:
> 
> 
> On 10/16/2017 04:56 PM, Yang Zhong wrote:
> >Qemu does not need pin NVDIMM memory for VFIO device during VFIO
> >hotplug, what's more, if there is no NVDIMM hw in the test machine,
> >the VFIO hotplug operation will need at least 10 minutes to pin RAM
> >as the NVDIMM, this time is not accepted. So we add "nopin=on" option
> >in the memory-backed-file, which can avoid to pin RAM memory for NVDIMM.
> 
> No.
> 
> memory-backed-file does not dedicate for nvdimm only, it can be mapped
> as normal memory as well. Rather more, this is no way to stop guest to
> use it as DMA.

Guangrong, thanks for your comments.

This issue was found in ClearContainer test and the general Qemu should also face
same issue. If test machine does not have nvdimm hw and RAM is limitted, the
VFIO hotplug or SRIOV will cost some minutes to preallocate memory for nvdimm, 
even the hotplug faild because of RAM is not enough.

We want to find a way to avoid this kind of issue, so the configurable "nopin" is added,
but the default operation is not changed, the nvdimm still can be DMA target. thanks!

Regards,

Yang
  

Re: [Qemu-devel] [PATCH 0/2] add "nopin" option in the memory-backend-file

[Zhong Yang]

On Mon, Oct 16, 2017 at 01:58:59PM +0200, Paolo Bonzini wrote:
> On 16/10/2017 12:20, Xiao Guangrong wrote:
> > 
> >> Qemu does not need pin NVDIMM memory for VFIO device during VFIO
> >> hotplug, what's more, if there is no NVDIMM hw in the test machine,
> >> the VFIO hotplug operation will need at least 10 minutes to pin RAM
> >> as the NVDIMM, this time is not accepted. So we add "nopin=on" option
> >> in the memory-backed-file, which can avoid to pin RAM memory for NVDIMM.
> > 
> > No.
> > 
> > memory-backed-file does not dedicate for nvdimm only, it can be mapped
> > as normal memory as well. Rather more, this is no way to stop guest to
> > use it as DMA.
> 
> Right, so a better name for the object property could be "dma" rather
> than "nopin".  I'll let others comment on whether MemoryBackend (not
> just memory-backend-file) is the right place for the option.
  
  Paolo, thanks for your comments!
 
  Yes, i can change this property from "nopin" to "dma" in memory-backend
  (not in ,memmory-backed-file). thanks!

  Yang

> I am also not sure whether VFIO is not the right place for the "other
> side" of the hook.  If you add the memory region to the CPU address
> space and not the PCI address space, you can hide it from all PCI devices.
> 
> Paolo
  thanks for your suggestion!

  Your suggestion can avoid DMA target to nvdimm if the nvdimm memory region
  was skipped during VFIO hotplug. It is valuable to try this solution. by the
  way, please share me some clue for PCI address space related with memory region,
  below address_space_mem is right?  Many thanks!

  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
                         MemoryRegion *address_space_mem,
                         MemoryRegion *address_space_io,
                         uint8_t devfn_min)  
  
  Regards,

  Yang

Re: [Qemu-devel] [PATCH 0/2] add "nopin" option in the memory-backend-file

[Paolo Bonzini]

On 18/10/2017 07:57, Zhong Yang wrote:
>   Your suggestion can avoid DMA target to nvdimm if the nvdimm memory region
>   was skipped during VFIO hotplug. It is valuable to try this solution. by the
>   way, please share me some clue for PCI address space related with memory region,
>   below address_space_mem is right?  Many thanks!
> 
>   static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>                          MemoryRegion *address_space_mem,
>                          MemoryRegion *address_space_io,
>                          uint8_t devfn_min)  

Yes.  You would have to add the region directly to the CPU address
space, and not to address_space_memory (which is included in both the
CPU and PCI address spaces).

Paolo

Re: [Qemu-devel] [PATCH 0/2] add "nopin" option in the memory-backend-file

[Zhong Yang]

On Wed, Oct 18, 2017 at 11:48:11AM +0200, Paolo Bonzini wrote:
> On 18/10/2017 07:57, Zhong Yang wrote:
> >   Your suggestion can avoid DMA target to nvdimm if the nvdimm memory region
> >   was skipped during VFIO hotplug. It is valuable to try this solution. by the
> >   way, please share me some clue for PCI address space related with memory region,
> >   below address_space_mem is right?  Many thanks!
> > 
> >   static void pci_bus_init(PCIBus *bus, DeviceState *parent,
> >                          MemoryRegion *address_space_mem,
> >                          MemoryRegion *address_space_io,
> >                          uint8_t devfn_min)  
> 
> Yes.  You would have to add the region directly to the CPU address
> space, and not to address_space_memory (which is included in both the
> CPU and PCI address spaces).
> 
> Paolo
  
  Thanks Paolo for your great help! I will try this solution in next week, thanks again.

  Regards,

  Yang