[Qemu-devel] [PATCH v3 0/4] Balloon inhibit enhancements, vfio restriction

[Qemu-devel] [PATCH v3 0/4] Balloon inhibit enhancements, vfio restriction

[Alex Williamson]

v3:
 - Drop "nested" term in commit log (David)
 - Adopt suggested wording in ccw code (Cornelia)
 - Explain balloon inhibitor usage in vfio common (Peter)
 - Fix to call inhibitor prior to re-using existing containers
   to avoid gap that pinning may have occurred in set container
   ioctl (self) - Peter, this change is the reason I didn't
   include your R-b.
 - Add R-b to patches 1 & 2

v2:
 - Use atomic ops for balloon inhibit counter (Peter)
 - Allow endpoint driver opt-in for ballooning, vfio-ccw opt-in by
   default, vfio-pci opt-in by device option, only allowed for mdev
   devices, no support added for platform as there are no platform
   mdev devices.

See patch 3/4 for detailed explanation why ballooning and device
assignment typically don't mix.  If this eventually changes, flags
on the iommu info struct or perhaps device info struct can inform
us for automatic opt-in.  Thanks,

Alex

Alex Williamson (4):
  balloon: Allow multiple inhibit users
  kvm: Use inhibit to prevent ballooning without synchronous mmu
  vfio: Inhibit ballooning based on group attachment to a container
  vfio/ccw/pci: Allow devices to opt-in for ballooning

 accel/kvm/kvm-all.c           |  4 +++
 balloon.c                     | 13 ++++++---
 hw/vfio/ccw.c                 |  9 +++++++
 hw/vfio/common.c              | 51 +++++++++++++++++++++++++++++++++++
 hw/vfio/pci.c                 | 26 +++++++++++++++++-
 hw/vfio/trace-events          |  1 +
 hw/virtio/virtio-balloon.c    |  4 +--
 include/hw/vfio/vfio-common.h |  2 ++
 8 files changed, 103 insertions(+), 7 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH v3 1/4] balloon: Allow multiple inhibit users

[Alex Williamson]

A simple true/false internal state does not allow multiple users.  Fix
this within the existing interface by converting to a counter, so long
as the counter is elevated, ballooning is inhibited.

Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 balloon.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/balloon.c b/balloon.c
index 6bf0a9681377..931987983858 100644
--- a/balloon.c
+++ b/balloon.c
@@ -26,6 +26,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu-common.h"
+#include "qemu/atomic.h"
 #include "exec/cpu-common.h"
 #include "sysemu/kvm.h"
 #include "sysemu/balloon.h"
@@ -37,16 +38,22 @@
 static QEMUBalloonEvent *balloon_event_fn;
 static QEMUBalloonStatus *balloon_stat_fn;
 static void *balloon_opaque;
-static bool balloon_inhibited;
+static int balloon_inhibit_count;
 
 bool qemu_balloon_is_inhibited(void)
 {
-    return balloon_inhibited;
+    return atomic_read(&balloon_inhibit_count) > 0;
 }
 
 void qemu_balloon_inhibit(bool state)
 {
-    balloon_inhibited = state;
+    if (state) {
+        atomic_inc(&balloon_inhibit_count);
+    } else {
+        atomic_dec(&balloon_inhibit_count);
+    }
+
+    assert(atomic_read(&balloon_inhibit_count) >= 0);
 }
 
 static bool have_balloon(Error **errp)
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 1/4] balloon: Allow multiple inhibit users

[Michael S. Tsirkin]

On Tue, Aug 07, 2018 at 01:31:22PM -0600, Alex Williamson wrote:
> A simple true/false internal state does not allow multiple users.  Fix
> this within the existing interface by converting to a counter, so long
> as the counter is elevated, ballooning is inhibited.
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  balloon.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/balloon.c b/balloon.c
> index 6bf0a9681377..931987983858 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -26,6 +26,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
> +#include "qemu/atomic.h"
>  #include "exec/cpu-common.h"
>  #include "sysemu/kvm.h"
>  #include "sysemu/balloon.h"
> @@ -37,16 +38,22 @@
>  static QEMUBalloonEvent *balloon_event_fn;
>  static QEMUBalloonStatus *balloon_stat_fn;
>  static void *balloon_opaque;
> -static bool balloon_inhibited;
> +static int balloon_inhibit_count;
>  
>  bool qemu_balloon_is_inhibited(void)
>  {
> -    return balloon_inhibited;
> +    return atomic_read(&balloon_inhibit_count) > 0;
>  }
>  
>  void qemu_balloon_inhibit(bool state)
>  {
> -    balloon_inhibited = state;
> +    if (state) {
> +        atomic_inc(&balloon_inhibit_count);
> +    } else {
> +        atomic_dec(&balloon_inhibit_count);
> +    }
> +
> +    assert(atomic_read(&balloon_inhibit_count) >= 0);
>  }
>  
>  static bool have_balloon(Error **errp)

This blocks QEMU_MADV_WONTNEED but it also blocks QEMU_MADV_WILLNEED.
Is this necessarily a good idea?

> -- 
> 2.18.0

Re: [Qemu-devel] [PATCH v3 1/4] balloon: Allow multiple inhibit users

[Alex Williamson]

On Tue, 7 Aug 2018 22:44:11 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Aug 07, 2018 at 01:31:22PM -0600, Alex Williamson wrote:
> > A simple true/false internal state does not allow multiple users.  Fix
> > this within the existing interface by converting to a counter, so long
> > as the counter is elevated, ballooning is inhibited.
> > 
> > Reviewed-by: David Hildenbrand <david@redhat.com>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  balloon.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/balloon.c b/balloon.c
> > index 6bf0a9681377..931987983858 100644
> > --- a/balloon.c
> > +++ b/balloon.c
> > @@ -26,6 +26,7 @@
> >  
> >  #include "qemu/osdep.h"
> >  #include "qemu-common.h"
> > +#include "qemu/atomic.h"
> >  #include "exec/cpu-common.h"
> >  #include "sysemu/kvm.h"
> >  #include "sysemu/balloon.h"
> > @@ -37,16 +38,22 @@
> >  static QEMUBalloonEvent *balloon_event_fn;
> >  static QEMUBalloonStatus *balloon_stat_fn;
> >  static void *balloon_opaque;
> > -static bool balloon_inhibited;
> > +static int balloon_inhibit_count;
> >  
> >  bool qemu_balloon_is_inhibited(void)
> >  {
> > -    return balloon_inhibited;
> > +    return atomic_read(&balloon_inhibit_count) > 0;
> >  }
> >  
> >  void qemu_balloon_inhibit(bool state)
> >  {
> > -    balloon_inhibited = state;
> > +    if (state) {
> > +        atomic_inc(&balloon_inhibit_count);
> > +    } else {
> > +        atomic_dec(&balloon_inhibit_count);
> > +    }
> > +
> > +    assert(atomic_read(&balloon_inhibit_count) >= 0);
> >  }
> >  
> >  static bool have_balloon(Error **errp)  
> 
> This blocks QEMU_MADV_WONTNEED but it also blocks QEMU_MADV_WILLNEED.
> Is this necessarily a good idea?

