[Qemu-devel] [PATCH 0/2] memory/intel_iommu: Generate error for incompatible usage

[Qemu-devel] [PATCH 0/2] memory/intel_iommu: Generate error for incompatible usage

[Alex Williamson]

VT-d emulation is currently incompatible with device assignment due
to intel_iommu's lack of support for memory_region_notify_iommu().
Alexey has proposed a nice addition to the MemoryRegionIOMMUOps
structure that adds callbacks when the first iommu notifier is
registered and the last is removed.  For POWER this will allow them
to switch the view of the iommu depending on whether anyone in
userspace is watching.  For VT-d I expect that eventually we'll use
these callbacks to enable and disable code paths so that we avoid
notifier overhead when there are no registered notifiy-ees.  For now,
we don't support calling memory_region_notify_iommu(), so this
signals an incompatible hardware configuration.  If we choose to make
CM=0 a user selectable option, something like this might continue to
be useful if we only support notifies via invalidations rather than
full VT-d data structure shadowing.

Even though we're currently working on enabling users like vfio-pci
with VT-d, I believe this is correct for the current state of things.
We might even want to consider this stable for v2.6.x so that
downstreams pick it up to avoid incompatible configurations.

Alexey, I hope I'm not stepping on your toes by extracting this
from your latest patch series.  Please let us know whether you
approve.  Thanks,

Alex

---

Alex Williamson (1):
      intel_iommu: Throw hw_error on notify_started

Alexey Kardashevskiy (1):
      memory: Add MemoryRegionIOMMUOps.notify_started/stopped callbacks


 hw/i386/intel_iommu.c |   12 ++++++++++++
 hw/vfio/common.c      |    5 +++--
 include/exec/memory.h |    8 +++++++-
 memory.c              |   10 +++++++++-
 4 files changed, 31 insertions(+), 4 deletions(-)

[Qemu-devel] [PATCH 1/2] memory: Add MemoryRegionIOMMUOps.notify_started/stopped callbacks

[Alex Williamson]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

