[PATCH] util/vfio-helpers.c: Use ram_block_discard_disable() in qemu_vfio_open_pci()

[PATCH] util/vfio-helpers.c: Use ram_block_discard_disable() in qemu_vfio_open_pci()

[David Hildenbrand]

Currently, when using "nvme://" for a block device, like
    -drive file=nvme://0000:01:00.0/1,if=none,id=drive0 \
    -device virtio-blk,drive=drive0 \

VFIO may pin all guest memory, and discarding of RAM no longer works as
expected. I was able to reproduce this easily with my
    01:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd
            NVMe SSD Controller SM981/PM981/PM983

Similar to common VFIO, we have to disable it, making sure that:
a) virtio-balloon won't discard any memory ("silently disabled")
b) virtio-mem and nvme:// run mutually exclusive

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Wei Yang <richardw.yang@linux.intel.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 stubs/ram-block.c   |  6 ++++++
 util/vfio-helpers.c | 14 ++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/stubs/ram-block.c b/stubs/ram-block.c
index 73c0a3ee08..108197683b 100644
--- a/stubs/ram-block.c
+++ b/stubs/ram-block.c
@@ -1,6 +1,7 @@
 #include "qemu/osdep.h"
 #include "exec/ramlist.h"
 #include "exec/cpu-common.h"
+#include "exec/memory.h"
 
 void *qemu_ram_get_host_addr(RAMBlock *rb)
 {
@@ -29,3 +30,8 @@ int qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque)
 {
     return 0;
 }
+
+int ram_block_discard_disable(bool state)
+{
+    return 0;
+}
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index c469beb061..2bec48e163 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -16,6 +16,7 @@
 #include "qapi/error.h"
 #include "exec/ramlist.h"
 #include "exec/cpu-common.h"
+#include "exec/memory.h"
 #include "trace.h"
 #include "qemu/error-report.h"
 #include "standard-headers/linux/pci_regs.h"
@@ -494,8 +495,20 @@ QEMUVFIOState *qemu_vfio_open_pci(const char *device, Error **errp)
     int r;
     QEMUVFIOState *s = g_new0(QEMUVFIOState, 1);
 
+    /*
+     * VFIO may pin all memory inside mappings, resulting it in pinning
+     * all memory inside RAM blocks unconditionally.
+     */
+    r = ram_block_discard_disable(true);
+    if (r) {
+        error_setg_errno(errp, -r, "Cannot set discarding of RAM broken");
+        g_free(s);
+        return NULL;
+    }
+
     r = qemu_vfio_init_pci(s, device, errp);
     if (r) {
+        ram_block_discard_disable(false);
         g_free(s);
         return NULL;
     }
@@ -837,4 +850,5 @@ void qemu_vfio_close(QEMUVFIOState *s)
     close(s->device);
     close(s->group);
     close(s->container);
+    ram_block_discard_disable(false);
 }
-- 
2.26.2

Re: [PATCH] util/vfio-helpers.c: Use ram_block_discard_disable() in qemu_vfio_open_pci()

[Philippe Mathieu-Daudé]

Hi David,

Is this aiming at 5.2?

