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

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Jan Beulich wrote on 2014-05-27:
>>>> On 26.05.14 at 18:37, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxx>
>> 
>> 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@xxxxxxxxxx>
>> 
>> 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



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