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

[Bertrand Marquis <Bertrand.Marquis@arm.com>]

> On 18 Aug 2020, at 10:23, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 18/08/2020 10:05, Bertrand Marquis wrote:
>>> On 18 Aug 2020, at 09:58, Julien Grall <julien@xen.org> 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@amazon.com>
>>>>>>> 
>>>>>>> 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@amazon.com>
>>>>>> 
>>>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>>> 
>>>>>> 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>

Sorry, I meant here including directly the asm one in a C file (without the xen one before) if you test 
only “ifndef __XEN_GUEST_ACCESS_H__” in the asm one.

> 
> 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/*>.

But this only enforced by review (not by any magics with ifdefs), why not doing the same for directly inclusion of the asm one ?

> 
>> 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.

Then if we want to enforce it with an error during compilation, your solution is the only one working I agree.

Cheers
Bertrand