The IOMMU driver may change behavior depending on whether a notifier
client is present.  In the case of POWER, this represents a change in
the visibility of the IOTLB, for other drivers such as intel-iommu and
future AMD-Vi emulation, notifier support is not yet enabled and this
provides the opportunity to flag that incompatibility.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
[new log & extracted from [PATCH qemu v17 12/12] spapr_iommu, vfio, memory: Notify IOMMU about starting/stopping listening]
---
 hw/vfio/common.c      |    5 +++--
 include/exec/memory.h |    8 +++++++-
 memory.c              |   10 +++++++++-
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index e51ed3a..94f4a6c 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -463,7 +463,8 @@ static void vfio_listener_region_del(MemoryListener *listener,
 
         QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
             if (giommu->iommu == section->mr) {
-                memory_region_unregister_iommu_notifier(&giommu->n);
+                memory_region_unregister_iommu_notifier(giommu->iommu,
+                                                        &giommu->n);
                 QLIST_REMOVE(giommu, giommu_next);
                 g_free(giommu);
                 break;
@@ -999,7 +1000,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
         QLIST_REMOVE(container, next);
 
         QLIST_FOREACH_SAFE(giommu, &container->giommu_list, giommu_next, tmp) {
-            memory_region_unregister_iommu_notifier(&giommu->n);
+            memory_region_unregister_iommu_notifier(giommu->iommu, &giommu->n);
             QLIST_REMOVE(giommu, giommu_next);
             g_free(giommu);
         }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 4ab6800..a6862fc 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -151,6 +151,10 @@ typedef struct MemoryRegionIOMMUOps MemoryRegionIOMMUOps;
 struct MemoryRegionIOMMUOps {
     /* Return a TLB entry that contains a given address. */
     IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool is_write);
+    /* Called when the first notifier is set */
+    void (*notify_started)(MemoryRegion *iommu);
+    /* Called when the last notifier is removed */
+    void (*notify_stopped)(MemoryRegion *iommu);
 };
 
 typedef struct CoalescedMemoryRange CoalescedMemoryRange;
@@ -611,9 +615,11 @@ void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n,
  * memory_region_unregister_iommu_notifier: unregister a notifier for
  * changes to IOMMU translation entries.
  *
+ * @mr: the memory region which was observed and for which notity_stopped()
+ *      needs to be called
  * @n: the notifier to be removed.
  */
-void memory_region_unregister_iommu_notifier(Notifier *n);
+void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n);
 
 /**
  * memory_region_name: get a memory region's name
diff --git a/memory.c b/memory.c
index 8ba496d..008ebe3 100644
--- a/memory.c
+++ b/memory.c
@@ -1499,6 +1499,10 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client)
 
 void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n)
 {
+    if (mr->iommu_ops->notify_started &&
+        QLIST_EMPTY(&mr->iommu_notify.notifiers)) {
+        mr->iommu_ops->notify_started(mr);
+    }
     notifier_list_add(&mr->iommu_notify, n);
 }
 
@@ -1522,9 +1526,13 @@ void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n,
     }
 }
 
-void memory_region_unregister_iommu_notifier(Notifier *n)
+void memory_region_unregister_iommu_notifier(MemoryRegion *mr, Notifier *n)
 {
     notifier_remove(n);
+    if (mr->iommu_ops->notify_stopped &&
+        QLIST_EMPTY(&mr->iommu_notify.notifiers)) {
+        mr->iommu_ops->notify_stopped(mr);
+    }
 }
 
 void memory_region_notify_iommu(MemoryRegion *mr,

[Qemu-devel] [PATCH 2/2] intel_iommu: Throw hw_error on notify_started

[Alex Williamson]

We don't currently support the MemoryRegionIOMMUOps notifier, so throw
an error should a device require it.

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

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 347718f..5eba704 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -24,6 +24,7 @@
 #include "exec/address-spaces.h"
 #include "intel_iommu_internal.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -1871,6 +1872,16 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr,
     return ret;
 }
 
+static void vtd_iommu_notify_started(MemoryRegion *iommu)
+{
+    VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
+
+    hw_error("Device at bus %s addr %02x.%d requires iommu notifier which "
+             "is currently not supported by intel-iommu emulation",
+             vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
+             PCI_FUNC(vtd_as->devfn));
+}
+
 static const VMStateDescription vtd_vmstate = {
     .name = "iommu-intel",
     .unmigratable = 1,
@@ -1938,6 +1949,7 @@ static void vtd_init(IntelIOMMUState *s)
     memset(s->womask, 0, DMAR_REG_SIZE);
 
     s->iommu_ops.translate = vtd_iommu_translate;
+    s->iommu_ops.notify_started = vtd_iommu_notify_started;
     s->root = 0;
     s->root_extended = false;
     s->dmar_enabled = false;

Re: [Qemu-devel] [PATCH 2/2] intel_iommu: Throw hw_error on notify_started

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1971 bytes --]

On Wed, Jun 15, 2016 at 09:56:16AM -0600, Alex Williamson wrote:
> We don't currently support the MemoryRegionIOMMUOps notifier, so throw
> an error should a device require it.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/i386/intel_iommu.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 347718f..5eba704 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -24,6 +24,7 @@
>  #include "exec/address-spaces.h"
>  #include "intel_iommu_internal.h"
>  #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
>  
>  /*#define DEBUG_INTEL_IOMMU*/
>  #ifdef DEBUG_INTEL_IOMMU
> @@ -1871,6 +1872,16 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr,
>      return ret;
>  }
>  
> +static void vtd_iommu_notify_started(MemoryRegion *iommu)
> +{
> +    VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
> +
> +    hw_error("Device at bus %s addr %02x.%d requires iommu notifier which "
> +             "is currently not supported by intel-iommu emulation",
> +             vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
> +             PCI_FUNC(vtd_as->devfn));
> +}
> +
>  static const VMStateDescription vtd_vmstate = {
>      .name = "iommu-intel",
>      .unmigratable = 1,
> @@ -1938,6 +1949,7 @@ static void vtd_init(IntelIOMMUState *s)
>      memset(s->womask, 0, DMAR_REG_SIZE);
>  
>      s->iommu_ops.translate = vtd_iommu_translate;
> +    s->iommu_ops.notify_started = vtd_iommu_notify_started;
>      s->root = 0;
>      s->root_extended = false;
>      s->dmar_enabled = false;
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/2] memory/intel_iommu: Generate error for incompatible usage

