Re: [Xen-devel] [PATCHv1] CA-201372: x86: don't flush the whole cache when changing cachability

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.03.16 at 15:36, <david.vrabel@xxxxxxxxxx> wrote:
> On 09/03/16 14:01, Jan Beulich wrote:
>>>>> On 08.03.16 at 19:44, <david.vrabel@xxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/flushtlb.c
>>> +++ b/xen/arch/x86/flushtlb.c
>>> @@ -140,7 +140,8 @@ unsigned int flush_area_local(const void *va, unsigned 
> int flags)
>>>          if ( order < (BITS_PER_LONG - PAGE_SHIFT) )
>>>              sz = 1UL << (order + PAGE_SHIFT);
>>>  
>>> -        if ( !(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) &&
>>> +        if ( (!(flags & (FLUSH_TLB|FLUSH_TLB_GLOBAL)) ||
>>> +              flags & FLUSH_VA_VALID) &&
>> 
>> The & wants to be parenthesized.
> 
> Style nit-picks really should come with a patch to CODING_STYLE.  The
> existing code isn't consistent enough to deduce the preferred style.

Well, I would have silently added the parentheses upon commit
if there wasn't that other issue below. (Besides that I don't think
you'll find that many unparenthesized & or | which are themselves
operands to another operator. Others, e.g. Andrew, are even
more strict than me in requiring parentheses around all binary
operations that are themselves operands; I personally think that
some precedence rules can be assumed to be commonly known,
but the relationship of & and | vs && and || is clearly not among
those.)

>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -5641,7 +5641,7 @@ int map_pages_to_xen(
>>>          flush_flags |= FLUSH_TLB_GLOBAL;       \
>>>      if ( (flags & _PAGE_PRESENT) &&            \
>>>           (((o_) ^ flags) & PAGE_CACHE_ATTRS) ) \
>>> -        flush_flags |= FLUSH_CACHE;            \
>>> +        flush_flags |= FLUSH_CACHE_BY_VA;      \
>>>  } while (0)
>> 
>> No, that's too simple. You may flush by VA only if the MFN didn't
>> change (or else you flush the wrong page).
> 
> Cachability changes goes through update_xen_mappings() which always uses
> the same MFN -> virt mapping so the MFN never changes.

But the code you change affects map_pages_to_xen(), of which
only one caller is update_xen_mappings(). It would be calling for
hard to debug bugs if we baked in an assumption that only the
latter can do cachability adjustments.

Jan


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