On 11/16/20 11:59 AM, David Hildenbrand wrote:
> Currently, when using "nvme://" for a block device, like
>     -drive file=nvme://0000:01:00.0/1,if=none,id=drive0 \
>     -device virtio-blk,drive=drive0 \
> 
> VFIO may pin all guest memory, and discarding of RAM no longer works as
> expected. I was able to reproduce this easily with my
>     01:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd
>             NVMe SSD Controller SM981/PM981/PM983
> 
> Similar to common VFIO, we have to disable it, making sure that:
> a) virtio-balloon won't discard any memory ("silently disabled")
> b) virtio-mem and nvme:// run mutually exclusive
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Wei Yang <richardw.yang@linux.intel.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  stubs/ram-block.c   |  6 ++++++
>  util/vfio-helpers.c | 14 ++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/stubs/ram-block.c b/stubs/ram-block.c
> index 73c0a3ee08..108197683b 100644
> --- a/stubs/ram-block.c
> +++ b/stubs/ram-block.c
> @@ -1,6 +1,7 @@
>  #include "qemu/osdep.h"
>  #include "exec/ramlist.h"
>  #include "exec/cpu-common.h"
> +#include "exec/memory.h"
>  
>  void *qemu_ram_get_host_addr(RAMBlock *rb)
>  {
> @@ -29,3 +30,8 @@ int qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque)
>  {
>      return 0;
>  }
> +
> +int ram_block_discard_disable(bool state)
> +{
> +    return 0;
> +}
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index c469beb061..2bec48e163 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -16,6 +16,7 @@
>  #include "qapi/error.h"
>  #include "exec/ramlist.h"
>  #include "exec/cpu-common.h"
> +#include "exec/memory.h"
>  #include "trace.h"
>  #include "qemu/error-report.h"
>  #include "standard-headers/linux/pci_regs.h"
> @@ -494,8 +495,20 @@ QEMUVFIOState *qemu_vfio_open_pci(const char *device, Error **errp)
>      int r;
>      QEMUVFIOState *s = g_new0(QEMUVFIOState, 1);
>  
> +    /*
> +     * VFIO may pin all memory inside mappings, resulting it in pinning
> +     * all memory inside RAM blocks unconditionally.
> +     */
> +    r = ram_block_discard_disable(true);
> +    if (r) {
> +        error_setg_errno(errp, -r, "Cannot set discarding of RAM broken");
> +        g_free(s);
> +        return NULL;
> +    }
> +
>      r = qemu_vfio_init_pci(s, device, errp);
>      if (r) {
> +        ram_block_discard_disable(false);
>          g_free(s);
>          return NULL;
>      }
> @@ -837,4 +850,5 @@ void qemu_vfio_close(QEMUVFIOState *s)
>      close(s->device);
>      close(s->group);
>      close(s->container);
> +    ram_block_discard_disable(false);
>  }
> 

Re: [PATCH] util/vfio-helpers.c: Use ram_block_discard_disable() in qemu_vfio_open_pci()

[David Hildenbrand]

On 16.11.20 14:14, Philippe Mathieu-Daudé wrote:
> Hi David,
> 
> Is this aiming at 5.2?

Hi,

Good point!

If possible, we want this in 5.2.

Thanks!

> 
> On 11/16/20 11:59 AM, David Hildenbrand wrote:
>> Currently, when using "nvme://" for a block device, like
>>      -drive file=nvme://0000:01:00.0/1,if=none,id=drive0 \
>>      -device virtio-blk,drive=drive0 \
>>
>> VFIO may pin all guest memory, and discarding of RAM no longer works as
>> expected. I was able to reproduce this easily with my
>>      01:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd
>>              NVMe SSD Controller SM981/PM981/PM983
>>
>> Similar to common VFIO, we have to disable it, making sure that:
>> a) virtio-balloon won't discard any memory ("silently disabled")
>> b) virtio-mem and nvme:// run mutually exclusive
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Cc: Wei Yang <richardw.yang@linux.intel.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   stubs/ram-block.c   |  6 ++++++
>>   util/vfio-helpers.c | 14 ++++++++++++++
>>   2 files changed, 20 insertions(+)
>>
>> diff --git a/stubs/ram-block.c b/stubs/ram-block.c
>> index 73c0a3ee08..108197683b 100644
>> --- a/stubs/ram-block.c
>> +++ b/stubs/ram-block.c
>> @@ -1,6 +1,7 @@
>>   #include "qemu/osdep.h"
>>   #include "exec/ramlist.h"
>>   #include "exec/cpu-common.h"
>> +#include "exec/memory.h"
>>   
>>   void *qemu_ram_get_host_addr(RAMBlock *rb)
>>   {
>> @@ -29,3 +30,8 @@ int qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque)
>>   {
>>       return 0;
>>   }
>> +
>> +int ram_block_discard_disable(bool state)
>> +{
>> +    return 0;
>> +}
>> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
>> index c469beb061..2bec48e163 100644
>> --- a/util/vfio-helpers.c
>> +++ b/util/vfio-helpers.c
>> @@ -16,6 +16,7 @@
>>   #include "qapi/error.h"
>>   #include "exec/ramlist.h"
>>   #include "exec/cpu-common.h"
>> +#include "exec/memory.h"
>>   #include "trace.h"
>>   #include "qemu/error-report.h"
>>   #include "standard-headers/linux/pci_regs.h"
>> @@ -494,8 +495,20 @@ QEMUVFIOState *qemu_vfio_open_pci(const char *device, Error **errp)
>>       int r;
>>       QEMUVFIOState *s = g_new0(QEMUVFIOState, 1);
>>   
>> +    /*
>> +     * VFIO may pin all memory inside mappings, resulting it in pinning
>> +     * all memory inside RAM blocks unconditionally.
>> +     */
>> +    r = ram_block_discard_disable(true);
>> +    if (r) {
>> +        error_setg_errno(errp, -r, "Cannot set discarding of RAM broken");
>> +        g_free(s);
>> +        return NULL;
>> +    }
>> +
>>       r = qemu_vfio_init_pci(s, device, errp);
>>       if (r) {
>> +        ram_block_discard_disable(false);
>>           g_free(s);
>>           return NULL;
>>       }
>> @@ -837,4 +850,5 @@ void qemu_vfio_close(QEMUVFIOState *s)
>>       close(s->device);
>>       close(s->group);
>>       close(s->container);
>> +    ram_block_discard_disable(false);
>>   }
>>
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH] util/vfio-helpers.c: Use ram_block_discard_disable() in qemu_vfio_open_pci()