[Alexey Kardashevskiy]

On 16/06/16 01:56, Alex Williamson wrote:
> VT-d emulation is currently incompatible with device assignment due
> to intel_iommu's lack of support for memory_region_notify_iommu().
> Alexey has proposed a nice addition to the MemoryRegionIOMMUOps
> structure that adds callbacks when the first iommu notifier is
> registered and the last is removed.  For POWER this will allow them
> to switch the view of the iommu depending on whether anyone in
> userspace is watching.  For VT-d I expect that eventually we'll use
> these callbacks to enable and disable code paths so that we avoid
> notifier overhead when there are no registered notifiy-ees.  For now,
> we don't support calling memory_region_notify_iommu(), so this
> signals an incompatible hardware configuration.  If we choose to make
> CM=0 a user selectable option, something like this might continue to
> be useful if we only support notifies via invalidations rather than
> full VT-d data structure shadowing.
> 
> Even though we're currently working on enabling users like vfio-pci
> with VT-d, I believe this is correct for the current state of things.
> We might even want to consider this stable for v2.6.x so that
> downstreams pick it up to avoid incompatible configurations.
> 
> Alexey, I hope I'm not stepping on your toes by extracting this
> from your latest patch series.  Please let us know whether you
> approve.  Thanks,

I am totally fine with either way of accepting my patches, after all the
idea was yours, I just put it in the code :)


> 
> Alex
> 
> ---
> 
> Alex Williamson (1):
>       intel_iommu: Throw hw_error on notify_started
> 
> Alexey Kardashevskiy (1):
>       memory: Add MemoryRegionIOMMUOps.notify_started/stopped callbacks
> 
> 
>  hw/i386/intel_iommu.c |   12 ++++++++++++
>  hw/vfio/common.c      |    5 +++--
>  include/exec/memory.h |    8 +++++++-
>  memory.c              |   10 +++++++++-
>  4 files changed, 31 insertions(+), 4 deletions(-)
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 0/2] memory/intel_iommu: Generate error for incompatible usage

[Peter Xu]

On Wed, Jun 15, 2016 at 09:56:03AM -0600, Alex Williamson wrote:
> VT-d emulation is currently incompatible with device assignment due
> to intel_iommu's lack of support for memory_region_notify_iommu().
> Alexey has proposed a nice addition to the MemoryRegionIOMMUOps
> structure that adds callbacks when the first iommu notifier is
> registered and the last is removed.  For POWER this will allow them
> to switch the view of the iommu depending on whether anyone in
> userspace is watching.  For VT-d I expect that eventually we'll use
> these callbacks to enable and disable code paths so that we avoid
> notifier overhead when there are no registered notifiy-ees.  For now,
> we don't support calling memory_region_notify_iommu(), so this
> signals an incompatible hardware configuration.  If we choose to make
> CM=0 a user selectable option, something like this might continue to
> be useful if we only support notifies via invalidations rather than
> full VT-d data structure shadowing.
> 
> Even though we're currently working on enabling users like vfio-pci
> with VT-d, I believe this is correct for the current state of things.
> We might even want to consider this stable for v2.6.x so that
> downstreams pick it up to avoid incompatible configurations.

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

(This is much more friendly than a dead loop.)

