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

Commit Message

Julien Grall July 30, 2020, 6:18 p.m. UTC

From: Julien Grall <[e-mail]>

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 <[e-mail]>

---
    Changes in v2:
        - Remove some changes that weren't meant to be here.
---
 xen/arch/arm/decode.c                | 2 +-
 xen/arch/arm/domain.c                | 2 +-
 xen/arch/arm/guest_walk.c            | 3 ++-
 xen/arch/arm/guestcopy.c             | 2 +-
 xen/arch/arm/vgic-v3-its.c           | 2 +-
 xen/arch/x86/hvm/svm/svm.c           | 2 +-
 xen/arch/x86/hvm/viridian/viridian.c | 2 +-
 xen/arch/x86/hvm/vmx/vmx.c           | 2 +-
 xen/common/libelf/libelf-loader.c    | 2 +-
 xen/include/asm-arm/guest_access.h   | 1 -
 xen/lib/x86/private.h                | 2 +-
 11 files changed, 11 insertions(+), 11 deletions(-)

Comments

Stefano Stabellini July 30, 2020, 7:37 p.m. UTC | #1

On Thu, 30 Jul 2020, Julien Grall wrote:
> From: Julien Grall <[e-mail]>
> 
> 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 <[e-mail]>

Acked-by: Stefano Stabellini <[e-mail]>


> ---
>     Changes in v2:
>         - Remove some changes that weren't meant to be here.
> ---
>  xen/arch/arm/decode.c                | 2 +-
>  xen/arch/arm/domain.c                | 2 +-
>  xen/arch/arm/guest_walk.c            | 3 ++-
>  xen/arch/arm/guestcopy.c             | 2 +-
>  xen/arch/arm/vgic-v3-its.c           | 2 +-
>  xen/arch/x86/hvm/svm/svm.c           | 2 +-
>  xen/arch/x86/hvm/viridian/viridian.c | 2 +-
>  xen/arch/x86/hvm/vmx/vmx.c           | 2 +-
>  xen/common/libelf/libelf-loader.c    | 2 +-
>  xen/include/asm-arm/guest_access.h   | 1 -
>  xen/lib/x86/private.h                | 2 +-
>  11 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> index 144793c8cea0..792c2e92a7eb 100644
> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -17,12 +17,12 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <xen/guest_access.h>
>  #include <xen/lib.h>
>  #include <xen/sched.h>
>  #include <xen/types.h>
>  
>  #include <asm/current.h>
> -#include <asm/guest_access.h>
>  
>  #include "decode.h"
>  
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 31169326b2e3..9258f6d3faa2 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -12,6 +12,7 @@
>  #include <xen/bitops.h>
>  #include <xen/errno.h>
>  #include <xen/grant_table.h>
> +#include <xen/guest_access.h>
>  #include <xen/hypercall.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
> @@ -26,7 +27,6 @@
>  #include <asm/current.h>
>  #include <asm/event.h>
>  #include <asm/gic.h>
> -#include <asm/guest_access.h>
>  #include <asm/guest_atomics.h>
>  #include <asm/irq.h>
>  #include <asm/p2m.h>
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index a1cdd7f4afea..b4496c4c86c6 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -16,8 +16,9 @@
>   */
>  
>  #include <xen/domain_page.h>
> +#include <xen/guest_access.h>
>  #include <xen/sched.h>
> -#include <asm/guest_access.h>
> +
>  #include <asm/guest_walk.h>
>  #include <asm/short-desc.h>
>  
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index c8023e2bca5d..32681606d8fc 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -1,10 +1,10 @@
>  #include <xen/domain_page.h>
> +#include <xen/guest_access.h>
>  #include <xen/lib.h>
>  #include <xen/mm.h>
>  #include <xen/sched.h>
>  
>  #include <asm/current.h>
> -#include <asm/guest_access.h>
>  
>  #define COPY_flush_dcache   (1U << 0)
>  #define COPY_from_guest     (0U << 1)
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 6e153c698d56..58d939b85f92 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -32,6 +32,7 @@
>  #include <xen/bitops.h>
>  #include <xen/config.h>
>  #include <xen/domain_page.h>
> +#include <xen/guest_access.h>
>  #include <xen/lib.h>
>  #include <xen/init.h>
>  #include <xen/softirq.h>
> @@ -39,7 +40,6 @@
>  #include <xen/sched.h>
>  #include <xen/sizes.h>
>  #include <asm/current.h>
> -#include <asm/guest_access.h>
>  #include <asm/mmio.h>
>  #include <asm/gic_v3_defs.h>
>  #include <asm/gic_v3_its.h>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index ca3bbfcbb355..7301f3cd6004 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -16,6 +16,7 @@
>   * this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <xen/guest_access.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  #include <xen/trace.h>
> @@ -34,7 +35,6 @@
>  #include <asm/cpufeature.h>
>  #include <asm/processor.h>
>  #include <asm/amd.h>
> -#include <asm/guest_access.h>
>  #include <asm/debugreg.h>
>  #include <asm/msr.h>
>  #include <asm/i387.h>
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> index 977c1bc54fad..dc7183a54627 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -5,12 +5,12 @@
>   * Hypervisor Top Level Functional Specification for more information.
>   */
>  
> +#include <xen/guest_access.h>
>  #include <xen/sched.h>
>  #include <xen/version.h>
>  #include <xen/hypercall.h>
>  #include <xen/domain_page.h>
>  #include <xen/param.h>
> -#include <asm/guest_access.h>
>  #include <asm/guest/hyperv-tlfs.h>
>  #include <asm/paging.h>
>  #include <asm/p2m.h>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index eb54aadfbafb..cb5df1e81c9c 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -15,6 +15,7 @@
>   * this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <xen/guest_access.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  #include <xen/param.h>
> @@ -31,7 +32,6 @@
>  #include <asm/regs.h>
>  #include <asm/cpufeature.h>
>  #include <asm/processor.h>
> -#include <asm/guest_access.h>
>  #include <asm/debugreg.h>
>  #include <asm/msr.h>
>  #include <asm/p2m.h>
> diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
> index 0f468727d04a..629cc0d3e611 100644
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -16,7 +16,7 @@
>   */
>  
>  #ifdef __XEN__
> -#include <asm/guest_access.h>
> +#include <xen/guest_access.h>
>  #endif
>  
>  #include "libelf-private.h"
> diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
> index 31b9f03f0015..b9a89c495527 100644
> --- a/xen/include/asm-arm/guest_access.h
> +++ b/xen/include/asm-arm/guest_access.h
> @@ -1,7 +1,6 @@
>  #ifndef __ASM_ARM_GUEST_ACCESS_H__
>  #define __ASM_ARM_GUEST_ACCESS_H__
>  
> -#include <xen/guest_access.h>
>  #include <xen/errno.h>
>  #include <xen/sched.h>
>  
> diff --git a/xen/lib/x86/private.h b/xen/lib/x86/private.h
> index b793181464f3..2d53bd3ced23 100644
> --- a/xen/lib/x86/private.h
> +++ b/xen/lib/x86/private.h
> @@ -4,12 +4,12 @@
>  #ifdef __XEN__
>  
>  #include <xen/bitops.h>
> +#include <xen/guest_access.h>
>  #include <xen/kernel.h>
>  #include <xen/lib.h>
>  #include <xen/nospec.h>
>  #include <xen/types.h>
>  
> -#include <asm/guest_access.h>
>  #include <asm/msr-index.h>
>  
>  #define copy_to_buffer_offset copy_to_guest_offset
> -- 
> 2.17.1
>

