Re: [XEN PATCH] xen: cache clearing and invalidation helpers refactoring

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

>>>> 
>>>>>> --- a/xen/arch/arm/include/asm/page.h
>>>>>> +++ b/xen/arch/arm/include/asm/page.h
>>>>>> @@ -123,6 +123,7 @@
>>>>>> 
>>>>>>  #ifndef __ASSEMBLY__
>>>>>> 
>>>>>> +#include <public/grant_table.h>
>>>>> 
>>>>> This is a no-go, imo (also on x86): Adding this include here
>>>>> effectively
>>>>> means that nearly every CU will have a dependency on that header, 
>>>>> no
>>>>> matter that most are entirely agnostic of grants. Each arch has a
>>>>> grant_table.h - is there any reason the new, grant-specific helper
>>>>> can't
>>>>> be put there?
>>>>> 
>>>> 
>>>> I would have to test, but I think that can be done
>>>> 
>>> 
>>> The only blocker so far is that this triggers a build error due to a
>>> circular dependency between xen/mm.h and asm/flushtlb.h on x86. Also
>>> found some earlier evidence [1] that there are some oddities around
>>> asm/flushtlb's inclusion.
>>> 
>>> [1]
>>> https://lore.kernel.org/xen-devel/20200318210540.5602-1-andrew.cooper3@citrix.com/
>> 
>> There could be a way of untangling asm/flushtlb.h from xen/mm.h, by
>> moving "accumulate_tlbflush" and "filtered_flush_tlb_mask" introduced 
>> by
>> commit 80943aa40e30 ("replace tlbflush check and operation with inline
>> functions") [1].
>> However, these function should then be part of a generic 
>> xen/flushtlb.h
>> header, since they are used in common code (e.g., common/page_alloc) 
>> and
>> a bunch of common code source files should move their includes (see 
>> [2]
>> for a partial non-working patch). Do you feel that this is a feasible
>> route?
> 
> Yeah, introducing xen/flushtlb.h to hold these sounds pretty sensible.
> 

There is some shuffling of includes to be done to get it to build, which 
I'm still trying to address. Most problems are down to the use of struct 
page_info in page_set_tlbflush_timestamp in x86/.*/asm/flushtlb.h which 
then prompts the inclusion asm/mm.h probably.

>> In passing it should be noted that the header ordering in
>> x86/alternative.c is not the one usually prescribed, so that may be
>> taken care of as well.
> 
> I'm afraid I don't understand this remark.
> 

I just meant to say that this

#include <xen/delay.h>
#include <xen/types.h>
#include <asm/apic.h>
#include <asm/endbr.h>
#include <asm/processor.h>
#include <asm/alternative.h>
#include <xen/init.h>
#include <asm/setup.h>
#include <asm/system.h>
#include <asm/traps.h>
#include <asm/nmi.h>
#include <asm/nops.h>
#include <xen/livepatch.h>

is not the usual order of xen/*.h then asm/*.h and there is no comment 
justifying that ordering. So in the process of including asm/flushtlb.h 
here the inclusion order can be tidied up (or also indipendently), 
unless there is some reason I'm missing that disallows it.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)