[PATCH] iommu/vt-d: Remove unnecassary qi clflushes

[PATCH] iommu/vt-d: Remove unnecassary qi clflushes

[Nadav Amit]

According to the manual: "Hardware access to ...  invalidation queue ...
are always coherent."

Remove unnecassary clflushes accordingly.

Signed-off-by: Nadav Amit <namit-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>

---

Build-tested since I do not have an IOMMU that does not support
coherency.
---
 drivers/iommu/dmar.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 6a86b5d..ca56ec3 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1155,8 +1155,6 @@ static int qi_check_fault(struct intel_iommu *iommu, int index)
 				(unsigned long long)qi->desc[index].high);
 			memcpy(&qi->desc[index], &qi->desc[wait_index],
 					sizeof(struct qi_desc));
-			__iommu_flush_cache(iommu, &qi->desc[index],
-					sizeof(struct qi_desc));
 			writel(DMA_FSTS_IQE, iommu->reg + DMAR_FSTS_REG);
 			return -EINVAL;
 		}
@@ -1231,9 +1229,6 @@ restart:
 
 	hw[wait_index] = wait_desc;
 
-	__iommu_flush_cache(iommu, &hw[index], sizeof(struct qi_desc));
-	__iommu_flush_cache(iommu, &hw[wait_index], sizeof(struct qi_desc));
-
 	qi->free_head = (qi->free_head + 2) % QI_LENGTH;
 	qi->free_cnt -= 2;
 
-- 
2.7.4

Re: [PATCH] iommu/vt-d: Remove unnecassary qi clflushes

[Joerg Roedel]

On Fri, Jun 24, 2016 at 06:13:14AM -0700, Nadav Amit wrote:
> According to the manual: "Hardware access to ...  invalidation queue ...
> are always coherent."
> 
> Remove unnecassary clflushes accordingly.

It is one thing what the spec says and another how hardware really
behaves. Have you tested this on (potentially really old) VT-d machines
to make sure the spec is _always_ right here?


	Joerg

Re: [PATCH] iommu/vt-d: Remove unnecassary qi clflushes

[Nadav Amit]

Joerg Roedel <joro@8bytes.org> wrote:

> On Fri, Jun 24, 2016 at 06:13:14AM -0700, Nadav Amit wrote:
>> According to the manual: "Hardware access to ...  invalidation queue ...
>> are always coherent."
>> 
>> Remove unnecassary clflushes accordingly.
> 
> It is one thing what the spec says and another how hardware really
> behaves. Have you tested this on (potentially really old) VT-d machines
> to make sure the spec is _always_ right here?

No I didn’t as the commit message says. I would be happy for someones’
tested-by.

Having said that - FreeBSD does not do these (unnecessary)
invalidations [1], and their code comment clearly says it. Since this code
is not new, I would assume FreeBSD would crash by now if the code was
buggy.

Although such hardware is old, there are some hypervisors that do not set
the ecap.coherency of emulated IOMMUs. Yes, it is unwise, but there is no
reason to further punish these hypervisors.

Thanks,
Nadav

[1] http://web.mit.edu/freebsd/head/sys/x86/iommu/intel_qi.c
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/vt-d: Remove unnecassary qi clflushes

[Nadav Amit]

Nadav Amit <nadav.amit@gmail.com> wrote:

> Joerg Roedel <joro@8bytes.org> wrote:
> 
>> On Fri, Jun 24, 2016 at 06:13:14AM -0700, Nadav Amit wrote:
>>> According to the manual: "Hardware access to ...  invalidation queue ...
>>> are always coherent."
>>> 
>>> Remove unnecassary clflushes accordingly.
>> 
>> It is one thing what the spec says and another how hardware really
>> behaves. Have you tested this on (potentially really old) VT-d machines
>> to make sure the spec is _always_ right here?
> 
> No I didn’t as the commit message says. I would be happy for someones’
> tested-by.
> 
> Having said that - FreeBSD does not do these (unnecessary)
> invalidations [1], and their code comment clearly says it. Since this code
> is not new, I would assume FreeBSD would crash by now if the code was
> buggy.
> 
> Although such hardware is old, there are some hypervisors that do not set
> the ecap.coherency of emulated IOMMUs. Yes, it is unwise, but there is no
> reason to further punish these hypervisors.

Ping? To clarify - this behavior applies to the guest of VMware and KVM
(which uses QEMU for IOMMU emulation).

Regardless, please respond to the other patch I sent as well.

Thanks,
Nadav

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/vt-d: Remove unnecassary qi clflushes

[Paolo Bonzini]

On 05/07/2016 18:27, Nadav Amit wrote:
>> Although such hardware is old, there are some hypervisors that do not set
>> the ecap.coherency of emulated IOMMUs. Yes, it is unwise, but there is no
>> reason to further punish these hypervisors.

QEMU will need the kernel to respect ecap.coherency in order to support
nested VFIO, for example.

Paolo

Re: [PATCH] iommu/vt-d: Remove unnecassary qi clflushes

[Nadav Amit]

Paolo Bonzini <pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> 
> 
> On 05/07/2016 18:27, Nadav Amit wrote:
>>> Although such hardware is old, there are some hypervisors that do not set
>>> the ecap.coherency of emulated IOMMUs. Yes, it is unwise, but there is no
>>> reason to further punish these hypervisors.
> 
> QEMU will need the kernel to respect ecap.coherency in order to support
> nested VFIO, for example.

To clarify - the kernel respects the coherency, but performs more clflushes
than necessary. It has no functional impact, but induces performance
degradation (which I  did not measure, but is likely to be several hundreds
of cycles per flush).

Regards,
Nadav

Re: [PATCH] iommu/vt-d: Remove unnecassary qi clflushes

[Paolo Bonzini]

On 05/07/2016 18:50, Nadav Amit wrote:
> Paolo Bonzini <pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
>>
>>
>> On 05/07/2016 18:27, Nadav Amit wrote:
>>>> Although such hardware is old, there are some hypervisors that do not set
>>>> the ecap.coherency of emulated IOMMUs. Yes, it is unwise, but there is no
>>>> reason to further punish these hypervisors.
>>
>> QEMU will need the kernel to respect ecap.coherency in order to support
>> nested VFIO, for example.
> 
> To clarify - the kernel respects the coherency, but performs more clflushes
> than necessary. It has no functional impact, but induces performance
> degradation (which I  did not measure, but is likely to be several hundreds
> of cycles per flush).

Oh, then QEMU indeed doesn't care about flushing the invalidation queue.

Paolo

Re: [PATCH] iommu/vt-d: Remove unnecassary qi clflushes

[Joerg Roedel]

On Fri, Jun 24, 2016 at 06:13:14AM -0700, Nadav Amit wrote:
> According to the manual: "Hardware access to ...  invalidation queue ...
> are always coherent."
> 
> Remove unnecassary clflushes accordingly.
> 
> Signed-off-by: Nadav Amit <namit-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>

Applied, thanks.