Jan Beulich July 31, 2020, 11:36 a.m. UTC | #2

On 30.07.2020 20:18, Julien Grall wrote:
> From: Julien Grall <[e-mail]>
> 
> 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 <[e-mail]>

Acked-by: Jan Beulich <[e-mail]>

Is there any chance you could take measures to avoid new inclusions
of asm/guest_access.h to appear?

Jan

Bertrand Marquis July 31, 2020, 12:45 p.m. UTC | #3

> On 30 Jul 2020, at 20:18, Julien Grall <[e-mail]> wrote:
>
> From: Julien Grall <[e-mail]>
>
> 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 <[e-mail]>
Reviewed-by: Bertrand Marquis <[e-mail]>

>
> ---
>    Changes in v2:
>        - Remove some changes that weren't meant to be here.
> ---
> xen/arch/arm/decode.c                | 2 +-
> xen/arch/arm/domain.c                | 2 +-
> xen/arch/arm/guest_walk.c            | 3 ++-
> xen/arch/arm/guestcopy.c             | 2 +-
> xen/arch/arm/vgic-v3-its.c           | 2 +-
> xen/arch/x86/hvm/svm/svm.c           | 2 +-
> xen/arch/x86/hvm/viridian/viridian.c | 2 +-
> xen/arch/x86/hvm/vmx/vmx.c           | 2 +-
> xen/common/libelf/libelf-loader.c    | 2 +-
> xen/include/asm-arm/guest_access.h   | 1 -
> xen/lib/x86/private.h                | 2 +-
> 11 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> index 144793c8cea0..792c2e92a7eb 100644
> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -17,12 +17,12 @@
>  * GNU General Public License for more details.
>  */
>
> +#include <xen/guest_access.h>
> #include <xen/lib.h>
> #include <xen/sched.h>
> #include <xen/types.h>
>
> #include <asm/current.h>
> -#include <asm/guest_access.h>
>
> #include "decode.h"
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 31169326b2e3..9258f6d3faa2 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -12,6 +12,7 @@
> #include <xen/bitops.h>
> #include <xen/errno.h>
> #include <xen/grant_table.h>
> +#include <xen/guest_access.h>
> #include <xen/hypercall.h>
> #include <xen/init.h>
> #include <xen/lib.h>
> @@ -26,7 +27,6 @@
> #include <asm/current.h>
> #include <asm/event.h>
> #include <asm/gic.h>
> -#include <asm/guest_access.h>
> #include <asm/guest_atomics.h>
> #include <asm/irq.h>
> #include <asm/p2m.h>
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index a1cdd7f4afea..b4496c4c86c6 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -16,8 +16,9 @@
>  */
>
> #include <xen/domain_page.h>
> +#include <xen/guest_access.h>
> #include <xen/sched.h>
> -#include <asm/guest_access.h>
> +
> #include <asm/guest_walk.h>
> #include <asm/short-desc.h>
>
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index c8023e2bca5d..32681606d8fc 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -1,10 +1,10 @@
> #include <xen/domain_page.h>
> +#include <xen/guest_access.h>
> #include <xen/lib.h>
> #include <xen/mm.h>
> #include <xen/sched.h>
>
> #include <asm/current.h>
> -#include <asm/guest_access.h>
>
> #define COPY_flush_dcache   (1U << 0)
> #define COPY_from_guest     (0U << 1)
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 6e153c698d56..58d939b85f92 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -32,6 +32,7 @@
> #include <xen/bitops.h>
> #include <xen/config.h>
> #include <xen/domain_page.h>
> +#include <xen/guest_access.h>
> #include <xen/lib.h>
> #include <xen/init.h>
> #include <xen/softirq.h>
> @@ -39,7 +40,6 @@
> #include <xen/sched.h>
> #include <xen/sizes.h>
> #include <asm/current.h>
> -#include <asm/guest_access.h>
> #include <asm/mmio.h>
> #include <asm/gic_v3_defs.h>
> #include <asm/gic_v3_its.h>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index ca3bbfcbb355..7301f3cd6004 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -16,6 +16,7 @@
>  * this program; If not, see <http://www.gnu.org/licenses/>.
>  */
>
> +#include <xen/guest_access.h>
> #include <xen/init.h>
> #include <xen/lib.h>
> #include <xen/trace.h>
> @@ -34,7 +35,6 @@
> #include <asm/cpufeature.h>
> #include <asm/processor.h>
> #include <asm/amd.h>
> -#include <asm/guest_access.h>
> #include <asm/debugreg.h>
> #include <asm/msr.h>
> #include <asm/i387.h>
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> index 977c1bc54fad..dc7183a54627 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -5,12 +5,12 @@
>  * Hypervisor Top Level Functional Specification for more information.
>  */
>
> +#include <xen/guest_access.h>
> #include <xen/sched.h>
> #include <xen/version.h>
> #include <xen/hypercall.h>
> #include <xen/domain_page.h>
> #include <xen/param.h>
> -#include <asm/guest_access.h>
> #include <asm/guest/hyperv-tlfs.h>
> #include <asm/paging.h>
> #include <asm/p2m.h>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index eb54aadfbafb..cb5df1e81c9c 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -15,6 +15,7 @@
>  * this program; If not, see <http://www.gnu.org/licenses/>.
>  */
>
> +#include <xen/guest_access.h>
> #include <xen/init.h>
> #include <xen/lib.h>
> #include <xen/param.h>
> @@ -31,7 +32,6 @@
> #include <asm/regs.h>
> #include <asm/cpufeature.h>
> #include <asm/processor.h>
> -#include <asm/guest_access.h>
> #include <asm/debugreg.h>
> #include <asm/msr.h>
> #include <asm/p2m.h>
> diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
> index 0f468727d04a..629cc0d3e611 100644
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -16,7 +16,7 @@
>  */
>
> #ifdef __XEN__
> -#include <asm/guest_access.h>
> +#include <xen/guest_access.h>
> #endif
>
> #include "libelf-private.h"
> diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
> index 31b9f03f0015..b9a89c495527 100644
> --- a/xen/include/asm-arm/guest_access.h
> +++ b/xen/include/asm-arm/guest_access.h
> @@ -1,7 +1,6 @@
> #ifndef __ASM_ARM_GUEST_ACCESS_H__
> #define __ASM_ARM_GUEST_ACCESS_H__
>
> -#include <xen/guest_access.h>
> #include <xen/errno.h>
> #include <xen/sched.h>
>
> diff --git a/xen/lib/x86/private.h b/xen/lib/x86/private.h
> index b793181464f3..2d53bd3ced23 100644
> --- a/xen/lib/x86/private.h
> +++ b/xen/lib/x86/private.h
> @@ -4,12 +4,12 @@
> #ifdef __XEN__
>
> #include <xen/bitops.h>
> +#include <xen/guest_access.h>
> #include <xen/kernel.h>
> #include <xen/lib.h>
> #include <xen/nospec.h>
> #include <xen/types.h>
>
> -#include <asm/guest_access.h>
> #include <asm/msr-index.h>
>
> #define copy_to_buffer_offset copy_to_guest_offset
> --
> 2.17.1
>
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Bertrand Marquis July 31, 2020, 12:45 p.m. UTC | #4