This is existing balloon inhibitor behavior, but do you have some
reason to suspect WILLNEED is necessary?  It's my impression that
WILLNEED is a purely optional prefetch directive that's entirely
unnecessary if the page wasn't previously zapped with WONTNEED.  If the
page was zapped, it will fault in on demand, potentially with higher
latency, but functionally correct.  With vfio usage of the inhibitor,
we expect pinning to fault in any previously ballooned pages, so
calling WILLNEED on pages where the inhibit count is elevated due to an
assigned device seems unnecessary.  Thanks,

Alex

Re: [Qemu-devel] [PATCH v3 1/4] balloon: Allow multiple inhibit users

[Michael S. Tsirkin]

On Tue, Aug 07, 2018 at 02:08:26PM -0600, Alex Williamson wrote:
> On Tue, 7 Aug 2018 22:44:11 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Aug 07, 2018 at 01:31:22PM -0600, Alex Williamson wrote:
> > > A simple true/false internal state does not allow multiple users.  Fix
> > > this within the existing interface by converting to a counter, so long
> > > as the counter is elevated, ballooning is inhibited.
> > > 
> > > Reviewed-by: David Hildenbrand <david@redhat.com>
> > > Reviewed-by: Peter Xu <peterx@redhat.com>
> > > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > ---
> > >  balloon.c | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/balloon.c b/balloon.c
> > > index 6bf0a9681377..931987983858 100644
> > > --- a/balloon.c
> > > +++ b/balloon.c
> > > @@ -26,6 +26,7 @@
> > >  
> > >  #include "qemu/osdep.h"
> > >  #include "qemu-common.h"
> > > +#include "qemu/atomic.h"
> > >  #include "exec/cpu-common.h"
> > >  #include "sysemu/kvm.h"
> > >  #include "sysemu/balloon.h"
> > > @@ -37,16 +38,22 @@
> > >  static QEMUBalloonEvent *balloon_event_fn;
> > >  static QEMUBalloonStatus *balloon_stat_fn;
> > >  static void *balloon_opaque;
> > > -static bool balloon_inhibited;
> > > +static int balloon_inhibit_count;
> > >  
> > >  bool qemu_balloon_is_inhibited(void)
> > >  {
> > > -    return balloon_inhibited;
> > > +    return atomic_read(&balloon_inhibit_count) > 0;
> > >  }
> > >  
> > >  void qemu_balloon_inhibit(bool state)
> > >  {
> > > -    balloon_inhibited = state;
> > > +    if (state) {
> > > +        atomic_inc(&balloon_inhibit_count);
> > > +    } else {
> > > +        atomic_dec(&balloon_inhibit_count);
> > > +    }
> > > +
> > > +    assert(atomic_read(&balloon_inhibit_count) >= 0);
> > >  }
> > >  
> > >  static bool have_balloon(Error **errp)  
> > 
> > This blocks QEMU_MADV_WONTNEED but it also blocks QEMU_MADV_WILLNEED.
> > Is this necessarily a good idea?
> 
> This is existing balloon inhibitor behavior, but do you have some
> reason to suspect WILLNEED is necessary?  It's my impression that
> WILLNEED is a purely optional prefetch directive that's entirely
> unnecessary if the page wasn't previously zapped with WONTNEED.  If the
> page was zapped, it will fault in on demand, potentially with higher
> latency, but functionally correct.  With vfio usage of the inhibitor,
> we expect pinning to fault in any previously ballooned pages, so
> calling WILLNEED on pages where the inhibit count is elevated due to an
> assigned device seems unnecessary.  Thanks,
> 
> Alex

So inhibit interface isn't great - it was designed for a single
user: the post-copy. My point is generalizing it by reference counting does
not seem to make for a sensible interface.
I agree vfio itself pages in all guest memory but how will
other users of this interface handle it?

What would a sensible interface look like, and how it would account
for post-copy requirements, I'm not yet sure.
Ideas welcome.

-- 
MST

[Qemu-devel] [PATCH v3 2/4] kvm: Use inhibit to prevent ballooning without synchronous mmu

[Alex Williamson]

Remove KVM specific tests in balloon_page(), instead marking
ballooning as inhibited without KVM_CAP_SYNC_MMU support.

Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 accel/kvm/kvm-all.c        | 4 ++++
 hw/virtio/virtio-balloon.c | 4 +---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index eb7db92a5e3b..38f468d8e2b1 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -39,6 +39,7 @@
 #include "trace.h"
 #include "hw/irq.h"
 #include "sysemu/sev.h"
+#include "sysemu/balloon.h"
 
 #include "hw/boards.h"
 
@@ -1698,6 +1699,9 @@ static int kvm_init(MachineState *ms)
     s->many_ioeventfds = kvm_check_many_ioeventfds();
 
     s->sync_mmu = !!kvm_vm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
+    if (!s->sync_mmu) {
+        qemu_balloon_inhibit(true);
+    }
 
     return 0;
 
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 1f7a87f09429..b5425080c5fb 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -21,7 +21,6 @@
 #include "hw/mem/pc-dimm.h"
 #include "sysemu/balloon.h"
 #include "hw/virtio/virtio-balloon.h"
-#include "sysemu/kvm.h"
 #include "exec/address-spaces.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-misc.h"