[Paolo Bonzini]

On 16/11/20 11:59, David Hildenbrand wrote:
> Currently, when using "nvme://" for a block device, like
>      -drive file=nvme://0000:01:00.0/1,if=none,id=drive0 \
>      -device virtio-blk,drive=drive0 \
> 
> VFIO may pin all guest memory, and discarding of RAM no longer works as
> expected. I was able to reproduce this easily with my
>      01:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd
>              NVMe SSD Controller SM981/PM981/PM983
> 
> Similar to common VFIO, we have to disable it, making sure that:
> a) virtio-balloon won't discard any memory ("silently disabled")
> b) virtio-mem and nvme:// run mutually exclusive
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Wei Yang <richardw.yang@linux.intel.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   stubs/ram-block.c   |  6 ++++++
>   util/vfio-helpers.c | 14 ++++++++++++++
>   2 files changed, 20 insertions(+)
> 
> diff --git a/stubs/ram-block.c b/stubs/ram-block.c
> index 73c0a3ee08..108197683b 100644
> --- a/stubs/ram-block.c
> +++ b/stubs/ram-block.c
> @@ -1,6 +1,7 @@
>   #include "qemu/osdep.h"
>   #include "exec/ramlist.h"
>   #include "exec/cpu-common.h"
> +#include "exec/memory.h"
>   
>   void *qemu_ram_get_host_addr(RAMBlock *rb)
>   {
> @@ -29,3 +30,8 @@ int qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque)
>   {
>       return 0;
>   }
> +
> +int ram_block_discard_disable(bool state)
> +{
> +    return 0;
> +}
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index c469beb061..2bec48e163 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -16,6 +16,7 @@
>   #include "qapi/error.h"
>   #include "exec/ramlist.h"
>   #include "exec/cpu-common.h"
> +#include "exec/memory.h"
>   #include "trace.h"
>   #include "qemu/error-report.h"
>   #include "standard-headers/linux/pci_regs.h"
> @@ -494,8 +495,20 @@ QEMUVFIOState *qemu_vfio_open_pci(const char *device, Error **errp)
>       int r;
>       QEMUVFIOState *s = g_new0(QEMUVFIOState, 1);
>   
> +    /*
> +     * VFIO may pin all memory inside mappings, resulting it in pinning
> +     * all memory inside RAM blocks unconditionally.
> +     */
> +    r = ram_block_discard_disable(true);
> +    if (r) {
> +        error_setg_errno(errp, -r, "Cannot set discarding of RAM broken");
> +        g_free(s);
> +        return NULL;
> +    }
> +
>       r = qemu_vfio_init_pci(s, device, errp);
>       if (r) {
> +        ram_block_discard_disable(false);
>           g_free(s);
>           return NULL;
>       }
> @@ -837,4 +850,5 @@ void qemu_vfio_close(QEMUVFIOState *s)
>       close(s->device);
>       close(s->group);
>       close(s->container);
> +    ram_block_discard_disable(false);
>   }
> 

Queued, thanks.

Paolo