> On 30 Jul 2020, at 20:18, Julien Grall <[e-mail]> wrote:
> 
> From: Julien Grall <[e-mail]>
> 
> 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 <[e-mail]>
Reviewed-by: Bertrand Marquis <[e-mail]>

(sorry forgot to remove the disclaimer in the previous one)
> 
> ---
>    Changes in v2:
>        - Remove some changes that weren't meant to be here.
> ---
> xen/arch/arm/decode.c                | 2 +-
> xen/arch/arm/domain.c                | 2 +-
> xen/arch/arm/guest_walk.c            | 3 ++-
> xen/arch/arm/guestcopy.c             | 2 +-
> xen/arch/arm/vgic-v3-its.c           | 2 +-
> xen/arch/x86/hvm/svm/svm.c           | 2 +-
> xen/arch/x86/hvm/viridian/viridian.c | 2 +-
> xen/arch/x86/hvm/vmx/vmx.c           | 2 +-
> xen/common/libelf/libelf-loader.c    | 2 +-
> xen/include/asm-arm/guest_access.h   | 1 -
> xen/lib/x86/private.h                | 2 +-
> 11 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> index 144793c8cea0..792c2e92a7eb 100644
> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -17,12 +17,12 @@
>  * GNU General Public License for more details.
>  */
> 
> +#include <xen/guest_access.h>
> #include <xen/lib.h>
> #include <xen/sched.h>
> #include <xen/types.h>
> 
> #include <asm/current.h>
> -#include <asm/guest_access.h>
> 
> #include "decode.h"
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 31169326b2e3..9258f6d3faa2 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -12,6 +12,7 @@
> #include <xen/bitops.h>
> #include <xen/errno.h>
> #include <xen/grant_table.h>
> +#include <xen/guest_access.h>
> #include <xen/hypercall.h>
> #include <xen/init.h>
> #include <xen/lib.h>
> @@ -26,7 +27,6 @@
> #include <asm/current.h>
> #include <asm/event.h>
> #include <asm/gic.h>
> -#include <asm/guest_access.h>
> #include <asm/guest_atomics.h>
> #include <asm/irq.h>
> #include <asm/p2m.h>
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index a1cdd7f4afea..b4496c4c86c6 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -16,8 +16,9 @@
>  */
> 
> #include <xen/domain_page.h>
> +#include <xen/guest_access.h>
> #include <xen/sched.h>
> -#include <asm/guest_access.h>
> +
> #include <asm/guest_walk.h>
> #include <asm/short-desc.h>
> 
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index c8023e2bca5d..32681606d8fc 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -1,10 +1,10 @@
> #include <xen/domain_page.h>
> +#include <xen/guest_access.h>
> #include <xen/lib.h>
> #include <xen/mm.h>
> #include <xen/sched.h>
> 
> #include <asm/current.h>
> -#include <asm/guest_access.h>
> 
> #define COPY_flush_dcache   (1U << 0)
> #define COPY_from_guest     (0U << 1)
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 6e153c698d56..58d939b85f92 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -32,6 +32,7 @@
> #include <xen/bitops.h>
> #include <xen/config.h>
> #include <xen/domain_page.h>
> +#include <xen/guest_access.h>
> #include <xen/lib.h>
> #include <xen/init.h>
> #include <xen/softirq.h>
> @@ -39,7 +40,6 @@
> #include <xen/sched.h>
> #include <xen/sizes.h>
> #include <asm/current.h>
> -#include <asm/guest_access.h>
> #include <asm/mmio.h>
> #include <asm/gic_v3_defs.h>
> #include <asm/gic_v3_its.h>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index ca3bbfcbb355..7301f3cd6004 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -16,6 +16,7 @@
>  * this program; If not, see <http://www.gnu.org/licenses/>.
>  */
> 
> +#include <xen/guest_access.h>
> #include <xen/init.h>
> #include <xen/lib.h>
> #include <xen/trace.h>
> @@ -34,7 +35,6 @@
> #include <asm/cpufeature.h>
> #include <asm/processor.h>
> #include <asm/amd.h>
> -#include <asm/guest_access.h>
> #include <asm/debugreg.h>
> #include <asm/msr.h>
> #include <asm/i387.h>
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> index 977c1bc54fad..dc7183a54627 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -5,12 +5,12 @@
>  * Hypervisor Top Level Functional Specification for more information.
>  */
> 
> +#include <xen/guest_access.h>
> #include <xen/sched.h>
> #include <xen/version.h>
> #include <xen/hypercall.h>
> #include <xen/domain_page.h>
> #include <xen/param.h>
> -#include <asm/guest_access.h>
> #include <asm/guest/hyperv-tlfs.h>
> #include <asm/paging.h>
> #include <asm/p2m.h>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index eb54aadfbafb..cb5df1e81c9c 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -15,6 +15,7 @@
>  * this program; If not, see <http://www.gnu.org/licenses/>.
>  */
> 
> +#include <xen/guest_access.h>
> #include <xen/init.h>
> #include <xen/lib.h>
> #include <xen/param.h>
> @@ -31,7 +32,6 @@
> #include <asm/regs.h>
> #include <asm/cpufeature.h>
> #include <asm/processor.h>
> -#include <asm/guest_access.h>
> #include <asm/debugreg.h>
> #include <asm/msr.h>
> #include <asm/p2m.h>
> diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
> index 0f468727d04a..629cc0d3e611 100644
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -16,7 +16,7 @@
>  */
> 
> #ifdef __XEN__
> -#include <asm/guest_access.h>
> +#include <xen/guest_access.h>
> #endif
> 
> #include "libelf-private.h"
> diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
> index 31b9f03f0015..b9a89c495527 100644
> --- a/xen/include/asm-arm/guest_access.h
> +++ b/xen/include/asm-arm/guest_access.h
> @@ -1,7 +1,6 @@
> #ifndef __ASM_ARM_GUEST_ACCESS_H__
> #define __ASM_ARM_GUEST_ACCESS_H__
> 
> -#include <xen/guest_access.h>
> #include <xen/errno.h>
> #include <xen/sched.h>
> 
> diff --git a/xen/lib/x86/private.h b/xen/lib/x86/private.h
> index b793181464f3..2d53bd3ced23 100644
> --- a/xen/lib/x86/private.h
> +++ b/xen/lib/x86/private.h
> @@ -4,12 +4,12 @@
> #ifdef __XEN__
> 
> #include <xen/bitops.h>
> +#include <xen/guest_access.h>
> #include <xen/kernel.h>
> #include <xen/lib.h>
> #include <xen/nospec.h>
> #include <xen/types.h>
> 
> -#include <asm/guest_access.h>
> #include <asm/msr-index.h>
> 
> #define copy_to_buffer_offset copy_to_guest_offset
> -- 
> 2.17.1
> 
>