@@ -36,8 +35,7 @@
 
 static void balloon_page(void *addr, int deflate)
 {
-    if (!qemu_balloon_is_inhibited() && (!kvm_enabled() ||
-                                         kvm_has_sync_mmu())) {
+    if (!qemu_balloon_is_inhibited()) {
         qemu_madvise(addr, BALLOON_PAGE_SIZE,
                 deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
     }
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 2/4] kvm: Use inhibit to prevent ballooning without synchronous mmu

[Alex Williamson]

On Tue,  7 Aug 2018 13:31:23 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> Remove KVM specific tests in balloon_page(), instead marking
> ballooning as inhibited without KVM_CAP_SYNC_MMU support.
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  accel/kvm/kvm-all.c        | 4 ++++
>  hw/virtio/virtio-balloon.c | 4 +---
>  2 files changed, 5 insertions(+), 3 deletions(-)

Paolo and Michael, can I get an ack for this one?  Otherwise I can drop
it from the series and let this continue to be a special case.  Thanks,

Alex
 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index eb7db92a5e3b..38f468d8e2b1 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -39,6 +39,7 @@
>  #include "trace.h"
>  #include "hw/irq.h"
>  #include "sysemu/sev.h"
> +#include "sysemu/balloon.h"
>  
>  #include "hw/boards.h"
>  
> @@ -1698,6 +1699,9 @@ static int kvm_init(MachineState *ms)
>      s->many_ioeventfds = kvm_check_many_ioeventfds();
>  
>      s->sync_mmu = !!kvm_vm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
> +    if (!s->sync_mmu) {
> +        qemu_balloon_inhibit(true);
> +    }
>  
>      return 0;
>  
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 1f7a87f09429..b5425080c5fb 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -21,7 +21,6 @@
>  #include "hw/mem/pc-dimm.h"
>  #include "sysemu/balloon.h"
>  #include "hw/virtio/virtio-balloon.h"
> -#include "sysemu/kvm.h"
>  #include "exec/address-spaces.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-events-misc.h"
> @@ -36,8 +35,7 @@
>  
>  static void balloon_page(void *addr, int deflate)
>  {
> -    if (!qemu_balloon_is_inhibited() && (!kvm_enabled() ||
> -                                         kvm_has_sync_mmu())) {
> +    if (!qemu_balloon_is_inhibited()) {
>          qemu_madvise(addr, BALLOON_PAGE_SIZE,
>                  deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
>      }

Re: [Qemu-devel] [PATCH v3 2/4] kvm: Use inhibit to prevent ballooning without synchronous mmu

[Paolo Bonzini]

On 16/08/2018 20:15, Alex Williamson wrote:
> On Tue,  7 Aug 2018 13:31:23 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
>> Remove KVM specific tests in balloon_page(), instead marking
>> ballooning as inhibited without KVM_CAP_SYNC_MMU support.
>>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>> ---
>>  accel/kvm/kvm-all.c        | 4 ++++
>>  hw/virtio/virtio-balloon.c | 4 +---
>>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> Paolo and Michael, can I get an ack for this one?  Otherwise I can drop
> it from the series and let this continue to be a special case.  Thanks,
> 
> Alex
>  
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index eb7db92a5e3b..38f468d8e2b1 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -39,6 +39,7 @@
>>  #include "trace.h"
>>  #include "hw/irq.h"
>>  #include "sysemu/sev.h"
>> +#include "sysemu/balloon.h"
>>  
>>  #include "hw/boards.h"
>>  
>> @@ -1698,6 +1699,9 @@ static int kvm_init(MachineState *ms)
>>      s->many_ioeventfds = kvm_check_many_ioeventfds();
>>  
>>      s->sync_mmu = !!kvm_vm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
>> +    if (!s->sync_mmu) {
>> +        qemu_balloon_inhibit(true);
>> +    }
>>  
>>      return 0;
>>  
>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> index 1f7a87f09429..b5425080c5fb 100644
>> --- a/hw/virtio/virtio-balloon.c
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -21,7 +21,6 @@
>>  #include "hw/mem/pc-dimm.h"
>>  #include "sysemu/balloon.h"
>>  #include "hw/virtio/virtio-balloon.h"
>> -#include "sysemu/kvm.h"
>>  #include "exec/address-spaces.h"
>>  #include "qapi/error.h"
>>  #include "qapi/qapi-events-misc.h"
>> @@ -36,8 +35,7 @@
>>  
>>  static void balloon_page(void *addr, int deflate)
>>  {
>> -    if (!qemu_balloon_is_inhibited() && (!kvm_enabled() ||
>> -                                         kvm_has_sync_mmu())) {
>> +    if (!qemu_balloon_is_inhibited()) {
>>          qemu_madvise(addr, BALLOON_PAGE_SIZE,
>>                  deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
>>      }
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks,

Paolo

[Qemu-devel] [PATCH v3 3/4] vfio: Inhibit ballooning based on group attachment to a container

[Alex Williamson]

We use a VFIOContainer to associate an AddressSpace to one or more
VFIOGroups.  The VFIOContainer represents the DMA context for that
AdressSpace for those VFIOGroups and is synchronized to changes in
that AddressSpace via a MemoryListener.  For IOMMU backed devices,
maintaining the DMA context for a VFIOGroup generally involves
pinning a host virtual address in order to create a stable host
physical address and then mapping a translation from the associated
guest physical address to that host physical address into the IOMMU.

While the above maintains the VFIOContainer synchronized to the QEMU
memory API of the VM, memory ballooning occurs outside of that API.
Inflating the memory balloon (ie. cooperatively capturing pages from
the guest for use by the host) simply uses MADV_DONTNEED to "zap"
pages from QEMU's host virtual address space.  The page pinning and
IOMMU mapping above remains in place, negating the host's ability to
reuse the page, but the host virtual to host physical mapping of the
page is invalidated outside of QEMU's memory API.

When the balloon is later deflated, attempting to cooperatively
return pages to the guest, the page is simply freed by the guest
balloon driver, allowing it to be used in the guest and incurring a
page fault when that occurs.  The page fault maps a new host physical
page backing the existing host virtual address, meanwhile the
VFIOContainer still maintains the translation to the original host
physical address.  At this point the guest vCPU and any assigned
devices will map different host physical addresses to the same guest
physical address.  Badness.

The IOMMU typically does not have page level granularity with which
it can track this mapping without also incurring inefficiencies in
using page size mappings throughout.  MMU notifiers in the host
kernel also provide indicators for invalidating the mapping on
balloon inflation, not for updating the mapping when the balloon is
deflated.  For these reasons we assume a default behavior that the
mapping of each VFIOGroup into the VFIOContainer is incompatible
with memory ballooning and increment the balloon inhibitor to match
the attached VFIOGroups.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/vfio/common.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index fb396cf00ac4..a3d758af8d8f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -32,6 +32,7 @@
 #include "hw/hw.h"
 #include "qemu/error-report.h"
 #include "qemu/range.h"
+#include "sysemu/balloon.h"
 #include "sysemu/kvm.h"
 #include "trace.h"
 #include "qapi/error.h"
@@ -1044,6 +1045,33 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
 
     space = vfio_get_address_space(as);
 
+    /*
+     * VFIO is currently incompatible with memory ballooning insofar as the
+     * madvise to purge (zap) the page from QEMU's address space does not
+     * interact with the memory API and therefore leaves stale virtual to
+     * physical mappings in the IOMMU if the page was previously pinned.  We
+     * therefore add a balloon inhibit for each group added to a container,
+     * whether the container is used individually or shared.  This provides
+     * us with options to allow devices within a group to opt-in and allow
+     * ballooning, so long as it is done consistently for a group (for instance
+     * if the device is an mdev device where it is known that the host vendor
+     * driver will never pin pages outside of the working set of the guest
+     * driver, which would thus not be ballooning candidates).
+     *
+     * The first opportunity to induce pinning occurs here where we attempt to
+     * attach the group to existing containers within the AddressSpace.  If any
+     * pages are already zapped from the virtual address space, such as from a
+     * previous ballooning opt-in, new pinning will cause valid mappings to be
+     * re-established.  Likewise, when the overall MemoryListener for a new
+     * container is registered, a replay of mappings within the AddressSpace
+     * will occur, re-establishing any previously zapped pages as well.
+     *
+     * NB. Balloon inhibiting does not currently block operation of the
+     * balloon driver or revoke previously pinned pages, it only prevents
+     * calling madvise to modify the virtual mapping of ballooned pages.
+     */
+    qemu_balloon_inhibit(true);
+
     QLIST_FOREACH(container, &space->containers, next) {
         if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
             group->container = container;
@@ -1232,6 +1260,7 @@ close_fd_exit:
     close(fd);
 
 put_space_exit:
+    qemu_balloon_inhibit(false);
     vfio_put_address_space(space);
 
     return ret;
@@ -1352,6 +1381,7 @@ void vfio_put_group(VFIOGroup *group)
         return;
     }
 
+    qemu_balloon_inhibit(false);
     vfio_kvm_device_del_group(group);
     vfio_disconnect_container(group);
     QLIST_REMOVE(group, next);
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 3/4] vfio: Inhibit ballooning based on group attachment to a container

[Peter Xu]

On Tue, Aug 07, 2018 at 01:31:24PM -0600, Alex Williamson wrote:
> We use a VFIOContainer to associate an AddressSpace to one or more
> VFIOGroups.  The VFIOContainer represents the DMA context for that
> AdressSpace for those VFIOGroups and is synchronized to changes in
> that AddressSpace via a MemoryListener.  For IOMMU backed devices,
> maintaining the DMA context for a VFIOGroup generally involves
> pinning a host virtual address in order to create a stable host
> physical address and then mapping a translation from the associated
> guest physical address to that host physical address into the IOMMU.
> 
> While the above maintains the VFIOContainer synchronized to the QEMU
> memory API of the VM, memory ballooning occurs outside of that API.
> Inflating the memory balloon (ie. cooperatively capturing pages from
> the guest for use by the host) simply uses MADV_DONTNEED to "zap"
> pages from QEMU's host virtual address space.  The page pinning and
> IOMMU mapping above remains in place, negating the host's ability to
> reuse the page, but the host virtual to host physical mapping of the
> page is invalidated outside of QEMU's memory API.
> 
> When the balloon is later deflated, attempting to cooperatively
> return pages to the guest, the page is simply freed by the guest
> balloon driver, allowing it to be used in the guest and incurring a
> page fault when that occurs.  The page fault maps a new host physical
> page backing the existing host virtual address, meanwhile the
> VFIOContainer still maintains the translation to the original host
> physical address.  At this point the guest vCPU and any assigned
> devices will map different host physical addresses to the same guest
> physical address.  Badness.
> 
> The IOMMU typically does not have page level granularity with which
> it can track this mapping without also incurring inefficiencies in
> using page size mappings throughout.  MMU notifiers in the host
> kernel also provide indicators for invalidating the mapping on
> balloon inflation, not for updating the mapping when the balloon is
> deflated.  For these reasons we assume a default behavior that the
> mapping of each VFIOGroup into the VFIOContainer is incompatible
> with memory ballooning and increment the balloon inhibitor to match
> the attached VFIOGroups.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH v3 4/4] vfio/ccw/pci: Allow devices to opt-in for ballooning

[Alex Williamson]

If a vfio assigned device makes use of a physical IOMMU, then memory
ballooning is necessarily inhibited due to the page pinning, lack of
page level granularity at the IOMMU, and sufficient notifiers to both
remove the page on balloon inflation and add it back on deflation.
However, not all devices are backed by a physical IOMMU.  In the case
of mediated devices, if a vendor driver is well synchronized with the
guest driver, such that only pages actively used by the guest driver
are pinned by the host mdev vendor driver, then there should be no
overlap between pages available for the balloon driver and pages
actively in use by the device.  Under these conditions, ballooning
should be safe.

vfio-ccw devices are always mediated devices and always operate under
the constraints above.  Therefore we can consider all vfio-ccw devices
as balloon compatible.

The situation is far from straightforward with vfio-pci.  These
devices can be physical devices with physical IOMMU backing or
mediated devices where it is unknown whether a physical IOMMU is in
use or whether the vendor driver is well synchronized to the working
set of the guest driver.  The safest approach is therefore to assume
all vfio-pci devices are incompatible with ballooning, but allow user
opt-in should they have further insight into mediated devices.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/vfio/ccw.c                 |  9 +++++++++
 hw/vfio/common.c              | 23 ++++++++++++++++++++++-
 hw/vfio/pci.c                 | 26 +++++++++++++++++++++++++-
 hw/vfio/trace-events          |  1 +
 include/hw/vfio/vfio-common.h |  2 ++
 5 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 351b305e1ae7..e96bbdc78b48 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -349,6 +349,15 @@ static void vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev,
         }
     }
 
+    /*
+     * All vfio-ccw devices are believed to operate in a way compatible with
+     * memory ballooning, ie. pages pinned in the host are in the current
+     * working set of the guest driver and therefore never overlap with pages
+     * available to the guest balloon driver.  This needs to be set before
+     * vfio_get_device() for vfio common to handle the balloon inhibitor.
+     */
+    vcdev->vdev.balloon_allowed = true;
+
     if (vfio_get_device(group, vcdev->cdev.mdevid, &vcdev->vdev, errp)) {
         goto out_err;
     }
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index a3d758af8d8f..cd1f4af18abb 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1381,7 +1381,9 @@ void vfio_put_group(VFIOGroup *group)
         return;
     }
 
-    qemu_balloon_inhibit(false);
+    if (!group->balloon_allowed) {
+        qemu_balloon_inhibit(false);
+    }
     vfio_kvm_device_del_group(group);
     vfio_disconnect_container(group);
     QLIST_REMOVE(group, next);
@@ -1417,6 +1419,25 @@ int vfio_get_device(VFIOGroup *group, const char *name,
         return ret;
     }
 
+    /*
+     * Clear the balloon inhibitor for this group if the driver knows the
+     * device operates compatibly with ballooning.  Setting must be consistent
+     * per group, but since compatibility is really only possible with mdev
+     * currently, we expect singleton groups.
+     */
+    if (vbasedev->balloon_allowed != group->balloon_allowed) {
+        if (!QLIST_EMPTY(&group->device_list)) {
+            error_setg(errp,
+                       "Inconsistent device balloon setting within group");
+            return -1;
+        }
+
+        if (!group->balloon_allowed) {
+            group->balloon_allowed = true;
+            qemu_balloon_inhibit(false);
+        }
+    }
+
     vbasedev->fd = fd;
     vbasedev->group = group;
     QLIST_INSERT_HEAD(&group->device_list, vbasedev, next);
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 6cbb8fa0549d..056f3a887a8f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2804,12 +2804,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
     VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
     VFIODevice *vbasedev_iter;
     VFIOGroup *group;
-    char *tmp, group_path[PATH_MAX], *group_name;
+    char *tmp, *subsys, group_path[PATH_MAX], *group_name;
     Error *err = NULL;
     ssize_t len;
     struct stat st;
     int groupid;
     int i, ret;
+    bool is_mdev;
 
     if (!vdev->vbasedev.sysfsdev) {
         if (!(~vdev->host.domain || ~vdev->host.bus ||
@@ -2869,6 +2870,27 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         }
     }
 
+    /*
+     * Mediated devices *might* operate compatibly with memory ballooning, but
+     * we cannot know for certain, it depends on whether the mdev vendor driver
+     * stays in sync with the active working set of the guest driver.  Prevent
+     * the x-balloon-allowed option unless this is minimally an mdev device.
+     */
+    tmp = g_strdup_printf("%s/subsystem", vdev->vbasedev.sysfsdev);
+    subsys = realpath(tmp, NULL);
+    g_free(tmp);
+    is_mdev = (strcmp(subsys, "/sys/bus/mdev") == 0);
+    free(subsys);
+
+    trace_vfio_mdev(vdev->vbasedev.name, is_mdev);
+
+    if (vdev->vbasedev.balloon_allowed && !is_mdev) {
+        error_setg(errp, "x-balloon-allowed only potentially compatible "
+                   "with mdev devices");
+        vfio_put_group(group);
+        goto error;
+    }
+
     ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev, errp);
     if (ret) {
         vfio_put_group(group);
@@ -3170,6 +3192,8 @@ static Property vfio_pci_dev_properties[] = {
     DEFINE_PROP_BIT("x-igd-opregion", VFIOPCIDevice, features,
                     VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
     DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
+    DEFINE_PROP_BOOL("x-balloon-allowed", VFIOPCIDevice,
+                     vbasedev.balloon_allowed, false),
     DEFINE_PROP_BOOL("x-no-kvm-intx", VFIOPCIDevice, no_kvm_intx, false),
     DEFINE_PROP_BOOL("x-no-kvm-msi", VFIOPCIDevice, no_kvm_msi, false),
     DEFINE_PROP_BOOL("x-no-kvm-msix", VFIOPCIDevice, no_kvm_msix, false),
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index d2a74952e389..a85e8662eadb 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -39,6 +39,7 @@ vfio_pci_hot_reset_result(const char *name, const char *result) "%s hot reset: %
 vfio_populate_device_config(const char *name, unsigned long size, unsigned long offset, unsigned long flags) "Device %s config:\n  size: 0x%lx, offset: 0x%lx, flags: 0x%lx"
 vfio_populate_device_get_irq_info_failure(void) "VFIO_DEVICE_GET_IRQ_INFO failure: %m"
 vfio_realize(const char *name, int group_id) " (%s) group %d"
+vfio_mdev(const char *name, bool is_mdev) " (%s) is_mdev %d"
 vfio_add_ext_cap_dropped(const char *name, uint16_t cap, uint16_t offset) "%s 0x%x@0x%x"
 vfio_pci_reset(const char *name) " (%s)"
 vfio_pci_reset_flr(const char *name) "%s FLR/VFIO_DEVICE_RESET"
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index a9036929b220..15ea6c26fdbc 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -112,6 +112,7 @@ typedef struct VFIODevice {
     bool reset_works;
     bool needs_reset;
     bool no_mmap;
+    bool balloon_allowed;
     VFIODeviceOps *ops;
     unsigned int num_irqs;
     unsigned int num_regions;
@@ -131,6 +132,7 @@ typedef struct VFIOGroup {
     QLIST_HEAD(, VFIODevice) device_list;
     QLIST_ENTRY(VFIOGroup) next;
     QLIST_ENTRY(VFIOGroup) container_next;
+    bool balloon_allowed;
 } VFIOGroup;
 
 typedef struct VFIODMABuf {
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 0/4] Balloon inhibit enhancements, vfio restriction

[Michael S. Tsirkin]

On Tue, Aug 07, 2018 at 01:31:21PM -0600, Alex Williamson wrote:
> v3:
>  - Drop "nested" term in commit log (David)
>  - Adopt suggested wording in ccw code (Cornelia)
>  - Explain balloon inhibitor usage in vfio common (Peter)
>  - Fix to call inhibitor prior to re-using existing containers
>    to avoid gap that pinning may have occurred in set container
>    ioctl (self) - Peter, this change is the reason I didn't
>    include your R-b.
>  - Add R-b to patches 1 & 2
> 
> v2:
>  - Use atomic ops for balloon inhibit counter (Peter)
>  - Allow endpoint driver opt-in for ballooning, vfio-ccw opt-in by
>    default, vfio-pci opt-in by device option, only allowed for mdev
>    devices, no support added for platform as there are no platform
>    mdev devices.
> 
> See patch 3/4 for detailed explanation why ballooning and device
> assignment typically don't mix.  If this eventually changes, flags
> on the iommu info struct or perhaps device info struct can inform
> us for automatic opt-in.  Thanks,
> 
> Alex

One of the issues with pass-through is that it breaks overcommit
through swap. ballooning seems to offer one solution, instead of
making it work this patch just attempts to block ballooning.

I guess it's better than corrupting memory but I personally find this
approach disappointing.


> Alex Williamson (4):
>   balloon: Allow multiple inhibit users
>   kvm: Use inhibit to prevent ballooning without synchronous mmu
>   vfio: Inhibit ballooning based on group attachment to a container
>   vfio/ccw/pci: Allow devices to opt-in for ballooning
> 
>  accel/kvm/kvm-all.c           |  4 +++
>  balloon.c                     | 13 ++++++---
>  hw/vfio/ccw.c                 |  9 +++++++
>  hw/vfio/common.c              | 51 +++++++++++++++++++++++++++++++++++
>  hw/vfio/pci.c                 | 26 +++++++++++++++++-
>  hw/vfio/trace-events          |  1 +
>  hw/virtio/virtio-balloon.c    |  4 +--
>  include/hw/vfio/vfio-common.h |  2 ++
>  8 files changed, 103 insertions(+), 7 deletions(-)
> 
> -- 
> 2.18.0

Re: [Qemu-devel] [PATCH v3 0/4] Balloon inhibit enhancements, vfio restriction

[Alex Williamson]

On Tue, 7 Aug 2018 22:44:56 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Aug 07, 2018 at 01:31:21PM -0600, Alex Williamson wrote:
> > v3:
> >  - Drop "nested" term in commit log (David)
> >  - Adopt suggested wording in ccw code (Cornelia)
> >  - Explain balloon inhibitor usage in vfio common (Peter)
> >  - Fix to call inhibitor prior to re-using existing containers
> >    to avoid gap that pinning may have occurred in set container
> >    ioctl (self) - Peter, this change is the reason I didn't
> >    include your R-b.
> >  - Add R-b to patches 1 & 2
> > 
> > v2:
> >  - Use atomic ops for balloon inhibit counter (Peter)
> >  - Allow endpoint driver opt-in for ballooning, vfio-ccw opt-in by
> >    default, vfio-pci opt-in by device option, only allowed for mdev
> >    devices, no support added for platform as there are no platform
> >    mdev devices.
> > 
> > See patch 3/4 for detailed explanation why ballooning and device
> > assignment typically don't mix.  If this eventually changes, flags
> > on the iommu info struct or perhaps device info struct can inform
> > us for automatic opt-in.  Thanks,
> > 
> > Alex  
> 
> One of the issues with pass-through is that it breaks overcommit
> through swap. ballooning seems to offer one solution, instead of
> making it work this patch just attempts to block ballooning.
> 
> I guess it's better than corrupting memory but I personally find this
> approach disappointing.

Memory hotplug is the way to achieve variable density with assigned
device VMs, otherwise look towards approaches like mdev and shared
virtual addresses with PASID support.  We cannot shoehorn page faulting
without both hardware and software support.  Some class of "legacy"
device assignment will always have this incompatibility.  Thanks,

Alex

Re: [Qemu-devel] [PATCH v3 0/4] Balloon inhibit enhancements, vfio restriction

[Michael S. Tsirkin]

On Tue, Aug 07, 2018 at 01:53:03PM -0600, Alex Williamson wrote:
> On Tue, 7 Aug 2018 22:44:56 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Aug 07, 2018 at 01:31:21PM -0600, Alex Williamson wrote:
> > > v3:
> > >  - Drop "nested" term in commit log (David)
> > >  - Adopt suggested wording in ccw code (Cornelia)
> > >  - Explain balloon inhibitor usage in vfio common (Peter)
> > >  - Fix to call inhibitor prior to re-using existing containers
> > >    to avoid gap that pinning may have occurred in set container
> > >    ioctl (self) - Peter, this change is the reason I didn't
> > >    include your R-b.
> > >  - Add R-b to patches 1 & 2
> > > 
> > > v2:
> > >  - Use atomic ops for balloon inhibit counter (Peter)
> > >  - Allow endpoint driver opt-in for ballooning, vfio-ccw opt-in by
> > >    default, vfio-pci opt-in by device option, only allowed for mdev
> > >    devices, no support added for platform as there are no platform
> > >    mdev devices.
> > > 
> > > See patch 3/4 for detailed explanation why ballooning and device
> > > assignment typically don't mix.  If this eventually changes, flags
> > > on the iommu info struct or perhaps device info struct can inform
> > > us for automatic opt-in.  Thanks,
> > > 
> > > Alex  
> > 
> > One of the issues with pass-through is that it breaks overcommit
> > through swap. ballooning seems to offer one solution, instead of
> > making it work this patch just attempts to block ballooning.
> > 
> > I guess it's better than corrupting memory but I personally find this
> > approach disappointing.
> 
> Memory hotplug is the way to achieve variable density with assigned
> device VMs, otherwise look towards approaches like mdev and shared
> virtual addresses with PASID support.  We cannot shoehorn page faulting
> without both hardware and software support.  Some class of "legacy"
> device assignment will always have this incompatibility.  Thanks,
> 
> Alex

I'm not sure I agree.

At least with VTD, it seems entirely possible to change e.g. a PMD
atomically to point to a different set of PTEs, then flush.
That will allow removing memory at high granularity for
an arbitrary device without mdev or PASID dependency.

I suspect most IOMMUs are like this.

IIUC doing that within guest right now will cause a range to be unmapped
and them mapped again, which I suspect only works if we are lucky and
device does not access the range during this time.

So at some level it's a theoretical bug we would do well to fix,
and then we can support ballooning better.

-- 
MST

Re: [Qemu-devel] [PATCH v3 0/4] Balloon inhibit enhancements, vfio restriction

[Alex Williamson]

On Wed, 8 Aug 2018 00:58:32 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Aug 07, 2018 at 01:53:03PM -0600, Alex Williamson wrote:
> > On Tue, 7 Aug 2018 22:44:56 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Tue, Aug 07, 2018 at 01:31:21PM -0600, Alex Williamson wrote:  
> > > > v3:
> > > >  - Drop "nested" term in commit log (David)
> > > >  - Adopt suggested wording in ccw code (Cornelia)
> > > >  - Explain balloon inhibitor usage in vfio common (Peter)
> > > >  - Fix to call inhibitor prior to re-using existing containers
> > > >    to avoid gap that pinning may have occurred in set container
> > > >    ioctl (self) - Peter, this change is the reason I didn't
> > > >    include your R-b.
> > > >  - Add R-b to patches 1 & 2
> > > > 
> > > > v2:
> > > >  - Use atomic ops for balloon inhibit counter (Peter)
> > > >  - Allow endpoint driver opt-in for ballooning, vfio-ccw opt-in by
> > > >    default, vfio-pci opt-in by device option, only allowed for mdev
> > > >    devices, no support added for platform as there are no platform
> > > >    mdev devices.
> > > > 
> > > > See patch 3/4 for detailed explanation why ballooning and device
> > > > assignment typically don't mix.  If this eventually changes, flags
> > > > on the iommu info struct or perhaps device info struct can inform
> > > > us for automatic opt-in.  Thanks,
> > > > 
> > > > Alex    
> > > 
> > > One of the issues with pass-through is that it breaks overcommit
> > > through swap. ballooning seems to offer one solution, instead of
> > > making it work this patch just attempts to block ballooning.
> > > 
> > > I guess it's better than corrupting memory but I personally find this
> > > approach disappointing.  
> > 
> > Memory hotplug is the way to achieve variable density with assigned
> > device VMs, otherwise look towards approaches like mdev and shared
> > virtual addresses with PASID support.  We cannot shoehorn page faulting
> > without both hardware and software support.  Some class of "legacy"
> > device assignment will always have this incompatibility.  Thanks,
> > 
> > Alex  
> 
> I'm not sure I agree.
> 
> At least with VTD, it seems entirely possible to change e.g. a PMD
> atomically to point to a different set of PTEs, then flush.
> That will allow removing memory at high granularity for
> an arbitrary device without mdev or PASID dependency.
> 
> I suspect most IOMMUs are like this.
> 
> IIUC doing that within guest right now will cause a range to be unmapped
> and them mapped again, which I suspect only works if we are lucky and
> device does not access the range during this time.
> 
> So at some level it's a theoretical bug we would do well to fix,
> and then we can support ballooning better.

Being able to unmap the page atomically from the IOMMU is one aspect,
the other is re-mapping the page when the balloon is deflated, which is
currently done only via a page fault.  We cannot guarantee that a vCPU
will touch a page before the IO device does, so something needs to
fault in that page for the IOMMU.  So we have:

 - How do we handle re-mapping pages as the balloon is deflated?
   - IOMMU page faults?  Requires PRI, IOMMU & endpoint support.
   - Some new MMU notifier hook?  Not sure WILLNEED is appropriate here.

 - How do we handle un-mapping pages as the balloon is inflated?
   - Rewrite the kernel IOMMU API and IOMMU drivers to allow unmapping
     sub-pages within previous mappings.
   - MMU notifier hook to trigger above non-existent code?
   - Alternatively, sacrificing IOTLB performance and probably kernel
     bloat by using only PAGE_SIZE IOMMU mappings.

Maybe some of these will evolve over time, SVA efforts are working on
some of these interfaces, but apparently device assignment users have
been getting along just fine without ballooning for many years.  With
physical devices, or even modern VFs, it seems hard to push density
beyond what we can handle with memory hotplug.  Perhaps as we get into
scalable IOV type approaches we can opt-in more mediated devices by
default.  It seems like we're just going around in circles here though,
anything more than preventing QEMU from shooting itself is a long term
goal touching multiple levels of the stack. Thanks,

Alex

Re: [Qemu-devel] [PATCH v3 0/4] Balloon inhibit enhancements, vfio restriction

[Michael S. Tsirkin]

On Tue, Aug 07, 2018 at 04:40:33PM -0600, Alex Williamson wrote:
> Maybe some of these will evolve over time, SVA efforts are working on
> some of these interfaces, but apparently device assignment users have
> been getting along just fine without ballooning for many years.

But not any more I think.  It takes all the running you can do, to keep
in the same place.

Overcommit with device specific drivers is one of the things that
containers do better than VMs. If VMs had a better overcommit story with
PT devices, it would be interesting IMHO.

> With
> physical devices, or even modern VFs, it seems hard to push density
> beyond what we can handle with memory hotplug.  Perhaps as we get into
> scalable IOV type approaches we can opt-in more mediated devices by
> default.

I'm not sure what does mediated have to do with it though.  It seems
weird to fix internal Linux or even system call interfaces being
inadequate with custom hardware.

> It seems like we're just going around in circles here though,
> anything more than preventing QEMU from shooting itself is a long term
> goal touching multiple levels of the stack.

It's just QEMU and the kernel, I don't see why any other levels would be
involved. And it looks like we both agree it is a bug in the current VTD
emulation even though current guests do not trigger it.

I agree it's more work than just blocking things out, I am not making an
argument for nacking this specific patch, but I do hope this thread
motivates someone to look into it.

-- 
MST

Re: [Qemu-devel] [PATCH v3 0/4] Balloon inhibit enhancements, vfio restriction

[Peter Xu]

On Wed, Aug 08, 2018 at 12:58:32AM +0300, Michael S. Tsirkin wrote:
> At least with VTD, it seems entirely possible to change e.g. a PMD
> atomically to point to a different set of PTEs, then flush.
> That will allow removing memory at high granularity for
> an arbitrary device without mdev or PASID dependency.

My understanding is that the guest driver should prohibit this kind of
operation (say, modifying PMD).  Actually I don't see how it can
happen in Linux if the kernel drivers always call the IOMMU API since
there are only map/unmap APIs rather than this atomic-modify API.

The thing is that IMHO it's the guest driver's responsibility to make
sure the pages will never be used by the device before it removes the
entry (including modifying the PMD since that actually removes all the
entries on the old PMD).  If not, I would see it a guest kernel bug
instead of the bug in the emulation code.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 0/4] Balloon inhibit enhancements, vfio restriction

[Alex Williamson]

On Wed, 8 Aug 2018 11:45:43 +0800
Peter Xu <peterx@redhat.com> wrote:

> On Wed, Aug 08, 2018 at 12:58:32AM +0300, Michael S. Tsirkin wrote:
> > At least with VTD, it seems entirely possible to change e.g. a PMD
> > atomically to point to a different set of PTEs, then flush.
> > That will allow removing memory at high granularity for
> > an arbitrary device without mdev or PASID dependency.  
> 
> My understanding is that the guest driver should prohibit this kind of
> operation (say, modifying PMD).

There's currently no need for this sort of operation within the dma api
and the iommu api doesn't offer it either.

> Actually I don't see how it can
> happen in Linux if the kernel drivers always call the IOMMU API since
> there are only map/unmap APIs rather than this atomic-modify API.

Exactly, the vfio dma mapping api is just an extension of the iommu api
and there's only map and unmap.  Furthermore, unmap can currently return
more than requested if the original mapping made use of superpages in
the iommu, so the only way to achieve page level granularity is to make
only page size mappings.  Otherwise we're talking about new apis
across the board.
 
> The thing is that IMHO it's the guest driver's responsibility to make
> sure the pages will never be used by the device before it removes the
> entry (including modifying the PMD since that actually removes all the
> entries on the old PMD).  If not, I would see it a guest kernel bug
> instead of the bug in the emulation code.

This is why there is no atomic modify in the dma api, we have drivers
that directly manage the buffers for a device and know when it's in use
and when it's not.  There's never a need, currently, to replace the iova
mapping for a single page within a larger buffer.  Maybe the dma api
could also find use for it, but it seems more unique to the iommu api
that we have a "buffer", which happens to be a contiguous RAM region
for the VM, where we do want to change the mapping of a single page.
That single page might currently be mapped by a 2MB or 1GB page in the
case of Intel, or by an arbitrary page size in the case of AMD.  vfio
is the driver managing these mappings, but versus the dma api, we don't
have any insight to the device behavior, including inflight dma.  We can
stop all dma for the device, but not without interfering and potentially
breaking the behavior of the device.

So again, I think this comes down to new iommu driver support and new
iommu apis and new vfio apis to enable some sort of atomic update
interface, or sacrificing performance and adding bloat by forcing page
size mappings.  Thanks,

Alex

Re: [Qemu-devel] [PATCH v3 0/4] Balloon inhibit enhancements, vfio restriction

[Michael S. Tsirkin]

On Wed, Aug 08, 2018 at 04:23:04PM -0600, Alex Williamson wrote:
> So again, I think this comes down to new iommu driver support and new
> iommu apis and new vfio apis to enable some sort of atomic update
> interface,

Oh absolutely. My point is some guest OS can start using atomic updates
at any time since it's something IOMMU hardware supports.  Adherence to
a hardware spec would be preferable to adherence to an internal Linux
API.  I appreciate it's not an easy task involving host Linux and QEMU
changes.

-- 
MST

Re: [Qemu-devel] [PATCH v3 0/4] Balloon inhibit enhancements, vfio restriction

[Michael S. Tsirkin]

On Wed, Aug 08, 2018 at 11:45:43AM +0800, Peter Xu wrote:
> On Wed, Aug 08, 2018 at 12:58:32AM +0300, Michael S. Tsirkin wrote:
> > At least with VTD, it seems entirely possible to change e.g. a PMD
> > atomically to point to a different set of PTEs, then flush.
> > That will allow removing memory at high granularity for
> > an arbitrary device without mdev or PASID dependency.
> 
> My understanding is that the guest driver should prohibit this kind of
> operation (say, modifying PMD).

Interesting.  Which part of the VTD spec prohibits this?

> Actually I don't see how it can
> happen in Linux if the kernel drivers always call the IOMMU API since
> there are only map/unmap APIs rather than this atomic-modify API.

It could happen with a non-Linux guest which might have a different API.

> The thing is that IMHO it's the guest driver's responsibility to make
> sure the pages will never be used by the device before it removes the
> entry (including modifying the PMD since that actually removes all the
> entries on the old PMD).

If you switch PMDs atomically from one set of valid PTEs to another,
then flush, then as far as I could see it just works in the hardware
VTD, but not in the emulated VTD. So that's a difference in
behaviour. Maybe we are lucky and no one does that.

>  If not, I would see it a guest kernel bug
> instead of the bug in the emulation code.
> 
> Thanks,
> 
> -- 
> Peter Xu

Re: [Qemu-devel] [PATCH v3 0/4] Balloon inhibit enhancements, vfio restriction

[Peter Xu]

On Thu, Aug 09, 2018 at 12:23:43PM +0300, Michael S. Tsirkin wrote:
> On Wed, Aug 08, 2018 at 11:45:43AM +0800, Peter Xu wrote:
> > On Wed, Aug 08, 2018 at 12:58:32AM +0300, Michael S. Tsirkin wrote:
> > > At least with VTD, it seems entirely possible to change e.g. a PMD
> > > atomically to point to a different set of PTEs, then flush.
> > > That will allow removing memory at high granularity for
> > > an arbitrary device without mdev or PASID dependency.
> > 
> > My understanding is that the guest driver should prohibit this kind of
> > operation (say, modifying PMD).
> 
> Interesting.  Which part of the VTD spec prohibits this?
> 
> > Actually I don't see how it can
> > happen in Linux if the kernel drivers always call the IOMMU API since
> > there are only map/unmap APIs rather than this atomic-modify API.
> 
> It could happen with a non-Linux guest which might have a different API.
> 
> > The thing is that IMHO it's the guest driver's responsibility to make
> > sure the pages will never be used by the device before it removes the
> > entry (including modifying the PMD since that actually removes all the
> > entries on the old PMD).
> 
> If you switch PMDs atomically from one set of valid PTEs to another,
> then flush, then as far as I could see it just works in the hardware
> VTD, but not in the emulated VTD. So that's a difference in
> behaviour. Maybe we are lucky and no one does that.

Yes, but AFAICT that's also the best we can have now since the
userspace QEMU (or say, the VT-d emulation code) cannot really modify
a real PMD that the hardware uses - it can only call the VFIO APIs,
and finally it boils down again to the host kernel IOMMU APIs to do
map or unmap only.  So it's a impossible task until we provide such an
interface through the whole IOMMU/VFIO/... stack just like what you
have discussed in the other thread.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 0/4] Balloon inhibit enhancements, vfio restriction

[Michael S. Tsirkin]

On Thu, Aug 09, 2018 at 05:37:58PM +0800, Peter Xu wrote:
> On Thu, Aug 09, 2018 at 12:23:43PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Aug 08, 2018 at 11:45:43AM +0800, Peter Xu wrote:
> > > On Wed, Aug 08, 2018 at 12:58:32AM +0300, Michael S. Tsirkin wrote:
> > > > At least with VTD, it seems entirely possible to change e.g. a PMD
> > > > atomically to point to a different set of PTEs, then flush.
> > > > That will allow removing memory at high granularity for
> > > > an arbitrary device without mdev or PASID dependency.
> > > 
> > > My understanding is that the guest driver should prohibit this kind of
> > > operation (say, modifying PMD).
> > 
> > Interesting.  Which part of the VTD spec prohibits this?
> > 
> > > Actually I don't see how it can
> > > happen in Linux if the kernel drivers always call the IOMMU API since
> > > there are only map/unmap APIs rather than this atomic-modify API.
> > 
> > It could happen with a non-Linux guest which might have a different API.
> > 
> > > The thing is that IMHO it's the guest driver's responsibility to make
> > > sure the pages will never be used by the device before it removes the
> > > entry (including modifying the PMD since that actually removes all the
> > > entries on the old PMD).
> > 
> > If you switch PMDs atomically from one set of valid PTEs to another,
> > then flush, then as far as I could see it just works in the hardware
> > VTD, but not in the emulated VTD. So that's a difference in
> > behaviour. Maybe we are lucky and no one does that.
> 
> Yes, but AFAICT that's also the best we can have now since the
> userspace QEMU (or say, the VT-d emulation code) cannot really modify
> a real PMD that the hardware uses - it can only call the VFIO APIs,
> and finally it boils down again to the host kernel IOMMU APIs to do
> map or unmap only.  So it's a impossible task until we provide such an
> interface through the whole IOMMU/VFIO/... stack just like what you
> have discussed in the other thread.
> 
> Thanks,


This would need host kernel support, yes.

> -- 
> Peter Xu