> 
> Alexey, I hope I'm not stepping on your toes by extracting this
> from your latest patch series.  Please let us know whether you
> approve.  Thanks,
> 
> Alex
> 
> ---
> 
> Alex Williamson (1):
>       intel_iommu: Throw hw_error on notify_started
> 
> Alexey Kardashevskiy (1):
>       memory: Add MemoryRegionIOMMUOps.notify_started/stopped callbacks
> 
> 
>  hw/i386/intel_iommu.c |   12 ++++++++++++
>  hw/vfio/common.c      |    5 +++--
>  include/exec/memory.h |    8 +++++++-
>  memory.c              |   10 +++++++++-
>  4 files changed, 31 insertions(+), 4 deletions(-)

Re: [Qemu-devel] [PATCH 2/2] intel_iommu: Throw hw_error on notify_started

[Marcel Apfelbaum]

On 06/15/2016 06:56 PM, Alex Williamson wrote:
> We don't currently support the MemoryRegionIOMMUOps notifier, so throw
> an error should a device require it.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>   hw/i386/intel_iommu.c |   12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 347718f..5eba704 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -24,6 +24,7 @@
>   #include "exec/address-spaces.h"
>   #include "intel_iommu_internal.h"
>   #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
>
>   /*#define DEBUG_INTEL_IOMMU*/
>   #ifdef DEBUG_INTEL_IOMMU
> @@ -1871,6 +1872,16 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr,
>       return ret;
>   }
>
> +static void vtd_iommu_notify_started(MemoryRegion *iommu)
> +{
> +    VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
> +
> +    hw_error("Device at bus %s addr %02x.%d requires iommu notifier which "
> +             "is currently not supported by intel-iommu emulation",
> +             vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
> +             PCI_FUNC(vtd_as->devfn));
> +}
> +
>   static const VMStateDescription vtd_vmstate = {
>       .name = "iommu-intel",
>       .unmigratable = 1,
> @@ -1938,6 +1949,7 @@ static void vtd_init(IntelIOMMUState *s)
>       memset(s->womask, 0, DMAR_REG_SIZE);
>
>       s->iommu_ops.translate = vtd_iommu_translate;
> +    s->iommu_ops.notify_started = vtd_iommu_notify_started;
>       s->root = 0;
>       s->root_extended = false;
>       s->dmar_enabled = false;
>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 0/2] memory/intel_iommu: Generate error for incompatible usage

[Alexey Kardashevskiy]

On 16/06/16 11:43, Alexey Kardashevskiy wrote:
> On 16/06/16 01:56, Alex Williamson wrote:
>> VT-d emulation is currently incompatible with device assignment due
>> to intel_iommu's lack of support for memory_region_notify_iommu().
>> Alexey has proposed a nice addition to the MemoryRegionIOMMUOps
>> structure that adds callbacks when the first iommu notifier is
>> registered and the last is removed.  For POWER this will allow them
>> to switch the view of the iommu depending on whether anyone in
>> userspace is watching.  For VT-d I expect that eventually we'll use
>> these callbacks to enable and disable code paths so that we avoid
>> notifier overhead when there are no registered notifiy-ees.  For now,
>> we don't support calling memory_region_notify_iommu(), so this
>> signals an incompatible hardware configuration.  If we choose to make
>> CM=0 a user selectable option, something like this might continue to
>> be useful if we only support notifies via invalidations rather than
>> full VT-d data structure shadowing.
>>
>> Even though we're currently working on enabling users like vfio-pci
>> with VT-d, I believe this is correct for the current state of things.
>> We might even want to consider this stable for v2.6.x so that
>> downstreams pick it up to avoid incompatible configurations.
>>
>> Alexey, I hope I'm not stepping on your toes by extracting this
>> from your latest patch series.  Please let us know whether you
>> approve.  Thanks,
> 
> I am totally fine with either way of accepting my patches, after all the
> idea was yours, I just put it in the code :)