Julien Grall Aug. 14, 2020, 7:07 p.m. UTC | #5

Hi Jan,

On 31/07/2020 12:36, Jan Beulich wrote:
> On 30.07.2020 20:18, Julien Grall wrote:
>> From: Julien Grall <[e-mail]>
>>
>> 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 <[e-mail]>
> 
> Acked-by: Jan Beulich <[e-mail]>
> 
> 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>


> 
> Jan
>

Jan Beulich Aug. 18, 2020, 8:50 a.m. UTC | #6

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 <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>
>> Acked-by: Jan Beulich <[e-mail]>
>>
>> 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.

Jan

Julien Grall Aug. 18, 2020, 8:58 a.m. UTC | #7

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 <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>
>>> Acked-by: Jan Beulich <[e-mail]>
>>>
>>> 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.

Cheers,

Bertrand Marquis Aug. 18, 2020, 9:05 a.m. UTC | #8

> On 18 Aug 2020, at 09:58, Julien Grall <[e-mail]> 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 <[e-mail]>
>>>>> 
>>>>> 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 <[e-mail]>
>>>> 
>>>> Acked-by: Jan Beulich <[e-mail]>
>>>> 
>>>> 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

Julien Grall Aug. 18, 2020, 9:23 a.m. UTC | #9

On 18/08/2020 10:05, Bertrand Marquis wrote:
> 
> 
>> On 18 Aug 2020, at 09:58, Julien Grall <[e-mail]> 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 <[e-mail]>
>>>>>>
>>>>>> 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 <[e-mail]>
>>>>>
>>>>> Acked-by: Jan Beulich <[e-mail]>
>>>>>
>>>>> 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,

