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

[Keir Fraser <keir.xen@gmail.com>]

On 17/07/2013 17:01, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 17.07.13 at 17:40, Andrew Cooper <andrew.cooper3@citrix.com> 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@suse.com>
>>> 
>>> --- 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.

I'm not too strongly opinionated on this one, but it did make me look twice.
I think it would be cleaner something like:
#define flush_flags(oldf) ({ unsigned int f_ = 0; ...; _f; })
...
flush_flags |= flush_flags(...);
The 'name collision' I'm fine with, whereas going directly at a caller's
variable within a macro is rather grubby behaviour. ;)

Either way though I find the patch acceptable. And I will Ack it as already
posted.

 -- Keir

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