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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

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

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.

Cheers
Bertrand