Bertrand Marquis Aug. 18, 2020, 9:29 a.m. UTC | #10

> On 18 Aug 2020, at 10:23, Julien Grall <[e-mail]> wrote:
> 
> 
> 
> On 18/08/2020 10:05, Bertrand Marquis wrote:
>>> On 18 Aug 2020, at 09:58, Julien Grall <[e-mail]> 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 <[e-mail]>
>>>>>>> 
>>>>>>> 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 <[e-mail]>
>>>>>> 
>>>>>> Acked-by: Jan Beulich <[e-mail]>
>>>>>> 
>>>>>> 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

Jan Beulich Aug. 18, 2020, 11:32 a.m. UTC | #11

On 18.08.2020 10:58, Julien Grall 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 <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>
>>>> Acked-by: Jan Beulich <[e-mail]>
>>>>
>>>> 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>

But where's the problem with this? The first #include will already
have resulted in the inclusion of asm/guest_access.h, so the second
#include is simply a no-op.

Jan

Julien Grall Aug. 18, 2020, 1:14 p.m. UTC | #12

On 18/08/2020 12:32, Jan Beulich wrote:
> On 18.08.2020 10:58, Julien Grall wrote:
>>> 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>
> 
> But where's the problem with this? The first #include will already
> have resulted in the inclusion of asm/guest_access.h, so the second
> #include is simply a no-op.

