Re: [Xen-devel] [PATCH] x86: fix cache flushing condition in map_pages_to_xen()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.07.13 at 17:40, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> On 17/07/13 16:09, Jan Beulich wrote:
>> This fixes yet another shortcoming of the function (exposed by 8bfaa2c2
>> ["x86: add locking to map_pages_to_xen()"]'s adjustment to
>> msix_put_fixmap()): It must not flush caches when transitioning to a
>> non-present mapping. Doing so causes the CLFLUSH to fault, if used in
>> favor of WBINVD.
>>
>> To help code readability, factor out the whole flush flags updating
>> in map_pages_to_xen() into a helper macro.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -5430,6 +5430,15 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned l
>>                           flush_area_local((const void *)v, f) : \
>>                           flush_area_all((const void *)v, f))
>>  
>> +#define flush_flags(oldf) do {                                  \
>> +    unsigned int o_ = (oldf);                                   \
>> +    if ( (o_) & _PAGE_GLOBAL )                                  \
>> +        flush_flags |= FLUSH_TLB_GLOBAL;                        \
>> +    if ( (flags & _PAGE_PRESENT) &&                             \
>> +         (((o_) ^ flags) & PAGE_CACHE_ATTRS) )                  \
>> +        flush_flags |= FLUSH_CACHE;                             \
>> +} while (0)
>> +
> 
> I have to admit to being surprised that the compiler is even happy with
> a macro aliasing a variable, but please can it be renamed to something
> else (perhaps "set_flush_flags" ?) for the sanity of other humans trying
> to read the code.

Ugly. I actually picked the same name intentionally.

> Furthermore, are we not introducing consistency errors?
> 
> Previously, we occasionally decided to flush specific cache lines, and
> are now conditionally not flushing the cache lines depending on the
> mappings.
> 
> Should the fix not be "If we need to flush parts of the cache and dont
> have mappings to what we want to flush, use wbinvd()" ?

No. The cache flushing (a half hearted attempt only anyway) is
necessary only when changing cachability of a mapping (and in
fact the condition could be further relaxed to cover only the case
where a translation for the same physical address changes
cachability, but that would require not only comparing the
single MFNs from the old and new PTE, but the sets of all the
ones affected by the current function invocation). It could also
be relaxed to e.g. not get done when transitioning from UC or
to WB. But that's all pretty pointless.

As to the "half hearted" above: What we do currently isn't really
correct anyway: When transitioning a translation to a different
caching mode, we'd have to tear down the translation, flush TLB,
flush cache, and _only then_ establish the new translation. But
what we do right now seems to be good enough in practice.

Jan


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