[PATCH 0/2] iommu/virtio: Enable IOMMU_CAP_DERRED_FLUSH

[PATCH 0/2] iommu/virtio: Enable IOMMU_CAP_DERRED_FLUSH

[Niklas Schnelle]

Hi All,

Previously I used virtio-iommu as a non-s390x test vehicle[0] for the
single queue flushing scheme introduced by my s390x DMA API conversion
series[1]. For this I modified virtio-iommu to a) use .iotlb_sync_map
and b) enable IOMMU_CAP_DEFERRED_FLUSH. It turned out that deferred
flush and even just the introduction of ops->iotlb_sync_map yield
performance uplift[2] even with per-CPU queues. So here is a small
series of these two changes. This still applies on top of my series[1]
because its first patch titled "iommu: Allow .iotlb_sync_map to fail and
handle s390's -ENOMEM return" enable ops->iotlb_sync_map to return
errors and virtio-iommu's sync can fail. This also makes sure there is
no merge conflict with that series.

The code is also available on the b4/viommu-deferred-flush branch of my
kernel.org git repository[3]

Thanks,
Niklas

[0] https://lore.kernel.org/lkml/20230726111433.1105665-1-schnelle@linux.ibm.com/
[1] https://lore.kernel.org/lkml/20230825-dma_iommu-v12-0-4134455994a7@linux.ibm.com/
[2] https://lore.kernel.org/lkml/20230802123612.GA6142@myrica/
[3] https://git.kernel.org/pub/scm/linux/kernel/git/niks/linux.git/log/?h=b4/viommu-deferred-flush

Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
Niklas Schnelle (2):
      iommu/virtio: Make use of ops->iotlb_sync_map
      iommu/virtio: Add ops->flush_iotlb_all and enable deferred flush

 drivers/iommu/virtio-iommu.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)
---
base-commit: fcf2e20eb3ef21e36bc31fbbc84e37d17030fa16
change-id: 20230825-viommu-sync-map-1bf0cc4fdc15

Best regards,
-- 
Niklas Schnelle
Linux on Z Development

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement - https://www.ibm.com/privacy 

[PATCH 1/2] iommu/virtio: Make use of ops->iotlb_sync_map

[Niklas Schnelle]

Pull out the sync operation from viommu_map_pages() by implementing
ops->iotlb_sync_map. This allows the common IOMMU code to map multiple
elements of an sg with a single sync (see iommu_map_sg()). Furthermore,
it is also a requirement for IOMMU_CAP_DEFERRED_FLUSH.

Link: https://lore.kernel.org/lkml/20230726111433.1105665-1-schnelle@linux.ibm.com/
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/iommu/virtio-iommu.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 3551ed057774..fb73dec5b953 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -843,7 +843,7 @@ static int viommu_map_pages(struct iommu_domain *domain, unsigned long iova,
 			.flags		= cpu_to_le32(flags),
 		};
 
