Re: [RESEND][PATCH v2 5/7] xen: include xen/guest_access.h rather than asm/guest_access.h

[Julien Grall <julien@xxxxxxx>]

On 18/08/2020 10:05, Bertrand Marquis wrote:
> > On 18 Aug 2020, at 09:58, Julien Grall <julien@xxxxxxx> wrote:
> >
> >
> >
> > On 18/08/2020 09:50, Jan Beulich wrote:
> > > On 14.08.2020 21:07, Julien Grall wrote:
> > > > Hi Jan,
> > > >
> > > > On 31/07/2020 12:36, Jan Beulich wrote:
> > > > > On 30.07.2020 20:18, Julien Grall wrote:
> > > > > > From: Julien Grall <jgrall@xxxxxxxxxx>
> > > > > >
> > > > > > Only a few places are actually including asm/guest_access.h. While this
> > > > > > is fine today, a follow-up patch will want to move most of the helpers
> > > > > > from asm/guest_access.h to xen/guest_access.h.
> > > > > >
> > > > > > To prepare the move, everyone should include xen/guest_access.h rather
> > > > > > than asm/guest_access.h.
> > > > > >
> > > > > > Interestingly, asm-arm/guest_access.h includes xen/guest_access.h. The
> > > > > > inclusion is now removed as no-one but the latter should include the
> > > > > > former.
> > > > > >
> > > > > > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> > > > > Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > >
> > > > > Is there any chance you could take measures to avoid new inclusions
> > > > > of asm/guest_access.h to appear?
> > > > It should be possible.
> > > >
> > > > How about this:
> > > >
> > > > diff --git a/xen/include/asm-arm/guest_access.h 
> > > > b/xen/include/asm-arm/guest_access.h
> > > > index b9a89c495527..d8dbc7c973b4 100644
> > > > --- a/xen/include/asm-arm/guest_access.h
> > > > +++ b/xen/include/asm-arm/guest_access.h
> > > > @@ -1,3 +1,7 @@
> > > > +#ifndef ALLOW_INCLUDE_ASM_GUEST_ACCESS_H
> > > > +#error "asm/guest_access.h should not be included directly"
> > > > +#endif
> > > > +
> > > >   #ifndef __ASM_ARM_GUEST_ACCESS_H__
> > > >   #define __ASM_ARM_GUEST_ACCESS_H__
> > > >
> > > > diff --git a/xen/include/asm-x86/guest_access.h 
> > > > b/xen/include/asm-x86/guest_access.h
> > > > index 369676f31ac3..e665ca3a27af 100644
> > > > --- a/xen/include/asm-x86/guest_access.h
> > > > +++ b/xen/include/asm-x86/guest_access.h
> > > > @@ -4,6 +4,10 @@
> > > >    * Copyright (c) 2006, K A Fraser
> > > >    */
> > > >
> > > > +#ifndef ALLOW_INCLUDE_ASM_GUEST_ACCESS_H
> > > > +#error "asm/guest_access.h should not be included directly"
> > > > +#endif
> > > > +
> > > >   #ifndef __ASM_X86_GUEST_ACCESS_H__
> > > >   #define __ASM_X86_GUEST_ACCESS_H__
> > > >
> > > > diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
> > > > index 75103d30c8be..814e31329de9 100644
> > > > --- a/xen/include/xen/guest_access.h
> > > > +++ b/xen/include/xen/guest_access.h
> > > > @@ -7,7 +7,9 @@
> > > >   #ifndef __XEN_GUEST_ACCESS_H__
> > > >   #define __XEN_GUEST_ACCESS_H__
> > > >
> > > > +#define ALLOW_INCLUDE_ASM_GUEST_ACCESS_H
> > > >   #include <asm/guest_access.h>
> > > > +#undef ALLOW_INCLUDE_ASM_GUEST_ACCESS_H
> > > >   #include <xen/types.h>
> > > >   #include <public/xen.h>
> > > One option. Personally I'd prefer to avoid introduction of yet another
> > > constant, by leveraging __XEN_GUEST_ACCESS_H__ instead.
> > I thought about it but it doesn't prevent new inclusions of asm/guest_access.h. 
> > For instance, the following would still compile:
> >
> > #include <xen/guest_access.h>
> >
> > [...]
> >
> > #include <asm/guest_access.h>
> >
> > If we want to completely prevent new inclusion, then we need a new temporary 
> > constant.
> I would think that this would not handle all cases but would at least prevent 
> someone from including directly the asm header.
It is not entirely clear what you mean by "including directly" given 
that my example above a C file would include <asm/guest_access.h>
To make it more obvious <xen/guest_access.h> may have been included via 
another header.
The solution suggested by Jan would only prevent the following case:

#include <asm/guest_access.h>

[...]

#include <xen/guest_access.h>

But this should never happen given that we request <xen/*> to be before 
<asm/*>.
> The solution with the define and undef does not look really nice and headers 
> could become really ugly if we start doing that for
> all asm headers that should not be included directly.
It is not but that's the price to pay if we want to enforce the rule.

Cheers,

--
Julien Grall