Re: [PATCH for-4.14 v3] x86/tlb: fix assisted flush usage

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.06.2020 12:07, Roger Pau Monné wrote:
> On Fri, Jun 26, 2020 at 10:38:11AM +0100, Julien Grall wrote:
>> Hi Roger,
>>
>> Sorry I didn't manage to answer to your question before you sent v3.
>>
>> On 25/06/2020 12:30, Roger Pau Monne wrote:
>>> diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h
>>> index ab1aae5c90..7ae0543885 100644
>>> --- a/xen/include/asm-arm/flushtlb.h
>>> +++ b/xen/include/asm-arm/flushtlb.h
>>> @@ -27,6 +27,7 @@ static inline void page_set_tlbflush_timestamp(struct 
>>> page_info *page)
>>>   /* Flush specified CPUs' TLBs */
>>>   void flush_tlb_mask(const cpumask_t *mask);
>>> +#define flush_tlb_mask_sync flush_tlb_mask
>>
>> Dropping the parameter 'sync' from filtered_flush_tlb_mask() is a nice
>> improvement, but it unfortunately doesn't fully address my concern.
>>
>> After this patch there is exactly one use of flush_tlb_mask() in common code
>> (see grant_table.c). But without looking at the x86 code, it is not clear
>> why this requires a different flush compare to the two other sites.
> 
> It's not dealing with page allocation or page type changes directly,
> and hence doesn't need to use an IPI in order to prevent races with
> spurious_page_fault.
> 
>> IOW, if I want to modify the common code in the future, how do I know which
>> flush to call?
> 
> Unless you modify one of the specific areas mentioned above (page
> allocation or page type changes) you should use flush_tlb_mask.
> 
> This is not ideal, and my aim will be to be able to use the assisted
> flush everywhere if possible, so I would really like to get rid of the
> interrupt disabling done in spurious_page_fault and this model where
> x86 relies on blocking interrupts in order to prevent page type
> changes or page freeing.
> 
> Such change however doesn't feel appropriate for a release freeze
> period, and hence went with something smaller that restores the
> previous behavior. Another option is to just disable assisted flushes
> for the time being and re-enable them when a suitable solution is
> found.

As I can understand Julien's concern, maybe this would indeed be
the better approach for now? Andrew, Paul - thoughts?

Jan