A couple of reasons:
    1) I don't consider this solving completely your original request [1]
    2) I don't see how this is more important to prevent including 
<asm/guest_access.h> before and not after. Both will still compile fine, 
we just want to avoid it.


[1] "Is there any chance you could take measures to avoid new inclusions 
of asm/guest_access.h to appear?"

Jan Beulich Aug. 18, 2020, 4:04 p.m. UTC | #13

On 18.08.2020 15:14, Julien Grall wrote:
> 
> 
> On 18/08/2020 12:32, Jan Beulich wrote:
>> On 18.08.2020 10:58, Julien Grall wrote:
>>>> 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>
>>
>> But where's the problem with this? The first #include will already
>> have resulted in the inclusion of asm/guest_access.h, so the second
>> #include is simply a no-op.
> 
> A couple of reasons:
>    1) I don't consider this solving completely your original request [1]
>    2) I don't see how this is more important to prevent including <asm/guest_access.h> before and not after. Both will still compile fine, we just want to avoid it.
> 
> 
> [1] "Is there any chance you could take measures to avoid new inclusions of asm/guest_access.h to appear?"

Is

#include <xen/guest_access.h>
[...]
#include <asm/guest_access.h>

actually a problem (as opposed to an asm/ include without any include
of the xen/ one at all)?

Jan

Julien Grall Aug. 18, 2020, 4:20 p.m. UTC | #14

On 18/08/2020 17:04, Jan Beulich wrote:
> On 18.08.2020 15:14, Julien Grall wrote:
>>
>>
>> On 18/08/2020 12:32, Jan Beulich wrote:
>>> On 18.08.2020 10:58, Julien Grall wrote:
>>>>> 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>
>>>
>>> But where's the problem with this? The first #include will already
>>> have resulted in the inclusion of asm/guest_access.h, so the second
>>> #include is simply a no-op.
>>
>> A couple of reasons:
>>     1) I don't consider this solving completely your original request [1]
>>     2) I don't see how this is more important to prevent including <asm/guest_access.h> before and not after. Both will still compile fine, we just want to avoid it.
>>
>>
>> [1] "Is there any chance you could take measures to avoid new inclusions of asm/guest_access.h to appear?"
> 
> Is
> 
> #include <xen/guest_access.h>
> [...]
> #include <asm/guest_access.h>
> 
> actually a problem (as opposed to an asm/ include without any include
> of the xen/ one at all)?

Neither of them are really a problem today. So it is not entirely clear 
why we would want to prevent one and not the other.

Cheers,

Jan Beulich Aug. 18, 2020, 4:23 p.m. UTC | #15

On 18.08.2020 18:20, Julien Grall wrote:
> 
> 
> On 18/08/2020 17:04, Jan Beulich wrote:
>> On 18.08.2020 15:14, Julien Grall wrote:
>>>
>>>
>>> On 18/08/2020 12:32, Jan Beulich wrote:
>>>> On 18.08.2020 10:58, Julien Grall wrote:
>>>>>> 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>
>>>>
>>>> But where's the problem with this? The first #include will already
>>>> have resulted in the inclusion of asm/guest_access.h, so the second
>>>> #include is simply a no-op.
>>>
>>> A couple of reasons:
>>>     1) I don't consider this solving completely your original request [1]
>>>     2) I don't see how this is more important to prevent including <asm/guest_access.h> before and not after. Both will still compile fine, we just want to avoid it.
>>>
>>>
>>> [1] "Is there any chance you could take measures to avoid new inclusions of asm/guest_access.h to appear?"
>>
>> Is
>>
>> #include <xen/guest_access.h>
>> [...]
>> #include <asm/guest_access.h>
>>
>> actually a problem (as opposed to an asm/ include without any include
>> of the xen/ one at all)?
> 
> Neither of them are really a problem today. So it is not entirely clear why we would want to prevent one and not the other.

