[PATCH] x86: don't use VA for cache flush when also flushing TLB

[PATCH] x86: don't use VA for cache flush when also flushing TLB

[Jan Beulich]

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

Doing both flushes at once is a strong indication for the address
mapping to either having got dropped (in which case the cache flush,
when done via INVLPG, would fault) or its physical address having
changed (in which case the cache flush would end up being done on the
wrong address range). There is no adverse effect (other than the
obvious performance one) using WBINVD in this case regardless of the
range's size; only map_pages_to_xen() uses combined flushes at present.

This problem was observed with the 2nd try backport of d6cb14b3 ("VT-d:
suppress UR signaling for desktop chipsets") to 4.2 (where ioremap()
needs to be replaced with set_fixmap_nocache(); the now commented out
__set_fixmap(, 0, 0) there to undo the mapping resulted in the first of
the above two scenarios).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -152,7 +152,8 @@ void flush_area_local(const void *va, un
         if ( order < (BITS_PER_LONG - PAGE_SHIFT) )
             sz = 1UL << (order + PAGE_SHIFT);
 
-        if ( c->x86_clflush_size && c->x86_cache_size && sz &&
+        if ( !(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) &&
+             c->x86_clflush_size && c->x86_cache_size && sz &&
              ((sz >> 10) < c->x86_cache_size) )
         {
             va = (const void *)((unsigned long)va & ~(sz - 1));




[-- Attachment #2: x86-flush-mixed.patch --]
[-- Type: text/plain, Size: 1457 bytes --]

x86: don't use VA for cache flush when also flushing TLB

Doing both flushes at once is a strong indication for the address
mapping to either having got dropped (in which case the cache flush,
when done via INVLPG, would fault) or its physical address having
changed (in which case the cache flush would end up being done on the
wrong address range). There is no adverse effect (other than the
obvious performance one) using WBINVD in this case regardless of the
range's size; only map_pages_to_xen() uses combined flushes at present.

This problem was observed with the 2nd try backport of d6cb14b3 ("VT-d:
suppress UR signaling for desktop chipsets") to 4.2 (where ioremap()
needs to be replaced with set_fixmap_nocache(); the now commented out
__set_fixmap(, 0, 0) there to undo the mapping resulted in the first of
the above two scenarios).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -152,7 +152,8 @@ void flush_area_local(const void *va, un
         if ( order < (BITS_PER_LONG - PAGE_SHIFT) )
             sz = 1UL << (order + PAGE_SHIFT);
 
-        if ( c->x86_clflush_size && c->x86_cache_size && sz &&
+        if ( !(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) &&
+             c->x86_clflush_size && c->x86_cache_size && sz &&
              ((sz >> 10) < c->x86_cache_size) )
         {
             va = (const void *)((unsigned long)va & ~(sz - 1));

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86: don't use VA for cache flush when also flushing TLB

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2598 bytes --]

On 26/05/2014 11:17, Jan Beulich wrote:
> Doing both flushes at once is a strong indication for the address
> mapping to either having got dropped (in which case the cache flush,
> when done via INVLPG, would fault) or its physical address having
> changed (in which case the cache flush would end up being done on the
> wrong address range). There is no adverse effect (other than the
> obvious performance one) using WBINVD in this case regardless of the
> range's size; only map_pages_to_xen() uses combined flushes at present.
>
> This problem was observed with the 2nd try backport of d6cb14b3 ("VT-d:
> suppress UR signaling for desktop chipsets") to 4.2 (where ioremap()
> needs to be replaced with set_fixmap_nocache(); the now commented out
> __set_fixmap(, 0, 0) there to undo the mapping resulted in the first of
> the above two scenarios).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Overall, I agree with your final assessment, and it might indeed be the
easiest way, so on that basis, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

It would be nice to avoid the performance hit if possible.

The fault case can be fixed by reording the cache flush with respect to
the tlb flush inside flush_area_local.  For callers performing a TLB
shootdown and a cache flush, it is far more likely that the VAs wanting
flushing are under the old TLB mappings rather than the new.

This does come with a risk that the old TLB entry has already fallen out
of the TLB before the flush happens, at which point we are back into the
risk of a fault or flushing the wrong physical area.

The only way I can see to safely flush bot the caches and TLB for a
given change is to flush the cache by VA, then update the PTE, then
shoot down the TLB entry.  Despite this being awkward, I think it is
preferable to falling back to wbinvd() to cover up bad behaviour in the
callers.

~Andrew

>
> --- a/xen/arch/x86/flushtlb.c
> +++ b/xen/arch/x86/flushtlb.c
> @@ -152,7 +152,8 @@ void flush_area_local(const void *va, un
>          if ( order < (BITS_PER_LONG - PAGE_SHIFT) )
>              sz = 1UL << (order + PAGE_SHIFT);
>  
> -        if ( c->x86_clflush_size && c->x86_cache_size && sz &&
> +        if ( !(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) &&
> +             c->x86_clflush_size && c->x86_cache_size && sz &&
>               ((sz >> 10) < c->x86_cache_size) )
>          {
>              va = (const void *)((unsigned long)va & ~(sz - 1));
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 3698 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86: don't use VA for cache flush when also flushing TLB

[Jan Beulich]

>>> On 26.05.14 at 18:37, <andrew.cooper3@citrix.com> wrote:
> On 26/05/2014 11:17, Jan Beulich wrote:
>> Doing both flushes at once is a strong indication for the address
>> mapping to either having got dropped (in which case the cache flush,
>> when done via INVLPG, would fault) or its physical address having
>> changed (in which case the cache flush would end up being done on the
>> wrong address range). There is no adverse effect (other than the
>> obvious performance one) using WBINVD in this case regardless of the
>> range's size; only map_pages_to_xen() uses combined flushes at present.
>>
>> This problem was observed with the 2nd try backport of d6cb14b3 ("VT-d:
>> suppress UR signaling for desktop chipsets") to 4.2 (where ioremap()
>> needs to be replaced with set_fixmap_nocache(); the now commented out
>> __set_fixmap(, 0, 0) there to undo the mapping resulted in the first of
>> the above two scenarios).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Overall, I agree with your final assessment, and it might indeed be the
> easiest way, so on that basis, Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>
> 
> It would be nice to avoid the performance hit if possible.

While I agree with this in general, the only case where this actually
could happen right now is tearing down an uncachable fixmap
entry. I.e. the performance hit is purely theoretical at this point
(and we need this fix in practice only on the 4.2 branch;
everywhere else it'll just fix a latent bug).

> The fault case can be fixed by reording the cache flush with respect to
> the tlb flush inside flush_area_local.  For callers performing a TLB
> shootdown and a cache flush, it is far more likely that the VAs wanting
> flushing are under the old TLB mappings rather than the new.
> 
> This does come with a risk that the old TLB entry has already fallen out
> of the TLB before the flush happens, at which point we are back into the
> risk of a fault or flushing the wrong physical area.

I.e. not an option.

> The only way I can see to safely flush bot the caches and TLB for a
> given change is to flush the cache by VA, then update the PTE, then
> shoot down the TLB entry.  Despite this being awkward, I think it is
> preferable to falling back to wbinvd() to cover up bad behaviour in the
> callers.

Did you spend a few minutes try to find a way to do this half way
cleanly in map_pages_to_xen()? I did, and I _much_ prefer this simple
patch over a rather intrusive one to that function. Of course all only
as long as this isn't going to happen more frequently.

Jan

Re: [PATCH] x86: don't use VA for cache flush when also flushing TLB

[Zhang, Yang Z]

Jan Beulich wrote on 2014-05-27:
>>>> On 26.05.14 at 18:37, <andrew.cooper3@citrix.com> wrote:
>> On 26/05/2014 11:17, Jan Beulich wrote:
>>> Doing both flushes at once is a strong indication for the address
>>> mapping to either having got dropped (in which case the cache
>>> flush, when done via INVLPG, would fault) or its physical address
>>> having changed (in which case the cache flush would end up being
>>> done on the wrong address range). There is no adverse effect (other
>>> than the obvious performance one) using WBINVD in this case
>>> regardless of the range's size; only map_pages_to_xen() uses combined flushes at present.
>>> 
>>> This problem was observed with the 2nd try backport of d6cb14b3 ("VT-d:
>>> suppress UR signaling for desktop chipsets") to 4.2 (where
>>> ioremap() needs to be replaced with set_fixmap_nocache(); the now
>>> commented out __set_fixmap(, 0, 0) there to undo the mapping
>>> resulted in the first of the above two scenarios).
>>> 
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> Overall, I agree with your final assessment, and it might indeed be
>> the easiest way, so on that basis, Reviewed-by: Andrew Cooper
>> <andrew.cooper3@citrix.com>
>> 
>> It would be nice to avoid the performance hit if possible.
> 
> While I agree with this in general, the only case where this actually
> could happen right now is tearing down an uncachable fixmap entry.
> I.e. the performance hit is purely theoretical at this point (and we
> need this fix in practice only on the 4.2 branch; everywhere else it'll just fix a latent bug).

It happens only when (PAT||PCD||PWT) is modified. And those cases are not happened frequently. So it will not hit performance a lot. 

Besides, I am thinking should we flush cache unconditionally, e.g., if all agents that able to access memory have the snooping capability, then software is not required to flush cache always.(If I am wrong, please correct me and ignore the following changes). So is there any problem with this change?
@@ -145,7 +145,7 @@ void flush_area_local(const void *va, unsigned int flags)
         }
     }
 
-    if ( flags & FLUSH_CACHE )
+    if ( flags & FLUSH_CACHE && !(iommu_enable && iommu_snoop))
     {
         unsigned long i, sz = 0;



Best regards,
Yang

Re: [PATCH] x86: don't use VA for cache flush when also flushing TLB

[Jan Beulich]

>>> On 28.05.14 at 06:35, <yang.z.zhang@intel.com> wrote:
> Besides, I am thinking should we flush cache unconditionally, e.g., if all 
> agents that able to access memory have the snooping capability, then software 
> is not required to flush cache always.(If I am wrong, please correct me and 
> ignore the following changes). So is there any problem with this change?
> @@ -145,7 +145,7 @@ void flush_area_local(const void *va, unsigned int flags)
>          }
>      }
>  
> -    if ( flags & FLUSH_CACHE )
> +    if ( flags & FLUSH_CACHE && !(iommu_enable && iommu_snoop))
>      {
>          unsigned long i, sz = 0;

At least the iommu_snoop part was discussed at length a couple
of weeks back: I don't see how this relates to cache flushing
requirements. I also don't see how IOMMU presence/absence
would matter here - the behavior should clearly be identical
between systems with and without IOMMU.

Jan

Re: [PATCH] x86: don't use VA for cache flush when also flushing TLB

[Zhang, Yang Z]

Jan Beulich wrote on 2014-05-28:
>>>> On 28.05.14 at 06:35, <yang.z.zhang@intel.com> wrote:
>> Besides, I am thinking should we flush cache unconditionally, e.g., if
>> all agents that able to access memory have the snooping capability,
>> then software is not required to flush cache always.(If I am wrong,
>> please correct me and ignore the following changes). So is there any
>> problem with this change? @@ -145,7 +145,7 @@ void
>> flush_area_local(const void *va, unsigned int
> flags)
>>          }
>>      }
>> -    if ( flags & FLUSH_CACHE )
>> +    if ( flags & FLUSH_CACHE && !(iommu_enable && iommu_snoop))
>>      {
>>          unsigned long i, sz = 0;
> 
> At least the iommu_snoop part was discussed at length a couple of weeks back:
> I don't see how this relates to cache flushing requirements. I also
> don't see how IOMMU presence/absence would matter here - the behavior
> should clearly be identical between systems with and without IOMMU.

Ok. I remember there is a discussion around this before. I will check it.

> 
> Jan


Best regards,
Yang