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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

> > > > > > --- 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@xxxxxxxxxx/
> > 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)