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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

> 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


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