Alex, when are you planning on sending pull request with this patch? I am
asking as I'd prefer to respin "spapr_pci/spapr_pci_vfio: Support Dynamic
DMA" on top of it (assuming David rebases his queue on top of it) and avoid
doing unnecessary work. Thanks.


> 
> 
>>
>> Alex
>>
>> ---
>>
>> Alex Williamson (1):
>>       intel_iommu: Throw hw_error on notify_started
>>
>> Alexey Kardashevskiy (1):
>>       memory: Add MemoryRegionIOMMUOps.notify_started/stopped callbacks
>>
>>
>>  hw/i386/intel_iommu.c |   12 ++++++++++++
>>  hw/vfio/common.c      |    5 +++--
>>  include/exec/memory.h |    8 +++++++-
>>  memory.c              |   10 +++++++++-
>>  4 files changed, 31 insertions(+), 4 deletions(-)


-- 
Alexey

Re: [Qemu-devel] [PATCH 0/2] memory/intel_iommu: Generate error for incompatible usage

[Alex Williamson]

On Mon, 27 Jun 2016 14:46:12 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 16/06/16 11:43, Alexey Kardashevskiy wrote:
> > On 16/06/16 01:56, Alex Williamson wrote:  
> >> VT-d emulation is currently incompatible with device assignment due
> >> to intel_iommu's lack of support for memory_region_notify_iommu().
> >> Alexey has proposed a nice addition to the MemoryRegionIOMMUOps
> >> structure that adds callbacks when the first iommu notifier is
> >> registered and the last is removed.  For POWER this will allow them
> >> to switch the view of the iommu depending on whether anyone in
> >> userspace is watching.  For VT-d I expect that eventually we'll use
> >> these callbacks to enable and disable code paths so that we avoid
> >> notifier overhead when there are no registered notifiy-ees.  For now,
> >> we don't support calling memory_region_notify_iommu(), so this
> >> signals an incompatible hardware configuration.  If we choose to make
> >> CM=0 a user selectable option, something like this might continue to
> >> be useful if we only support notifies via invalidations rather than
> >> full VT-d data structure shadowing.
> >>
> >> Even though we're currently working on enabling users like vfio-pci
> >> with VT-d, I believe this is correct for the current state of things.
> >> We might even want to consider this stable for v2.6.x so that
> >> downstreams pick it up to avoid incompatible configurations.
> >>
> >> Alexey, I hope I'm not stepping on your toes by extracting this
> >> from your latest patch series.  Please let us know whether you
> >> approve.  Thanks,  
> > 
> > I am totally fine with either way of accepting my patches, after all the
> > idea was yours, I just put it in the code :)  
> 
> Alex, when are you planning on sending pull request with this patch? I am
> asking as I'd prefer to respin "spapr_pci/spapr_pci_vfio: Support Dynamic
> DMA" on top of it (assuming David rebases his queue on top of it) and avoid
> doing unnecessary work. Thanks.

The only file I maintain from this series is just minor collateral from
the api change.  I'll therefore redirect this question to Paolo, but
I'm happy to pull it through my tree with his ack.  Thanks,

Alex


> >> ---
> >>
> >> Alex Williamson (1):
> >>       intel_iommu: Throw hw_error on notify_started
> >>
> >> Alexey Kardashevskiy (1):
> >>       memory: Add MemoryRegionIOMMUOps.notify_started/stopped callbacks
> >>
> >>
> >>  hw/i386/intel_iommu.c |   12 ++++++++++++
> >>  hw/vfio/common.c      |    5 +++--
> >>  include/exec/memory.h |    8 +++++++-
> >>  memory.c              |   10 +++++++++-
> >>  4 files changed, 31 insertions(+), 4 deletions(-)  
> 
> 

Re: [Qemu-devel] [PATCH 0/2] memory/intel_iommu: Generate error for incompatible usage

[Alex Williamson]

Paolo & Michael,

Any comments on this series?  I think we need Paolo's ack for the memory
changes and either of your ack for hw/i386/.  I'm happy to pull this
through my tree with your approval though.  Thanks,