-		ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
+		ret = viommu_add_req(vdomain->viommu, &map, sizeof(map));
 		if (ret) {
 			viommu_del_mappings(vdomain, iova, end);
 			return ret;
@@ -909,9 +909,21 @@ static void viommu_iotlb_sync(struct iommu_domain *domain,
 {
 	struct viommu_domain *vdomain = to_viommu_domain(domain);
 
+	if (!vdomain->nr_endpoints)
+		return;
 	viommu_sync_req(vdomain->viommu);
 }
 
+static int viommu_iotlb_sync_map(struct iommu_domain *domain,
+				 unsigned long iova, size_t size)
+{
+	struct viommu_domain *vdomain = to_viommu_domain(domain);
+
+	if (!vdomain->nr_endpoints)
+		return 0;
+	return viommu_sync_req(vdomain->viommu);
+}
+
 static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
 {
 	struct iommu_resv_region *entry, *new_entry, *msi = NULL;
@@ -1058,6 +1070,7 @@ static struct iommu_ops viommu_ops = {
 		.unmap_pages		= viommu_unmap_pages,
 		.iova_to_phys		= viommu_iova_to_phys,
 		.iotlb_sync		= viommu_iotlb_sync,
+		.iotlb_sync_map		= viommu_iotlb_sync_map,
 		.free			= viommu_domain_free,
 	}
 };

-- 
2.39.2

Re: [PATCH 1/2] iommu/virtio: Make use of ops->iotlb_sync_map

[Jean-Philippe Brucker]

Hi Niklas,

Thanks for following up with these patches

On Fri, Aug 25, 2023 at 05:21:25PM +0200, Niklas Schnelle wrote:
> Pull out the sync operation from viommu_map_pages() by implementing
> ops->iotlb_sync_map. This allows the common IOMMU code to map multiple
> elements of an sg with a single sync (see iommu_map_sg()). Furthermore,
> it is also a requirement for IOMMU_CAP_DEFERRED_FLUSH.
> 
> Link: https://lore.kernel.org/lkml/20230726111433.1105665-1-schnelle@linux.ibm.com/
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>  drivers/iommu/virtio-iommu.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 3551ed057774..fb73dec5b953 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -843,7 +843,7 @@ static int viommu_map_pages(struct iommu_domain *domain, unsigned long iova,
>  			.flags		= cpu_to_le32(flags),
>  		};
>  
> -		ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
> +		ret = viommu_add_req(vdomain->viommu, &map, sizeof(map));
>  		if (ret) {
>  			viommu_del_mappings(vdomain, iova, end);
>  			return ret;
> @@ -909,9 +909,21 @@ static void viommu_iotlb_sync(struct iommu_domain *domain,
>  {
>  	struct viommu_domain *vdomain = to_viommu_domain(domain);
>  
> +	if (!vdomain->nr_endpoints)
> +		return;

I was wondering about these nr_endpoints checks, which seemed unnecessary:
if map()/unmap() were called with no attached endpoints, then no requests
were added to the queue, and viommu_sync_req() below is a nop.

But at least viommu_iotlb_sync_map() and viommu_flush_iotlb_all() need to
handle being called before the domain is finalized (for example by
iommu_create_device_direct_mappings()). In that case vdomain->viommu is
NULL so if you add a NULL check in viommu_sync_req() then you should be
able to drop the nr_endpoints checks in both patches.

Thanks,
Jean

>  	viommu_sync_req(vdomain->viommu);
>  }
>  
> +static int viommu_iotlb_sync_map(struct iommu_domain *domain,
> +				 unsigned long iova, size_t size)
> +{
> +	struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> +	if (!vdomain->nr_endpoints)
> +		return 0;
> +	return viommu_sync_req(vdomain->viommu);
> +}
> +
>  static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
>  {
>  	struct iommu_resv_region *entry, *new_entry, *msi = NULL;
> @@ -1058,6 +1070,7 @@ static struct iommu_ops viommu_ops = {
>  		.unmap_pages		= viommu_unmap_pages,
>  		.iova_to_phys		= viommu_iova_to_phys,
>  		.iotlb_sync		= viommu_iotlb_sync,
> +		.iotlb_sync_map		= viommu_iotlb_sync_map,
>  		.free			= viommu_domain_free,
>  	}
>  };
> 
> -- 
> 2.39.2
> 

[PATCH 2/2] iommu/virtio: Add ops->flush_iotlb_all and enable deferred flush

[Niklas Schnelle]

Add ops->flush_iotlb_all operation to enable virtio-iommu for the
dma-iommu deferred flush scheme. This results inn a significant increase
in performance in exchange for a window in which devices can still
access previously IOMMU mapped memory. To get back to the prior behavior
iommu.strict=1 may be set on the kernel command line.

Link: https://lore.kernel.org/lkml/20230802123612.GA6142@myrica/
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/iommu/virtio-iommu.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index fb73dec5b953..1b7526494490 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -924,6 +924,15 @@ static int viommu_iotlb_sync_map(struct iommu_domain *domain,
 	return viommu_sync_req(vdomain->viommu);
 }
 
+static void viommu_flush_iotlb_all(struct iommu_domain *domain)
+{
+	struct viommu_domain *vdomain = to_viommu_domain(domain);
+
+	if (!vdomain->nr_endpoints)
+		return;
+	viommu_sync_req(vdomain->viommu);
+}
+
 static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
 {
 	struct iommu_resv_region *entry, *new_entry, *msi = NULL;
@@ -1049,6 +1058,8 @@ static bool viommu_capable(struct device *dev, enum iommu_cap cap)
 	switch (cap) {
 	case IOMMU_CAP_CACHE_COHERENCY:
 		return true;
+	case IOMMU_CAP_DEFERRED_FLUSH:
+		return true;
 	default:
 		return false;
 	}
@@ -1069,6 +1080,7 @@ static struct iommu_ops viommu_ops = {
 		.map_pages		= viommu_map_pages,
 		.unmap_pages		= viommu_unmap_pages,
 		.iova_to_phys		= viommu_iova_to_phys,
+		.flush_iotlb_all	= viommu_flush_iotlb_all,
 		.iotlb_sync		= viommu_iotlb_sync,
 		.iotlb_sync_map		= viommu_iotlb_sync_map,
 		.free			= viommu_domain_free,

-- 
2.39.2

Re: [PATCH 2/2] iommu/virtio: Add ops->flush_iotlb_all and enable deferred flush

[Jean-Philippe Brucker]

On Fri, Aug 25, 2023 at 05:21:26PM +0200, Niklas Schnelle wrote:
> Add ops->flush_iotlb_all operation to enable virtio-iommu for the
> dma-iommu deferred flush scheme. This results inn a significant increase

in

> in performance in exchange for a window in which devices can still
> access previously IOMMU mapped memory. To get back to the prior behavior
> iommu.strict=1 may be set on the kernel command line.

Maybe add that it depends on CONFIG_IOMMU_DEFAULT_DMA_{LAZY,STRICT} as
well, because I've seen kernel configs that enable either.

> 
> Link: https://lore.kernel.org/lkml/20230802123612.GA6142@myrica/
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>  drivers/iommu/virtio-iommu.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index fb73dec5b953..1b7526494490 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -924,6 +924,15 @@ static int viommu_iotlb_sync_map(struct iommu_domain *domain,
>  	return viommu_sync_req(vdomain->viommu);
>  }
>  
> +static void viommu_flush_iotlb_all(struct iommu_domain *domain)
> +{
> +	struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> +	if (!vdomain->nr_endpoints)
> +		return;

As for patch 1, a NULL check in viommu_sync_req() would allow dropping
this one

Thanks,
Jean

> +	viommu_sync_req(vdomain->viommu);
> +}
> +
>  static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
>  {
>  	struct iommu_resv_region *entry, *new_entry, *msi = NULL;
> @@ -1049,6 +1058,8 @@ static bool viommu_capable(struct device *dev, enum iommu_cap cap)
>  	switch (cap) {
>  	case IOMMU_CAP_CACHE_COHERENCY:
>  		return true;
> +	case IOMMU_CAP_DEFERRED_FLUSH:
> +		return true;
>  	default:
>  		return false;
>  	}
> @@ -1069,6 +1080,7 @@ static struct iommu_ops viommu_ops = {
>  		.map_pages		= viommu_map_pages,
>  		.unmap_pages		= viommu_unmap_pages,
>  		.iova_to_phys		= viommu_iova_to_phys,
> +		.flush_iotlb_all	= viommu_flush_iotlb_all,
>  		.iotlb_sync		= viommu_iotlb_sync,
>  		.iotlb_sync_map		= viommu_iotlb_sync_map,
>  		.free			= viommu_domain_free,
> 
> -- 
> 2.39.2
> 

Re: [PATCH 2/2] iommu/virtio: Add ops->flush_iotlb_all and enable deferred flush

[Robin Murphy]

On 2023-09-04 16:34, Jean-Philippe Brucker wrote:
> On Fri, Aug 25, 2023 at 05:21:26PM +0200, Niklas Schnelle wrote:
>> Add ops->flush_iotlb_all operation to enable virtio-iommu for the
>> dma-iommu deferred flush scheme. This results inn a significant increase
> 
> in
> 
>> in performance in exchange for a window in which devices can still
>> access previously IOMMU mapped memory. To get back to the prior behavior
>> iommu.strict=1 may be set on the kernel command line.
> 
> Maybe add that it depends on CONFIG_IOMMU_DEFAULT_DMA_{LAZY,STRICT} as
> well, because I've seen kernel configs that enable either.

Indeed, I'd be inclined phrase it in terms of the driver now actually 
being able to honour lazy mode when requested (which happens to be the 
default on x86), rather than as if it might be some 
potentially-unexpected change in behaviour.

Thanks,
Robin.

>> Link: https://lore.kernel.org/lkml/20230802123612.GA6142@myrica/
>> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
>> ---
>>   drivers/iommu/virtio-iommu.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
>> index fb73dec5b953..1b7526494490 100644
>> --- a/drivers/iommu/virtio-iommu.c
>> +++ b/drivers/iommu/virtio-iommu.c
>> @@ -924,6 +924,15 @@ static int viommu_iotlb_sync_map(struct iommu_domain *domain,
>>   	return viommu_sync_req(vdomain->viommu);
>>   }
>>   
>> +static void viommu_flush_iotlb_all(struct iommu_domain *domain)
>> +{
>> +	struct viommu_domain *vdomain = to_viommu_domain(domain);
>> +
>> +	if (!vdomain->nr_endpoints)
>> +		return;
> 
> As for patch 1, a NULL check in viommu_sync_req() would allow dropping
> this one
> 
> Thanks,
> Jean
> 
>> +	viommu_sync_req(vdomain->viommu);
>> +}
>> +
>>   static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
>>   {
>>   	struct iommu_resv_region *entry, *new_entry, *msi = NULL;
>> @@ -1049,6 +1058,8 @@ static bool viommu_capable(struct device *dev, enum iommu_cap cap)
>>   	switch (cap) {
>>   	case IOMMU_CAP_CACHE_COHERENCY:
>>   		return true;
>> +	case IOMMU_CAP_DEFERRED_FLUSH:
>> +		return true;
>>   	default:
>>   		return false;
>>   	}
>> @@ -1069,6 +1080,7 @@ static struct iommu_ops viommu_ops = {
>>   		.map_pages		= viommu_map_pages,
>>   		.unmap_pages		= viommu_unmap_pages,
>>   		.iova_to_phys		= viommu_iova_to_phys,
>> +		.flush_iotlb_all	= viommu_flush_iotlb_all,
>>   		.iotlb_sync		= viommu_iotlb_sync,
>>   		.iotlb_sync_map		= viommu_iotlb_sync_map,
>>   		.free			= viommu_domain_free,
>>
>> -- 
>> 2.39.2
>>

Re: [PATCH 2/2] iommu/virtio: Add ops->flush_iotlb_all and enable deferred flush

[Niklas Schnelle]

On Mon, 2023-09-04 at 17:33 +0100, Robin Murphy wrote:
> On 2023-09-04 16:34, Jean-Philippe Brucker wrote:
> > On Fri, Aug 25, 2023 at 05:21:26PM +0200, Niklas Schnelle wrote:
> > > Add ops->flush_iotlb_all operation to enable virtio-iommu for the
> > > dma-iommu deferred flush scheme. This results inn a significant increase
> > 
> > in
> > 
> > > in performance in exchange for a window in which devices can still
> > > access previously IOMMU mapped memory. To get back to the prior behavior
> > > iommu.strict=1 may be set on the kernel command line.
> > 
> > Maybe add that it depends on CONFIG_IOMMU_DEFAULT_DMA_{LAZY,STRICT} as
> > well, because I've seen kernel configs that enable either.
> 
> Indeed, I'd be inclined phrase it in terms of the driver now actually 
> being able to honour lazy mode when requested (which happens to be the 
> default on x86), rather than as if it might be some 
> potentially-unexpected change in behaviour.
> 
> Thanks,
> Robin.

I kept running this series on a KVM guest on my private workstation
(QEMU v8.0.4) and while running iperf3 on a passed-through Intel 82599
VF. I got a bunch of IOMMU events similar to the following as well as
card resets in the host.

..
[ 5959.338214] vfio-pci 0000:04:10.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0037 address=0x7b657064 flags=0x0000]
[ 5963.353429] ixgbe 0000:03:00.0 enp3s0: Detected Tx Unit Hang
                 Tx Queue             <0>
                 TDH, TDT             <93>, <9d>
                 next_to_use          <9d>
                 next_to_clean        <93>
               tx_buffer_info[next_to_clean]
                 time_stamp           <10019e800>
                 jiffies              <10019ec80>
...

I retested on v6.5 vanilla (guest & host) and still get the above
errors so luckily for me it doesn't seem to be caused by the new code
but I can't reproduce it without virtio-iommu. Any idea what could
cause this?


> 
> > > Link: https://lore.kernel.org/lkml/20230802123612.GA6142@myrica/
> > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > ---
> > >   drivers/iommu/virtio-iommu.c | 12 ++++++++++++
> > >   1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > > index fb73dec5b953..1b7526494490 100644
> > > --- a/drivers/iommu/virtio-iommu.c
> > > +++ b/drivers/iommu/virtio-iommu.c
> > > @@ -924,6 +924,15 @@ static int viommu_iotlb_sync_map(struct iommu_domain *domain,
> > >   	return viommu_sync_req(vdomain->viommu);
> > >   }
> > >   
> > > +static void viommu_flush_iotlb_all(struct iommu_domain *domain)
> > > +{
> > > +	struct viommu_domain *vdomain = to_viommu_domain(domain);
> > > +
> > > +	if (!vdomain->nr_endpoints)
> > > +		return;
> > 
> > As for patch 1, a NULL check in viommu_sync_req() would allow dropping
> > this one
> > 
> > Thanks,
> > Jean

Right, makes sense will move the check into viommu_sync_req() and add a
coment that it is there fore the cases where viommu_iotlb_sync() et al
get called before the IOMMU is set up.

> > 
> > > +	viommu_sync_req(vdomain->viommu);
> > > +}
> > > +
> > >   static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
> > >   {
> > >   	struct iommu_resv_region *entry, *new_entry, *msi = NULL;
> > > @@ -1049,6 +1058,8 @@ static bool viommu_capable(struct device *dev, enum iommu_cap cap)
> > >   	switch (cap) {
> > >   	case IOMMU_CAP_CACHE_COHERENCY:
> > >   		return true;
> > > +	case IOMMU_CAP_DEFERRED_FLUSH:
> > > +		return true;
> > >   	default:
> > >   		return false;
> > >   	}
> > > @@ -1069,6 +1080,7 @@ static struct iommu_ops viommu_ops = {
> > >   		.map_pages		= viommu_map_pages,
> > >   		.unmap_pages		= viommu_unmap_pages,
> > >   		.iova_to_phys		= viommu_iova_to_phys,
> > > +		.flush_iotlb_all	= viommu_flush_iotlb_all,
> > >   		.iotlb_sync		= viommu_iotlb_sync,
> > >   		.iotlb_sync_map		= viommu_iotlb_sync_map,
> > >   		.free			= viommu_domain_free,
> > > 
> > > -- 
> > > 2.39.2
> > > 

Re: [PATCH 2/2] iommu/virtio: Add ops->flush_iotlb_all and enable deferred flush

[Jean-Philippe Brucker]

On Wed, Sep 06, 2023 at 09:55:49AM +0200, Niklas Schnelle wrote:
> On Mon, 2023-09-04 at 17:33 +0100, Robin Murphy wrote:
> > On 2023-09-04 16:34, Jean-Philippe Brucker wrote:
> > > On Fri, Aug 25, 2023 at 05:21:26PM +0200, Niklas Schnelle wrote:
> > > > Add ops->flush_iotlb_all operation to enable virtio-iommu for the
> > > > dma-iommu deferred flush scheme. This results inn a significant increase
> > > 
> > > in
> > > 
> > > > in performance in exchange for a window in which devices can still
> > > > access previously IOMMU mapped memory. To get back to the prior behavior
> > > > iommu.strict=1 may be set on the kernel command line.
> > > 
> > > Maybe add that it depends on CONFIG_IOMMU_DEFAULT_DMA_{LAZY,STRICT} as
> > > well, because I've seen kernel configs that enable either.
> > 
> > Indeed, I'd be inclined phrase it in terms of the driver now actually 
> > being able to honour lazy mode when requested (which happens to be the 
> > default on x86), rather than as if it might be some 
> > potentially-unexpected change in behaviour.
> > 
> > Thanks,
> > Robin.
> 
> I kept running this series on a KVM guest on my private workstation
> (QEMU v8.0.4) and while running iperf3 on a passed-through Intel 82599
> VF. I got a bunch of IOMMU events similar to the following as well as
> card resets in the host.
> 
> ..
> [ 5959.338214] vfio-pci 0000:04:10.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0037 address=0x7b657064 flags=0x0000]
> [ 5963.353429] ixgbe 0000:03:00.0 enp3s0: Detected Tx Unit Hang
>                  Tx Queue             <0>
>                  TDH, TDT             <93>, <9d>
>                  next_to_use          <9d>
>                  next_to_clean        <93>
>                tx_buffer_info[next_to_clean]
>                  time_stamp           <10019e800>
>                  jiffies              <10019ec80>
> ...
> 
> I retested on v6.5 vanilla (guest & host) and still get the above
> errors so luckily for me it doesn't seem to be caused by the new code
> but I can't reproduce it without virtio-iommu. Any idea what could
> cause this?

Adding Eric in case this looks familiar.

I don't have hardware to test this but I guess QEMU system emulation may
be able to reproduce the issue since it has an AMD IOMMU (unmaintained)
and igb, I can give that a try.

Thanks,
Jean

> 
> > 
> > > > Link: https://lore.kernel.org/lkml/20230802123612.GA6142@myrica/
> > > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > > ---
> > > >   drivers/iommu/virtio-iommu.c | 12 ++++++++++++
> > > >   1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > > > index fb73dec5b953..1b7526494490 100644
> > > > --- a/drivers/iommu/virtio-iommu.c
> > > > +++ b/drivers/iommu/virtio-iommu.c
> > > > @@ -924,6 +924,15 @@ static int viommu_iotlb_sync_map(struct iommu_domain *domain,
> > > >   	return viommu_sync_req(vdomain->viommu);
> > > >   }
> > > >   
> > > > +static void viommu_flush_iotlb_all(struct iommu_domain *domain)
> > > > +{
> > > > +	struct viommu_domain *vdomain = to_viommu_domain(domain);
> > > > +
> > > > +	if (!vdomain->nr_endpoints)
> > > > +		return;
> > > 
> > > As for patch 1, a NULL check in viommu_sync_req() would allow dropping
> > > this one
> > > 
> > > Thanks,
> > > Jean
> 
> Right, makes sense will move the check into viommu_sync_req() and add a
> coment that it is there fore the cases where viommu_iotlb_sync() et al
> get called before the IOMMU is set up.
> 
> > > 
> > > > +	viommu_sync_req(vdomain->viommu);
> > > > +}
> > > > +
> > > >   static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
> > > >   {
> > > >   	struct iommu_resv_region *entry, *new_entry, *msi = NULL;
> > > > @@ -1049,6 +1058,8 @@ static bool viommu_capable(struct device *dev, enum iommu_cap cap)
> > > >   	switch (cap) {
> > > >   	case IOMMU_CAP_CACHE_COHERENCY:
> > > >   		return true;
> > > > +	case IOMMU_CAP_DEFERRED_FLUSH:
> > > > +		return true;
> > > >   	default:
> > > >   		return false;
> > > >   	}
> > > > @@ -1069,6 +1080,7 @@ static struct iommu_ops viommu_ops = {
> > > >   		.map_pages		= viommu_map_pages,
> > > >   		.unmap_pages		= viommu_unmap_pages,
> > > >   		.iova_to_phys		= viommu_iova_to_phys,
> > > > +		.flush_iotlb_all	= viommu_flush_iotlb_all,
> > > >   		.iotlb_sync		= viommu_iotlb_sync,
> > > >   		.iotlb_sync_map		= viommu_iotlb_sync_map,
> > > >   		.free			= viommu_domain_free,
> > > > 
> > > > -- 
> > > > 2.39.2
> > > > 
> 

Re: [PATCH 2/2] iommu/virtio: Add ops->flush_iotlb_all and enable deferred flush

[Eric Auger]

Hi,

On 9/6/23 15:20, Jean-Philippe Brucker wrote:
> On Wed, Sep 06, 2023 at 09:55:49AM +0200, Niklas Schnelle wrote:
>> On Mon, 2023-09-04 at 17:33 +0100, Robin Murphy wrote:
>>> On 2023-09-04 16:34, Jean-Philippe Brucker wrote:
>>>> On Fri, Aug 25, 2023 at 05:21:26PM +0200, Niklas Schnelle wrote:
>>>>> Add ops->flush_iotlb_all operation to enable virtio-iommu for the
>>>>> dma-iommu deferred flush scheme. This results inn a significant increase
>>>> in
>>>>
>>>>> in performance in exchange for a window in which devices can still
>>>>> access previously IOMMU mapped memory. To get back to the prior behavior
>>>>> iommu.strict=1 may be set on the kernel command line.
>>>> Maybe add that it depends on CONFIG_IOMMU_DEFAULT_DMA_{LAZY,STRICT} as
>>>> well, because I've seen kernel configs that enable either.
>>> Indeed, I'd be inclined phrase it in terms of the driver now actually 
>>> being able to honour lazy mode when requested (which happens to be the 
>>> default on x86), rather than as if it might be some 
>>> potentially-unexpected change in behaviour.
>>>
>>> Thanks,
>>> Robin.
>> I kept running this series on a KVM guest on my private workstation
>> (QEMU v8.0.4) and while running iperf3 on a passed-through Intel 82599
>> VF. I got a bunch of IOMMU events similar to the following as well as
>> card resets in the host.
>>
>> ..
>> [ 5959.338214] vfio-pci 0000:04:10.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0037 address=0x7b657064 flags=0x0000]
>> [ 5963.353429] ixgbe 0000:03:00.0 enp3s0: Detected Tx Unit Hang
>>                  Tx Queue             <0>
>>                  TDH, TDT             <93>, <9d>
>>                  next_to_use          <9d>
>>                  next_to_clean        <93>
>>                tx_buffer_info[next_to_clean]
>>                  time_stamp           <10019e800>
>>                  jiffies              <10019ec80>
>> ...
>>
>> I retested on v6.5 vanilla (guest & host) and still get the above
>> errors so luckily for me it doesn't seem to be caused by the new code
>> but I can't reproduce it without virtio-iommu. Any idea what could
>> cause this?
> Adding Eric in case this looks familiar.
Unfortunately no idea of what could cause those page faults. On ther
other hand I mostly test on ARM and INTEL.

Thanks

Eric
>
> I don't have hardware to test this but I guess QEMU system emulation may
> be able to reproduce the issue since it has an AMD IOMMU (unmaintained)
> and igb, I can give that a try.
>
> Thanks,
> Jean
>
>>>>> Link: https://lore.kernel.org/lkml/20230802123612.GA6142@myrica/
>>>>> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
>>>>> ---
>>>>>   drivers/iommu/virtio-iommu.c | 12 ++++++++++++
>>>>>   1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
>>>>> index fb73dec5b953..1b7526494490 100644
>>>>> --- a/drivers/iommu/virtio-iommu.c
>>>>> +++ b/drivers/iommu/virtio-iommu.c
>>>>> @@ -924,6 +924,15 @@ static int viommu_iotlb_sync_map(struct iommu_domain *domain,
>>>>>   	return viommu_sync_req(vdomain->viommu);
>>>>>   }
>>>>>   
>>>>> +static void viommu_flush_iotlb_all(struct iommu_domain *domain)
>>>>> +{
>>>>> +	struct viommu_domain *vdomain = to_viommu_domain(domain);
>>>>> +
>>>>> +	if (!vdomain->nr_endpoints)
>>>>> +		return;
>>>> As for patch 1, a NULL check in viommu_sync_req() would allow dropping
>>>> this one
>>>>
>>>> Thanks,
>>>> Jean
>> Right, makes sense will move the check into viommu_sync_req() and add a
>> coment that it is there fore the cases where viommu_iotlb_sync() et al
>> get called before the IOMMU is set up.
>>
>>>>> +	viommu_sync_req(vdomain->viommu);
>>>>> +}
>>>>> +
>>>>>   static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
>>>>>   {
>>>>>   	struct iommu_resv_region *entry, *new_entry, *msi = NULL;
>>>>> @@ -1049,6 +1058,8 @@ static bool viommu_capable(struct device *dev, enum iommu_cap cap)
>>>>>   	switch (cap) {
>>>>>   	case IOMMU_CAP_CACHE_COHERENCY:
>>>>>   		return true;
>>>>> +	case IOMMU_CAP_DEFERRED_FLUSH:
>>>>> +		return true;
>>>>>   	default:
>>>>>   		return false;
>>>>>   	}
>>>>> @@ -1069,6 +1080,7 @@ static struct iommu_ops viommu_ops = {
>>>>>   		.map_pages		= viommu_map_pages,
>>>>>   		.unmap_pages		= viommu_unmap_pages,
>>>>>   		.iova_to_phys		= viommu_iova_to_phys,
>>>>> +		.flush_iotlb_all	= viommu_flush_iotlb_all,
>>>>>   		.iotlb_sync		= viommu_iotlb_sync,
>>>>>   		.iotlb_sync_map		= viommu_iotlb_sync_map,
>>>>>   		.free			= viommu_domain_free,
>>>>>
>>>>> -- 
>>>>> 2.39.2
>>>>>