If neither is a problem, why the conversion?

Jan

Julien Grall Aug. 18, 2020, 4:34 p.m. UTC | #16

Hi Jan,

On 18/08/2020 17:23, Jan Beulich wrote:
> On 18.08.2020 18:20, Julien Grall wrote:
>>
>>
>> On 18/08/2020 17:04, Jan Beulich wrote:
>>> On 18.08.2020 15:14, Julien Grall wrote:
>>>>
>>>>
>>>> On 18/08/2020 12:32, Jan Beulich wrote:
>>>>> On 18.08.2020 10:58, Julien Grall wrote:
>>>>>>> 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>
>>>>>
>>>>> But where's the problem with this? The first #include will already
>>>>> have resulted in the inclusion of asm/guest_access.h, so the second
>>>>> #include is simply a no-op.
>>>>
>>>> A couple of reasons:
>>>>      1) I don't consider this solving completely your original request [1]
>>>>      2) I don't see how this is more important to prevent including <asm/guest_access.h> before and not after. Both will still compile fine, we just want to avoid it.
>>>>
>>>>
>>>> [1] "Is there any chance you could take measures to avoid new inclusions of asm/guest_access.h to appear?"
>>>
>>> Is
>>>
>>> #include <xen/guest_access.h>
>>> [...]
>>> #include <asm/guest_access.h>
>>>
>>> actually a problem (as opposed to an asm/ include without any include
>>> of the xen/ one at all)?
>>
>> Neither of them are really a problem today. So it is not entirely clear why we would want to prevent one and not the other.
> 
> If neither is a problem, why the conversion?

As I wrote in the commit message, some of the helpers will be moved from 
asm/guest_access.h. So existing users of asm/guest_access.h may not 
compiled anymore.

If you are not using any helpers from xen/guest_access.h, then you could 
theoritically only include asm/guest_access.h. But they are quite 
limited or maybe don't even exist.

Actually xen/guest_access.h was included from asm/guest_access.h. But 
there are compilation issues if you try to include xen/guest_access.h 
from asm/guest_access.h.

Therefore it is better to request everyone to include 
<xen/guest_access.h>. With that you get all the guest access helpers 
rather than just the arch specific one.

Cheers,

Julien Grall Sept. 18, 2020, 6:39 p.m. UTC | #17

Hi all,

I was going to commit this patch but I realized that I still don't have 
a review from the maintainers of...

On 30/07/2020 19:18, Julien Grall wrote:
>   xen/arch/x86/hvm/viridian/viridian.c | 2 +-
... this file (Paul and Wei) and...

>   xen/arch/x86/hvm/vmx/vmx.c           | 2 +-

.. this one (Jun and Kevin).

Can I get one?

The change is pretty trivial so I plan to commit it on Monday afternoon 
unless otherwise.

Cheers,

Paul Durrant Sept. 21, 2020, 3:32 p.m. UTC | #18

> -----Original Message-----
> From: Julien Grall <[e-mail]>
> Sent: 30 July 2020 19:18
> To: [e-mail]
> Cc: [e-mail]; Julien Grall <[e-mail]>; Stefano Stabellini <[e-mail]>;
> Volodymyr Babchuk <[e-mail]>; Andrew Cooper <[e-mail]>; George
> Dunlap <[e-mail]>; Ian Jackson <[e-mail]>; Jan Beulich
> <[e-mail]>; Wei Liu <[e-mail]>; Roger Pau Monné <[e-mail]>; Paul Durrant
> <[e-mail]>; Jun Nakajima <[e-mail]>; Kevin Tian <[e-mail]>
> Subject: [RESEND][PATCH v2 5/7] xen: include xen/guest_access.h rather than asm/guest_access.h
> 
> From: Julien Grall <[e-mail]>
> 
> 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 <[e-mail]>

Viridian change...

Acked-by: Paul Durrant <[e-mail]>

Patch

diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
index 144793c8cea0..792c2e92a7eb 100644
--- a/xen/arch/arm/decode.c
+++ b/xen/arch/arm/decode.c
@@ -17,12 +17,12 @@ 
  * GNU General Public License for more details.
  */
 
+#include <xen/guest_access.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <xen/types.h>
 
 #include <asm/current.h>
-#include <asm/guest_access.h>
 
 #include "decode.h"
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 31169326b2e3..9258f6d3faa2 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -12,6 +12,7 @@ 
 #include <xen/bitops.h>
 #include <xen/errno.h>
 #include <xen/grant_table.h>