Alex

On Wed, 15 Jun 2016 09:56:03 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> VT-d emulation is currently incompatible with device assignment due
> to intel_iommu's lack of support for memory_region_notify_iommu().
> Alexey has proposed a nice addition to the MemoryRegionIOMMUOps
> structure that adds callbacks when the first iommu notifier is
> registered and the last is removed.  For POWER this will allow them
> to switch the view of the iommu depending on whether anyone in
> userspace is watching.  For VT-d I expect that eventually we'll use
> these callbacks to enable and disable code paths so that we avoid
> notifier overhead when there are no registered notifiy-ees.  For now,
> we don't support calling memory_region_notify_iommu(), so this
> signals an incompatible hardware configuration.  If we choose to make
> CM=0 a user selectable option, something like this might continue to
> be useful if we only support notifies via invalidations rather than
> full VT-d data structure shadowing.
> 
> Even though we're currently working on enabling users like vfio-pci
> with VT-d, I believe this is correct for the current state of things.
> We might even want to consider this stable for v2.6.x so that
> downstreams pick it up to avoid incompatible configurations.
> 
> Alexey, I hope I'm not stepping on your toes by extracting this
> from your latest patch series.  Please let us know whether you
> approve.  Thanks,
> 
> Alex
> 
> ---
> 
> Alex Williamson (1):
>       intel_iommu: Throw hw_error on notify_started
> 
> Alexey Kardashevskiy (1):
>       memory: Add MemoryRegionIOMMUOps.notify_started/stopped callbacks
> 
> 
>  hw/i386/intel_iommu.c |   12 ++++++++++++
>  hw/vfio/common.c      |    5 +++--
>  include/exec/memory.h |    8 +++++++-
>  memory.c              |   10 +++++++++-
>  4 files changed, 31 insertions(+), 4 deletions(-)

Re: [Qemu-devel] [PATCH 0/2] memory/intel_iommu: Generate error for incompatible usage

[Paolo Bonzini]

On 28/06/2016 16:49, Alex Williamson wrote:
> Paolo & Michael,
> 
> Any comments on this series?  I think we need Paolo's ack for the memory
> changes and either of your ack for hw/i386/.  I'm happy to pull this
> through my tree with your approval though.  Thanks,

I think I already acked the callbacks, in any case the patches look good.

Paolo

> Alex
> 
> On Wed, 15 Jun 2016 09:56:03 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
>> VT-d emulation is currently incompatible with device assignment due
>> to intel_iommu's lack of support for memory_region_notify_iommu().
>> Alexey has proposed a nice addition to the MemoryRegionIOMMUOps
>> structure that adds callbacks when the first iommu notifier is
>> registered and the last is removed.  For POWER this will allow them
>> to switch the view of the iommu depending on whether anyone in
>> userspace is watching.  For VT-d I expect that eventually we'll use
>> these callbacks to enable and disable code paths so that we avoid
>> notifier overhead when there are no registered notifiy-ees.  For now,
>> we don't support calling memory_region_notify_iommu(), so this
>> signals an incompatible hardware configuration.  If we choose to make
>> CM=0 a user selectable option, something like this might continue to
>> be useful if we only support notifies via invalidations rather than
>> full VT-d data structure shadowing.
>>
>> Even though we're currently working on enabling users like vfio-pci
>> with VT-d, I believe this is correct for the current state of things.
>> We might even want to consider this stable for v2.6.x so that
>> downstreams pick it up to avoid incompatible configurations.
>>
>> Alexey, I hope I'm not stepping on your toes by extracting this
>> from your latest patch series.  Please let us know whether you
>> approve.  Thanks,
>>
>> Alex
>>
>> ---
>>
>> Alex Williamson (1):
>>       intel_iommu: Throw hw_error on notify_started
>>
>> Alexey Kardashevskiy (1):
>>       memory: Add MemoryRegionIOMMUOps.notify_started/stopped callbacks
>>
>>
>>  hw/i386/intel_iommu.c |   12 ++++++++++++
>>  hw/vfio/common.c      |    5 +++--
>>  include/exec/memory.h |    8 +++++++-
>>  memory.c              |   10 +++++++++-
>>  4 files changed, 31 insertions(+), 4 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/2] memory/intel_iommu: Generate error for incompatible usage

[Alex Williamson]

On Wed, 29 Jun 2016 12:53:39 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 28/06/2016 16:49, Alex Williamson wrote:
> > Paolo & Michael,
> > 
> > Any comments on this series?  I think we need Paolo's ack for the memory
> > changes and either of your ack for hw/i386/.  I'm happy to pull this
> > through my tree with your approval though.  Thanks,  
> 
> I think I already acked the callbacks, in any case the patches look good.

Hmm, I can't find any evidence of that and I'm not terribly comfortable
inferring an Acked-by where it is not explicitly given.  Can you send
a formal sign-off or point me to where I can find the one you're
thinking of?  Thanks,

Alex

> > On Wed, 15 Jun 2016 09:56:03 -0600
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> >   
> >> VT-d emulation is currently incompatible with device assignment due
> >> to intel_iommu's lack of support for memory_region_notify_iommu().
> >> Alexey has proposed a nice addition to the MemoryRegionIOMMUOps
> >> structure that adds callbacks when the first iommu notifier is
> >> registered and the last is removed.  For POWER this will allow them
> >> to switch the view of the iommu depending on whether anyone in
> >> userspace is watching.  For VT-d I expect that eventually we'll use
> >> these callbacks to enable and disable code paths so that we avoid
> >> notifier overhead when there are no registered notifiy-ees.  For now,
> >> we don't support calling memory_region_notify_iommu(), so this
> >> signals an incompatible hardware configuration.  If we choose to make
> >> CM=0 a user selectable option, something like this might continue to
> >> be useful if we only support notifies via invalidations rather than
> >> full VT-d data structure shadowing.
> >>
> >> Even though we're currently working on enabling users like vfio-pci
> >> with VT-d, I believe this is correct for the current state of things.
> >> We might even want to consider this stable for v2.6.x so that
> >> downstreams pick it up to avoid incompatible configurations.
> >>
> >> Alexey, I hope I'm not stepping on your toes by extracting this
> >> from your latest patch series.  Please let us know whether you
> >> approve.  Thanks,
> >>
> >> Alex
> >>
> >> ---
> >>
> >> Alex Williamson (1):
> >>       intel_iommu: Throw hw_error on notify_started
> >>
> >> Alexey Kardashevskiy (1):
> >>       memory: Add MemoryRegionIOMMUOps.notify_started/stopped callbacks
> >>
> >>
> >>  hw/i386/intel_iommu.c |   12 ++++++++++++
> >>  hw/vfio/common.c      |    5 +++--
> >>  include/exec/memory.h |    8 +++++++-
> >>  memory.c              |   10 +++++++++-
> >>  4 files changed, 31 insertions(+), 4 deletions(-)  
> >   

Re: [Qemu-devel] [PATCH 0/2] memory/intel_iommu: Generate error for incompatible usage

[Paolo Bonzini]

On 29/06/2016 17:56, Alex Williamson wrote:
>>> > > Any comments on this series?  I think we need Paolo's ack for the memory
>>> > > changes and either of your ack for hw/i386/.  I'm happy to pull this
>>> > > through my tree with your approval though.  Thanks,  
>> > 
>> > I think I already acked the callbacks, in any case the patches look good.
> Hmm, I can't find any evidence of that and I'm not terribly comfortable
> inferring an Acked-by where it is not explicitly given.  Can you send
> a formal sign-off or point me to where I can find the one you're
> thinking of?  Thanks,

You're right, I had seen Alexey's patch but I hadn't acked it.  Sorry
for the delay then!

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

Paolo