+#include <xen/guest_access.h>
 #include <xen/hypercall.h>
 #include <xen/init.h>
 #include <xen/lib.h>
@@ -26,7 +27,6 @@ 
 #include <asm/current.h>
 #include <asm/event.h>
 #include <asm/gic.h>
-#include <asm/guest_access.h>
 #include <asm/guest_atomics.h>
 #include <asm/irq.h>
 #include <asm/p2m.h>
diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index a1cdd7f4afea..b4496c4c86c6 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -16,8 +16,9 @@ 
  */
 
 #include <xen/domain_page.h>
+#include <xen/guest_access.h>
 #include <xen/sched.h>
-#include <asm/guest_access.h>
+
 #include <asm/guest_walk.h>
 #include <asm/short-desc.h>
 
diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index c8023e2bca5d..32681606d8fc 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -1,10 +1,10 @@ 
 #include <xen/domain_page.h>
+#include <xen/guest_access.h>
 #include <xen/lib.h>
 #include <xen/mm.h>
 #include <xen/sched.h>
 
 #include <asm/current.h>
-#include <asm/guest_access.h>
 
 #define COPY_flush_dcache   (1U << 0)
 #define COPY_from_guest     (0U << 1)
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 6e153c698d56..58d939b85f92 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -32,6 +32,7 @@ 
 #include <xen/bitops.h>
 #include <xen/config.h>
 #include <xen/domain_page.h>
+#include <xen/guest_access.h>
 #include <xen/lib.h>
 #include <xen/init.h>
 #include <xen/softirq.h>
@@ -39,7 +40,6 @@ 
 #include <xen/sched.h>
 #include <xen/sizes.h>
 #include <asm/current.h>
-#include <asm/guest_access.h>
 #include <asm/mmio.h>
 #include <asm/gic_v3_defs.h>
 #include <asm/gic_v3_its.h>
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index ca3bbfcbb355..7301f3cd6004 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -16,6 +16,7 @@ 
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/guest_access.h>
 #include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/trace.h>
@@ -34,7 +35,6 @@ 
 #include <asm/cpufeature.h>
 #include <asm/processor.h>
 #include <asm/amd.h>
-#include <asm/guest_access.h>
 #include <asm/debugreg.h>
 #include <asm/msr.h>
 #include <asm/i387.h>
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 977c1bc54fad..dc7183a54627 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -5,12 +5,12 @@ 
  * Hypervisor Top Level Functional Specification for more information.
  */
 
+#include <xen/guest_access.h>
 #include <xen/sched.h>
 #include <xen/version.h>
 #include <xen/hypercall.h>
 #include <xen/domain_page.h>
 #include <xen/param.h>
-#include <asm/guest_access.h>
 #include <asm/guest/hyperv-tlfs.h>
 #include <asm/paging.h>
 #include <asm/p2m.h>
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index eb54aadfbafb..cb5df1e81c9c 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -15,6 +15,7 @@ 
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/guest_access.h>
 #include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/param.h>
@@ -31,7 +32,6 @@ 
 #include <asm/regs.h>
 #include <asm/cpufeature.h>
 #include <asm/processor.h>
-#include <asm/guest_access.h>
 #include <asm/debugreg.h>
 #include <asm/msr.h>
 #include <asm/p2m.h>
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 0f468727d04a..629cc0d3e611 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -16,7 +16,7 @@ 
  */
 
 #ifdef __XEN__
-#include <asm/guest_access.h>
+#include <xen/guest_access.h>
 #endif
 
 #include "libelf-private.h"
diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
index 31b9f03f0015..b9a89c495527 100644
--- a/xen/include/asm-arm/guest_access.h
+++ b/xen/include/asm-arm/guest_access.h
@@ -1,7 +1,6 @@ 
 #ifndef __ASM_ARM_GUEST_ACCESS_H__
 #define __ASM_ARM_GUEST_ACCESS_H__
 
-#include <xen/guest_access.h>
 #include <xen/errno.h>
 #include <xen/sched.h>
 
diff --git a/xen/lib/x86/private.h b/xen/lib/x86/private.h
index b793181464f3..2d53bd3ced23 100644
--- a/xen/lib/x86/private.h
+++ b/xen/lib/x86/private.h
@@ -4,12 +4,12 @@ 
 #ifdef __XEN__
 
 #include <xen/bitops.h>
+#include <xen/guest_access.h>
 #include <xen/kernel.h>
 #include <xen/lib.h>
 #include <xen/nospec.h>
 #include <xen/types.h>
 
-#include <asm/guest_access.h>
 #include <asm/msr-index.h>
 
 #define copy_to_buffer_offset copy_to_guest_offset