[RESEND][PATCH v2 0/7] xen: Consolidate asm-*/guest_access.h in xen/guest_access.h

[RESEND][PATCH v2 0/7] xen: Consolidate asm-*/guest_access.h in xen/guest_access.h

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

A lot of the helpers implemented in asm-*/guest_access.h are implemented
the same way. This series aims to avoid the duplication and implement
them only once in xen/guest_access.h.

Cheers,

Julien Grall (7):
  xen/guest_access: Add emacs magics
  xen/arm: kernel: Re-order the includes
  xen/arm: decode: Re-order the includes
  xen/arm: guestcopy: Re-order the includes
  xen: include xen/guest_access.h rather than asm/guest_access.h
  xen/guest_access: Consolidate guest access helpers in
    xen/guest_access.h
  xen/guest_access: Fix coding style in xen/guest_access.h

 xen/arch/arm/decode.c                |   7 +-
 xen/arch/arm/domain.c                |   2 +-
 xen/arch/arm/guest_walk.c            |   3 +-
 xen/arch/arm/guestcopy.c             |   5 +-
 xen/arch/arm/kernel.c                |  12 +--
 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   | 115 -----------------------
 xen/include/asm-x86/guest_access.h   | 116 ++----------------------
 xen/include/xen/guest_access.h       | 131 +++++++++++++++++++++++++++
 xen/lib/x86/private.h                |   2 +-
 14 files changed, 161 insertions(+), 242 deletions(-)

-- 
2.17.1

[RESEND][PATCH v2 1/7] xen/guest_access: Add emacs magics

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Add emacs magics for xen/guest_access.h and
asm-x86/guest_access.h.

Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

---
    Changes in v2:
        - Remove the word "missing"
---
 xen/include/asm-x86/guest_access.h | 8 ++++++++
 xen/include/xen/guest_access.h     | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/xen/include/asm-x86/guest_access.h b/xen/include/asm-x86/guest_access.h
index 2be3577bd340..3ffde205f6a1 100644
--- a/xen/include/asm-x86/guest_access.h
+++ b/xen/include/asm-x86/guest_access.h
@@ -160,3 +160,11 @@
 })
 
 #endif /* __ASM_X86_GUEST_ACCESS_H__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
index 09989df819ce..ef9aaa3efcfe 100644
--- a/xen/include/xen/guest_access.h
+++ b/xen/include/xen/guest_access.h
@@ -33,3 +33,11 @@ char *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
                                   size_t size, size_t max_size);
 
 #endif /* __XEN_GUEST_ACCESS_H__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.17.1

Re: [RESEND][PATCH v2 1/7] xen/guest_access: Add emacs magics

[Bertrand Marquis]

> On 30 Jul 2020, at 20:18, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Add emacs magics for xen/guest_access.h and
> asm-x86/guest_access.h.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Most of file in Xen source code seem to have a white line before the “emacs magics”.
If this is something that should be enforced, it should be done here.

If not the change seems ok :-)

> 
> ---
>    Changes in v2:
>        - Remove the word "missing"
> ---
> xen/include/asm-x86/guest_access.h | 8 ++++++++
> xen/include/xen/guest_access.h     | 8 ++++++++
> 2 files changed, 16 insertions(+)
> 
> diff --git a/xen/include/asm-x86/guest_access.h b/xen/include/asm-x86/guest_access.h
> index 2be3577bd340..3ffde205f6a1 100644
> --- a/xen/include/asm-x86/guest_access.h
> +++ b/xen/include/asm-x86/guest_access.h
> @@ -160,3 +160,11 @@
> })
> 
> #endif /* __ASM_X86_GUEST_ACCESS_H__ */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
> index 09989df819ce..ef9aaa3efcfe 100644
> --- a/xen/include/xen/guest_access.h
> +++ b/xen/include/xen/guest_access.h
> @@ -33,3 +33,11 @@ char *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
>                                   size_t size, size_t max_size);
> 
> #endif /* __XEN_GUEST_ACCESS_H__ */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> 2.17.1
> 
> 

Re: [RESEND][PATCH v2 1/7] xen/guest_access: Add emacs magics

[Julien Grall]

Hi,

On 31/07/2020 14:04, Bertrand Marquis wrote:
> 
> 
>> On 30 Jul 2020, at 20:18, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Add emacs magics for xen/guest_access.h and
>> asm-x86/guest_access.h.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Most of file in Xen source code seem to have a white line before the “emacs magics”.
> If this is something that should be enforced, it should be done here.
I am not sure whether we always ask the newline before. Anyway, I have 
added one as this is trivial.

Cheers,

-- 
Julien Grall

Re: [RESEND][PATCH v2 1/7] xen/guest_access: Add emacs magics

[Volodymyr Babchuk]

Hi Julie,

Julien Grall writes:

> From: Julien Grall <jgrall@amazon.com>
>
> Add emacs magics for xen/guest_access.h and
> asm-x86/guest_access.h.

As Emacs user I fully approve this.

But I want to hijack this thread a little to discuss even better
solution. Emacs supports .dir-locals.el file [1], which allows to set
all per-buffer variables once for all files down the directory tree. So,
instead of having "/* Local variables */" scourge in every file we can have
bunch of ".dir-locals.el" files placed in a strategic places to define
coding styles for different parts of Xen. As a bonus, it will be
possible to define Linux coding style for files taken from Linux, for example.

[1] https://www.gnu.org/software/emacs/manual/html_node/emacs/Directory-Variables.html

-- 
Volodymyr Babchuk at EPAM

[RESEND][PATCH v2 2/7] xen/arm: kernel: Re-order the includes

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

We usually have xen/ includes first and then asm/. They are also ordered
alphabetically among themselves.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/kernel.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 8eff0748367d..f95fa392af44 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -3,20 +3,20 @@
  *
  * Copyright (C) 2011 Citrix Systems, Inc.
  */
+#include <xen/domain_page.h>
 #include <xen/errno.h>
+#include <xen/gunzip.h>
 #include <xen/init.h>
 #include <xen/lib.h>
+#include <xen/libfdt/libfdt.h>
 #include <xen/mm.h>
-#include <xen/domain_page.h>
 #include <xen/sched.h>
-#include <asm/byteorder.h>
-#include <asm/setup.h>
-#include <xen/libfdt/libfdt.h>
-#include <xen/gunzip.h>
 #include <xen/vmap.h>
 
+#include <asm/byteorder.h>
 #include <asm/guest_access.h>
 #include <asm/kernel.h>
+#include <asm/setup.h>
 
 #define UIMAGE_MAGIC          0x27051956
 #define UIMAGE_NMLEN          32
-- 
2.17.1

Re: [RESEND][PATCH v2 2/7] xen/arm: kernel: Re-order the includes

[Stefano Stabellini]

On Thu, 30 Jul 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> We usually have xen/ includes first and then asm/. They are also ordered
> alphabetically among themselves.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/kernel.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 8eff0748367d..f95fa392af44 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -3,20 +3,20 @@
>   *
>   * Copyright (C) 2011 Citrix Systems, Inc.
>   */
> +#include <xen/domain_page.h>
>  #include <xen/errno.h>
> +#include <xen/gunzip.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
> +#include <xen/libfdt/libfdt.h>
>  #include <xen/mm.h>
> -#include <xen/domain_page.h>
>  #include <xen/sched.h>
> -#include <asm/byteorder.h>
> -#include <asm/setup.h>
> -#include <xen/libfdt/libfdt.h>
> -#include <xen/gunzip.h>
>  #include <xen/vmap.h>
>  
> +#include <asm/byteorder.h>
>  #include <asm/guest_access.h>
>  #include <asm/kernel.h>
> +#include <asm/setup.h>
>  
>  #define UIMAGE_MAGIC          0x27051956
>  #define UIMAGE_NMLEN          32
> -- 
> 2.17.1
> 

Re: [RESEND][PATCH v2 2/7] xen/arm: kernel: Re-order the includes

[Bertrand Marquis]

> On 30 Jul 2020, at 20:18, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> We usually have xen/ includes first and then asm/. They are also ordered
> alphabetically among themselves.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

> ---
> xen/arch/arm/kernel.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 8eff0748367d..f95fa392af44 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -3,20 +3,20 @@
>  *
>  * Copyright (C) 2011 Citrix Systems, Inc.
>  */
> +#include <xen/domain_page.h>
> #include <xen/errno.h>
> +#include <xen/gunzip.h>
> #include <xen/init.h>
> #include <xen/lib.h>
> +#include <xen/libfdt/libfdt.h>
> #include <xen/mm.h>
> -#include <xen/domain_page.h>
> #include <xen/sched.h>
> -#include <asm/byteorder.h>
> -#include <asm/setup.h>
> -#include <xen/libfdt/libfdt.h>
> -#include <xen/gunzip.h>
> #include <xen/vmap.h>
> 
> +#include <asm/byteorder.h>
> #include <asm/guest_access.h>
> #include <asm/kernel.h>
> +#include <asm/setup.h>
> 
> #define UIMAGE_MAGIC          0x27051956
> #define UIMAGE_NMLEN          32
> -- 
> 2.17.1
> 
> 

[RESEND][PATCH v2 3/7] xen/arm: decode: Re-order the includes

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

We usually have xen/ includes first and then asm/. They are also ordered
alphabetically among themselves.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/decode.c | 5 +++--
 xen/arch/arm/kernel.c | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
index 8b1e15d11892..144793c8cea0 100644
--- a/xen/arch/arm/decode.c
+++ b/xen/arch/arm/decode.c
@@ -17,11 +17,12 @@
  * GNU General Public License for more details.
  */
 
-#include <xen/types.h>
+#include <xen/lib.h>
 #include <xen/sched.h>
+#include <xen/types.h>
+
 #include <asm/current.h>
 #include <asm/guest_access.h>
-#include <xen/lib.h>
 
 #include "decode.h"
 
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index f95fa392af44..032923853f2c 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -5,6 +5,7 @@
  */
 #include <xen/domain_page.h>
 #include <xen/errno.h>
+#include <xen/guest_access.h>
 #include <xen/gunzip.h>
 #include <xen/init.h>
 #include <xen/lib.h>
@@ -14,7 +15,6 @@
 #include <xen/vmap.h>
 
 #include <asm/byteorder.h>
-#include <asm/guest_access.h>
 #include <asm/kernel.h>
 #include <asm/setup.h>
 
-- 
2.17.1

Re: [RESEND][PATCH v2 3/7] xen/arm: decode: Re-order the includes

[Stefano Stabellini]

On Thu, 30 Jul 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> We usually have xen/ includes first and then asm/. They are also ordered
> alphabetically among themselves.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Might wanna mention the change from asm/guest_access.h to
xen/guest_access.h. Anyway:

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/decode.c | 5 +++--
>  xen/arch/arm/kernel.c | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> index 8b1e15d11892..144793c8cea0 100644
> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -17,11 +17,12 @@
>   * GNU General Public License for more details.
>   */
>  
> -#include <xen/types.h>
> +#include <xen/lib.h>
>  #include <xen/sched.h>
> +#include <xen/types.h>
> +
>  #include <asm/current.h>
>  #include <asm/guest_access.h>
> -#include <xen/lib.h>
>  
>  #include "decode.h"
>  
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index f95fa392af44..032923853f2c 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -5,6 +5,7 @@
>   */
>  #include <xen/domain_page.h>
>  #include <xen/errno.h>
> +#include <xen/guest_access.h>
>  #include <xen/gunzip.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
> @@ -14,7 +15,6 @@
>  #include <xen/vmap.h>
>  
>  #include <asm/byteorder.h>
> -#include <asm/guest_access.h>
>  #include <asm/kernel.h>
>  #include <asm/setup.h>

Re: [RESEND][PATCH v2 3/7] xen/arm: decode: Re-order the includes

[Julien Grall]

Hi Stefano,

On 30/07/2020 20:37, Stefano Stabellini wrote:
> On Thu, 30 Jul 2020, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> We usually have xen/ includes first and then asm/. They are also ordered
>> alphabetically among themselves.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Might wanna mention the change from asm/guest_access.h to
> xen/guest_access.h. 

Actually, the change in kernel.c belongs to patch #5. I will move it there.

> Anyway:
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>

I will keep the Acked-by for the first change.

Thank you,

-- 
Julien Grall

[RESEND][PATCH v2 4/7] xen/arm: guestcopy: Re-order the includes

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

We usually have xen/ includes first and then asm/. They are also ordered
alphabetically among themselves.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/guestcopy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index 7a0f3e9d5fc6..c8023e2bca5d 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -1,7 +1,8 @@
-#include <xen/lib.h>
 #include <xen/domain_page.h>
+#include <xen/lib.h>
 #include <xen/mm.h>
 #include <xen/sched.h>
+
 #include <asm/current.h>
 #include <asm/guest_access.h>
 
-- 
2.17.1

Re: [RESEND][PATCH v2 4/7] xen/arm: guestcopy: Re-order the includes

[Stefano Stabellini]

On Thu, 30 Jul 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> We usually have xen/ includes first and then asm/. They are also ordered
> alphabetically among themselves.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
>  xen/arch/arm/guestcopy.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index 7a0f3e9d5fc6..c8023e2bca5d 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -1,7 +1,8 @@
> -#include <xen/lib.h>
>  #include <xen/domain_page.h>
> +#include <xen/lib.h>
>  #include <xen/mm.h>
>  #include <xen/sched.h>
> +
>  #include <asm/current.h>
>  #include <asm/guest_access.h>
>  
> -- 
> 2.17.1
> 

Re: [RESEND][PATCH v2 4/7] xen/arm: guestcopy: Re-order the includes

[Bertrand Marquis]

> On 30 Jul 2020, at 20:18, Julien Grall <julien@xen.org> wrote:
>
> From: Julien Grall <jgrall@amazon.com>
>
> We usually have xen/ includes first and then asm/. They are also ordered
> alphabetically among themselves.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

This could have been merged in patch 3.

But anyway:
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>


> ---
> xen/arch/arm/guestcopy.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index 7a0f3e9d5fc6..c8023e2bca5d 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -1,7 +1,8 @@
> -#include <xen/lib.h>
> #include <xen/domain_page.h>
> +#include <xen/lib.h>
> #include <xen/mm.h>
> #include <xen/sched.h>
> +
> #include <asm/current.h>
> #include <asm/guest_access.h>
>
> --
> 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.

Re: [RESEND][PATCH v2 4/7] xen/arm: guestcopy: Re-order the includes

[Bertrand Marquis]

> On 31 Jul 2020, at 14:53, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
> 
> 
> 
>> On 30 Jul 2020, at 20:18, Julien Grall <julien@xen.org> wrote:
>> 
>> From: Julien Grall <jgrall@amazon.com>
>> 
>> We usually have xen/ includes first and then asm/. They are also ordered
>> alphabetically among themselves.
>> 
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> This could have been merged in patch 3.
> 
> But anyway:
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> 
>> ---
>> xen/arch/arm/guestcopy.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
>> index 7a0f3e9d5fc6..c8023e2bca5d 100644
>> --- a/xen/arch/arm/guestcopy.c
>> +++ b/xen/arch/arm/guestcopy.c
>> @@ -1,7 +1,8 @@
>> -#include <xen/lib.h>
>> #include <xen/domain_page.h>
>> +#include <xen/lib.h>
>> #include <xen/mm.h>
>> #include <xen/sched.h>
>> +
>> #include <asm/current.h>
>> #include <asm/guest_access.h>
>> 
>> --
>> 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.

sorry for the notice, i need to find a way to turn it off automatically :-)

Re: [RESEND][PATCH v2 4/7] xen/arm: guestcopy: Re-order the includes

[Julien Grall]

Hi,

On 31/07/2020 13:53, Bertrand Marquis wrote:
> 
> 
>> On 30 Jul 2020, at 20:18, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> We usually have xen/ includes first and then asm/. They are also ordered
>> alphabetically among themselves.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> This could have been merged in patch 3.
This is not a tree-wide conversion (there are more instances in 
arch/arm), so I would prefer to keep one patch per file.

> But anyway:
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks!

Cheers,

-- 
Julien Grall

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

[Julien Grall]

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>

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

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

[Stefano Stabellini]

On Thu, 30 Jul 2020, 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: Stefano Stabellini <sstabellini@kernel.org>


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

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

[Jan Beulich]

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?

Jan

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

[Julien Grall]

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>


> 
> Jan
> 

-- 
Julien Grall

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

[Jan Beulich]

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.

Jan

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

[Julien Grall]

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.

Cheers,

-- 
Julien Grall

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

[Bertrand Marquis]

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

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

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

[Julien Grall]

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>

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

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

[Bertrand Marquis]

> 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

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

[Jan Beulich]

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

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

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

[Julien Grall]

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?"

-- 
Julien Grall

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

[Jan Beulich]

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Jan Beulich]

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

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

[Julien Grall]

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

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

[Bertrand Marquis]

> On 30 Jul 2020, at 20:18, Julien Grall <julien@xen.org> 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>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

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

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

[Bertrand Marquis]

> On 30 Jul 2020, at 20:18, Julien Grall <julien@xen.org> 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>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 30 July 2020 19:18
> To: xen-devel@lists.xenproject.org
> Cc: julien@xen.org; Julien Grall <jgrall@amazon.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper <andrew.cooper3@citrix.com>; George
> Dunlap <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Paul Durrant
> <paul@xen.org>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>
> Subject: [RESEND][PATCH v2 5/7] xen: include xen/guest_access.h rather than asm/guest_access.h
> 
> 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>

Viridian change...

Acked-by: Paul Durrant <paul@xen.org>

[RESEND][PATCH v2 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Most of the helpers to access guest memory are implemented the same way
on Arm and x86. The only differences are:
    - guest_handle_{from, to}_param(): while on x86 XEN_GUEST_HANDLE()
      and XEN_GUEST_HANDLE_PARAM() are the same, they are not on Arm. It
      is still fine to use the Arm implementation on x86.
    - __clear_guest_offset(): Interestingly the prototype does not match
      between the x86 and Arm. However, the Arm one is bogus. So the x86
      implementation can be used.
    - guest_handle{,_subrange}_okay(): They are validly differing
      because Arm is only supporting auto-translated guest and therefore
      handles are always valid.

In the past, the ia64 and ppc64 port use a different model to access
guest parameter. They have been long gone now.

Given Xen currently only support 2 archictures, it is too soon to have a
directory asm-generic as it is not possible to differentiate it with the
existing directory xen/. If/When there is a 3rd port, we can decide to
create the new directory if that new port decide to use a different way
to access guest parameter.

For now, consolidate it in xen/guest_access.h.

While it would be possible to adjust the coding style at the same, this
is left for a follow-up patch so 'diff' can be used to check the
consolidation was done correctly.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---
    Changes in v2:
        - Expand the commit message explaining why asm-generic is not
        created.
---
 xen/include/asm-arm/guest_access.h | 114 ---------------------------
 xen/include/asm-x86/guest_access.h | 108 --------------------------
 xen/include/xen/guest_access.h     | 119 +++++++++++++++++++++++++++++
 3 files changed, 119 insertions(+), 222 deletions(-)

diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
index b9a89c495527..53766386d3d8 100644
--- a/xen/include/asm-arm/guest_access.h
+++ b/xen/include/asm-arm/guest_access.h
@@ -23,88 +23,6 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf,
 #define __raw_copy_from_guest raw_copy_from_guest
 #define __raw_clear_guest raw_clear_guest
 
-/* Remainder copied from x86 -- could be common? */
-
-/* Is the guest handle a NULL reference? */
-#define guest_handle_is_null(hnd)        ((hnd).p == NULL)
-
-/* Offset the given guest handle into the array it refers to. */
-#define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
-#define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
-
-/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
- * to the specified type of XEN_GUEST_HANDLE_PARAM. */
-#define guest_handle_cast(hnd, type) ({         \
-    type *_x = (hnd).p;                         \
-    (XEN_GUEST_HANDLE_PARAM(type)) { _x };            \
-})
-
-/* Convert a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
-#define guest_handle_to_param(hnd, type) ({                  \
-    typeof((hnd).p) _x = (hnd).p;                            \
-    XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
-    /* type checking: make sure that the pointers inside     \
-     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
-     * the same type, then return hnd */                     \
-    (void)(&_x == &_y.p);                                    \
-    _y;                                                      \
-})
-
-#define guest_handle_for_field(hnd, type, fld)          \
-    ((XEN_GUEST_HANDLE(type)) { &(hnd).p->fld })
-
-#define guest_handle_from_ptr(ptr, type)        \
-    ((XEN_GUEST_HANDLE_PARAM(type)) { (type *)ptr })
-#define const_guest_handle_from_ptr(ptr, type)  \
-    ((XEN_GUEST_HANDLE_PARAM(const_##type)) { (const type *)ptr })
-
-/*
- * Copy an array of objects to guest context via a guest handle,
- * specifying an offset into the guest array.
- */
-#define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
-    const typeof(*(ptr)) *_s = (ptr);                   \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
-    /* Check that the handle is not for a const type */ \
-    void *__maybe_unused _t = (hnd).p;                  \
-    (void)((hnd).p == _s);                              \
-    raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
-})
-
-/*
- * Clear an array of objects in guest context via a guest handle,
- * specifying an offset into the guest array.
- */
-#define clear_guest_offset(hnd, off, nr) ({    \
-    void *_d = (hnd).p;                        \
-    raw_clear_guest(_d+(off), nr);             \
-})
-
-/*
- * Copy an array of objects from guest context via a guest handle,
- * specifying an offset into the guest array.
- */
-#define copy_from_guest_offset(ptr, hnd, off, nr) ({    \
-    const typeof(*(ptr)) *_s = (hnd).p;                 \
-    typeof(*(ptr)) *_d = (ptr);                         \
-    raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
-})
-
-/* Copy sub-field of a structure to guest context via a guest handle. */
-#define copy_field_to_guest(hnd, ptr, field) ({         \
-    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
-    void *_d = &(hnd).p->field;                         \
-    (void)(&(hnd).p->field == _s);                      \
-    raw_copy_to_guest(_d, _s, sizeof(*_s));             \
-})
-
-/* Copy sub-field of a structure from guest context via a guest handle. */
-#define copy_field_from_guest(ptr, hnd, field) ({       \
-    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
-    typeof(&(ptr)->field) _d = &(ptr)->field;           \
-    raw_copy_from_guest(_d, _s, sizeof(*_d));           \
-})
-
 /*
  * Pre-validate a guest handle.
  * Allows use of faster __copy_* functions.
@@ -113,38 +31,6 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf,
 #define guest_handle_okay(hnd, nr) (1)
 #define guest_handle_subrange_okay(hnd, first, last) (1)
 
-#define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
-    const typeof(*(ptr)) *_s = (ptr);                   \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
-    /* Check that the handle is not for a const type */ \
-    void *__maybe_unused _t = (hnd).p;                  \
-    (void)((hnd).p == _s);                              \
-    __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
-})
-
-#define __clear_guest_offset(hnd, off, ptr, nr) ({      \
-    __raw_clear_guest(_d+(off), nr);  \
-})
-
-#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
-    const typeof(*(ptr)) *_s = (hnd).p;                 \
-    typeof(*(ptr)) *_d = (ptr);                         \
-    __raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
-})
-
-#define __copy_field_to_guest(hnd, ptr, field) ({       \
-    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
-    void *_d = &(hnd).p->field;                         \
-    (void)(&(hnd).p->field == _s);                      \
-    __raw_copy_to_guest(_d, _s, sizeof(*_s));           \
-})
-
-#define __copy_field_from_guest(ptr, hnd, field) ({     \
-    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
-    typeof(&(ptr)->field) _d = &(ptr)->field;           \
-    __raw_copy_from_guest(_d, _s, sizeof(*_d));         \
-})
-
 #endif /* __ASM_ARM_GUEST_ACCESS_H__ */
 /*
  * Local variables:
diff --git a/xen/include/asm-x86/guest_access.h b/xen/include/asm-x86/guest_access.h
index 3ffde205f6a1..08c9fbbc78e1 100644
--- a/xen/include/asm-x86/guest_access.h
+++ b/xen/include/asm-x86/guest_access.h
@@ -38,81 +38,6 @@
      clear_user_hvm((dst), (len)) :             \
      clear_user((dst), (len)))
 
-/* Is the guest handle a NULL reference? */
-#define guest_handle_is_null(hnd)        ((hnd).p == NULL)
-
-/* Offset the given guest handle into the array it refers to. */
-#define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
-#define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
-
-/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
- * to the specified type of XEN_GUEST_HANDLE_PARAM. */
-#define guest_handle_cast(hnd, type) ({         \
-    type *_x = (hnd).p;                         \
-    (XEN_GUEST_HANDLE_PARAM(type)) { _x };            \
-})
-
-/* Convert a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
-#define guest_handle_to_param(hnd, type) ({                  \
-    /* type checking: make sure that the pointers inside     \
-     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
-     * the same type, then return hnd */                     \
-    (void)((typeof(&(hnd).p)) 0 ==                           \
-        (typeof(&((XEN_GUEST_HANDLE_PARAM(type)) {}).p)) 0); \
-    (hnd);                                                   \
-})
-
-#define guest_handle_for_field(hnd, type, fld)          \
-    ((XEN_GUEST_HANDLE(type)) { &(hnd).p->fld })
-
-#define guest_handle_from_ptr(ptr, type)        \
-    ((XEN_GUEST_HANDLE_PARAM(type)) { (type *)ptr })
-#define const_guest_handle_from_ptr(ptr, type)  \
-    ((XEN_GUEST_HANDLE_PARAM(const_##type)) { (const type *)ptr })
-
-/*
- * Copy an array of objects to guest context via a guest handle,
- * specifying an offset into the guest array.
- */
-#define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
-    const typeof(*(ptr)) *_s = (ptr);                   \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
-    /* Check that the handle is not for a const type */ \
-    void *__maybe_unused _t = (hnd).p;                  \
-    (void)((hnd).p == _s);                              \
-    raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
-})
-
-/*
- * Copy an array of objects from guest context via a guest handle,
- * specifying an offset into the guest array.
- */
-#define copy_from_guest_offset(ptr, hnd, off, nr) ({    \
-    const typeof(*(ptr)) *_s = (hnd).p;                 \
-    typeof(*(ptr)) *_d = (ptr);                         \
-    raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
-})
-
-#define clear_guest_offset(hnd, off, nr) ({    \
-    void *_d = (hnd).p;                        \
-    raw_clear_guest(_d+(off), nr);             \
-})
-
-/* Copy sub-field of a structure to guest context via a guest handle. */
-#define copy_field_to_guest(hnd, ptr, field) ({         \
-    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
-    void *_d = &(hnd).p->field;                         \
-    (void)(&(hnd).p->field == _s);                      \
-    raw_copy_to_guest(_d, _s, sizeof(*_s));             \
-})
-
-/* Copy sub-field of a structure from guest context via a guest handle. */
-#define copy_field_from_guest(ptr, hnd, field) ({       \
-    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
-    typeof(&(ptr)->field) _d = &(ptr)->field;           \
-    raw_copy_from_guest(_d, _s, sizeof(*_d));           \
-})
-
 /*
  * Pre-validate a guest handle.
  * Allows use of faster __copy_* functions.
@@ -126,39 +51,6 @@
                      (last)-(first)+1,                  \
                      sizeof(*(hnd).p)))
 
-#define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
-    const typeof(*(ptr)) *_s = (ptr);                   \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
-    /* Check that the handle is not for a const type */ \
-    void *__maybe_unused _t = (hnd).p;                  \
-    (void)((hnd).p == _s);                              \
-    __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
-})
-
-#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
-    const typeof(*(ptr)) *_s = (hnd).p;                 \
-    typeof(*(ptr)) *_d = (ptr);                         \
-    __raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
-})
-
-#define __clear_guest_offset(hnd, off, nr) ({    \
-    void *_d = (hnd).p;                          \
-    __raw_clear_guest(_d+(off), nr);             \
-})
-
-#define __copy_field_to_guest(hnd, ptr, field) ({       \
-    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
-    void *_d = &(hnd).p->field;                         \
-    (void)(&(hnd).p->field == _s);                      \
-    __raw_copy_to_guest(_d, _s, sizeof(*_s));           \
-})
-
-#define __copy_field_from_guest(ptr, hnd, field) ({     \
-    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
-    typeof(&(ptr)->field) _d = &(ptr)->field;           \
-    __raw_copy_from_guest(_d, _s, sizeof(*_d));         \
-})
-
 #endif /* __ASM_X86_GUEST_ACCESS_H__ */
 /*
  * Local variables:
diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
index ef9aaa3efcfe..4957b8d1f2b8 100644
--- a/xen/include/xen/guest_access.h
+++ b/xen/include/xen/guest_access.h
@@ -11,6 +11,86 @@
 #include <xen/types.h>
 #include <public/xen.h>
 
+/* Is the guest handle a NULL reference? */
+#define guest_handle_is_null(hnd)        ((hnd).p == NULL)
+
+/* Offset the given guest handle into the array it refers to. */
+#define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
+#define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
+
+/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
+ * to the specified type of XEN_GUEST_HANDLE_PARAM. */
+#define guest_handle_cast(hnd, type) ({         \
+    type *_x = (hnd).p;                         \
+    (XEN_GUEST_HANDLE_PARAM(type)) { _x };            \
+})
+
+/* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
+#define guest_handle_to_param(hnd, type) ({                  \
+    typeof((hnd).p) _x = (hnd).p;                            \
+    XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
+    /* type checking: make sure that the pointers inside     \
+     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
+     * the same type, then return hnd */                     \
+    (void)(&_x == &_y.p);                                    \
+    _y;                                                      \
+})
+
+#define guest_handle_for_field(hnd, type, fld)          \
+    ((XEN_GUEST_HANDLE(type)) { &(hnd).p->fld })
+
+#define guest_handle_from_ptr(ptr, type)        \
+    ((XEN_GUEST_HANDLE_PARAM(type)) { (type *)ptr })
+#define const_guest_handle_from_ptr(ptr, type)  \
+    ((XEN_GUEST_HANDLE_PARAM(const_##type)) { (const type *)ptr })
+
+/*
+ * Copy an array of objects to guest context via a guest handle,
+ * specifying an offset into the guest array.
+ */
+#define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
+    const typeof(*(ptr)) *_s = (ptr);                   \
+    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
+    /* Check that the handle is not for a const type */ \
+    void *__maybe_unused _t = (hnd).p;                  \
+    (void)((hnd).p == _s);                              \
+    raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
+})
+
+/*
+ * Clear an array of objects in guest context via a guest handle,
+ * specifying an offset into the guest array.
+ */
+#define clear_guest_offset(hnd, off, nr) ({    \
+    void *_d = (hnd).p;                        \
+    raw_clear_guest(_d+(off), nr);             \
+})
+
+/*
+ * Copy an array of objects from guest context via a guest handle,
+ * specifying an offset into the guest array.
+ */
+#define copy_from_guest_offset(ptr, hnd, off, nr) ({    \
+    const typeof(*(ptr)) *_s = (hnd).p;                 \
+    typeof(*(ptr)) *_d = (ptr);                         \
+    raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
+})
+
+/* Copy sub-field of a structure to guest context via a guest handle. */
+#define copy_field_to_guest(hnd, ptr, field) ({         \
+    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
+    void *_d = &(hnd).p->field;                         \
+    (void)(&(hnd).p->field == _s);                      \
+    raw_copy_to_guest(_d, _s, sizeof(*_s));             \
+})
+
+/* Copy sub-field of a structure from guest context via a guest handle. */
+#define copy_field_from_guest(ptr, hnd, field) ({       \
+    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
+    typeof(&(ptr)->field) _d = &(ptr)->field;           \
+    raw_copy_from_guest(_d, _s, sizeof(*_d));           \
+})
+
 #define copy_to_guest(hnd, ptr, nr)                     \
     copy_to_guest_offset(hnd, 0, ptr, nr)
 
@@ -20,6 +100,45 @@
 #define clear_guest(hnd, nr)                            \
     clear_guest_offset(hnd, 0, nr)
 
+/*
+ * The __copy_* functions should only be used after the guest handle has
+ * been pre-validated via guest_handle_okay() and
+ * guest_handle_subrange_okay().
+ */
+
+#define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
+    const typeof(*(ptr)) *_s = (ptr);                   \
+    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
+    /* Check that the handle is not for a const type */ \
+    void *__maybe_unused _t = (hnd).p;                  \
+    (void)((hnd).p == _s);                              \
+    __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
+})
+
+#define __clear_guest_offset(hnd, off, nr) ({    \
+    void *_d = (hnd).p;                          \
+    __raw_clear_guest(_d + (off), nr);           \
+})
+
+#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
+    const typeof(*(ptr)) *_s = (hnd).p;                 \
+    typeof(*(ptr)) *_d = (ptr);                         \
+    __raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
+})
+
+#define __copy_field_to_guest(hnd, ptr, field) ({       \
+    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
+    void *_d = &(hnd).p->field;                         \
+    (void)(&(hnd).p->field == _s);                      \
+    __raw_copy_to_guest(_d, _s, sizeof(*_s));           \
+})
+
+#define __copy_field_from_guest(ptr, hnd, field) ({     \
+    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
+    typeof(&(ptr)->field) _d = &(ptr)->field;           \
+    __raw_copy_from_guest(_d, _s, sizeof(*_d));         \
+})
+
 #define __copy_to_guest(hnd, ptr, nr)                   \
     __copy_to_guest_offset(hnd, 0, ptr, nr)
 
-- 
2.17.1

Re: [RESEND][PATCH v2 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Stefano Stabellini]

On Thu, 30 Jul 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Most of the helpers to access guest memory are implemented the same way
> on Arm and x86. The only differences are:
>     - guest_handle_{from, to}_param(): while on x86 XEN_GUEST_HANDLE()

It is actually just guest_handle_to_param() ?


>       and XEN_GUEST_HANDLE_PARAM() are the same, they are not on Arm. It
>       is still fine to use the Arm implementation on x86.
>     - __clear_guest_offset(): Interestingly the prototype does not match
>       between the x86 and Arm. However, the Arm one is bogus. So the x86
>       implementation can be used.
>     - guest_handle{,_subrange}_okay(): They are validly differing
>       because Arm is only supporting auto-translated guest and therefore
>       handles are always valid.
> 
> In the past, the ia64 and ppc64 port use a different model to access
> guest parameter. They have been long gone now.
> 
> Given Xen currently only support 2 archictures, it is too soon to have a
> directory asm-generic as it is not possible to differentiate it with the
> existing directory xen/. If/When there is a 3rd port, we can decide to
> create the new directory if that new port decide to use a different way
> to access guest parameter.
> 
> For now, consolidate it in xen/guest_access.h.
> 
> While it would be possible to adjust the coding style at the same, this
> is left for a follow-up patch so 'diff' can be used to check the
> consolidation was done correctly.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Looks good to me

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Expand the commit message explaining why asm-generic is not
>         created.
> ---
>  xen/include/asm-arm/guest_access.h | 114 ---------------------------
>  xen/include/asm-x86/guest_access.h | 108 --------------------------
>  xen/include/xen/guest_access.h     | 119 +++++++++++++++++++++++++++++
>  3 files changed, 119 insertions(+), 222 deletions(-)
> 
> diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h
> index b9a89c495527..53766386d3d8 100644
> --- a/xen/include/asm-arm/guest_access.h
> +++ b/xen/include/asm-arm/guest_access.h
> @@ -23,88 +23,6 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf,
>  #define __raw_copy_from_guest raw_copy_from_guest
>  #define __raw_clear_guest raw_clear_guest
>  
> -/* Remainder copied from x86 -- could be common? */
> -
> -/* Is the guest handle a NULL reference? */
> -#define guest_handle_is_null(hnd)        ((hnd).p == NULL)
> -
> -/* Offset the given guest handle into the array it refers to. */
> -#define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
> -#define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
> -
> -/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
> - * to the specified type of XEN_GUEST_HANDLE_PARAM. */
> -#define guest_handle_cast(hnd, type) ({         \
> -    type *_x = (hnd).p;                         \
> -    (XEN_GUEST_HANDLE_PARAM(type)) { _x };            \
> -})
> -
> -/* Convert a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
> -#define guest_handle_to_param(hnd, type) ({                  \
> -    typeof((hnd).p) _x = (hnd).p;                            \
> -    XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
> -    /* type checking: make sure that the pointers inside     \
> -     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
> -     * the same type, then return hnd */                     \
> -    (void)(&_x == &_y.p);                                    \
> -    _y;                                                      \
> -})
> -
> -#define guest_handle_for_field(hnd, type, fld)          \
> -    ((XEN_GUEST_HANDLE(type)) { &(hnd).p->fld })
> -
> -#define guest_handle_from_ptr(ptr, type)        \
> -    ((XEN_GUEST_HANDLE_PARAM(type)) { (type *)ptr })
> -#define const_guest_handle_from_ptr(ptr, type)  \
> -    ((XEN_GUEST_HANDLE_PARAM(const_##type)) { (const type *)ptr })
> -
> -/*
> - * Copy an array of objects to guest context via a guest handle,
> - * specifying an offset into the guest array.
> - */
> -#define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
> -    const typeof(*(ptr)) *_s = (ptr);                   \
> -    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> -    /* Check that the handle is not for a const type */ \
> -    void *__maybe_unused _t = (hnd).p;                  \
> -    (void)((hnd).p == _s);                              \
> -    raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
> -})
> -
> -/*
> - * Clear an array of objects in guest context via a guest handle,
> - * specifying an offset into the guest array.
> - */
> -#define clear_guest_offset(hnd, off, nr) ({    \
> -    void *_d = (hnd).p;                        \
> -    raw_clear_guest(_d+(off), nr);             \
> -})
> -
> -/*
> - * Copy an array of objects from guest context via a guest handle,
> - * specifying an offset into the guest array.
> - */
> -#define copy_from_guest_offset(ptr, hnd, off, nr) ({    \
> -    const typeof(*(ptr)) *_s = (hnd).p;                 \
> -    typeof(*(ptr)) *_d = (ptr);                         \
> -    raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
> -})
> -
> -/* Copy sub-field of a structure to guest context via a guest handle. */
> -#define copy_field_to_guest(hnd, ptr, field) ({         \
> -    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
> -    void *_d = &(hnd).p->field;                         \
> -    (void)(&(hnd).p->field == _s);                      \
> -    raw_copy_to_guest(_d, _s, sizeof(*_s));             \
> -})
> -
> -/* Copy sub-field of a structure from guest context via a guest handle. */
> -#define copy_field_from_guest(ptr, hnd, field) ({       \
> -    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
> -    typeof(&(ptr)->field) _d = &(ptr)->field;           \
> -    raw_copy_from_guest(_d, _s, sizeof(*_d));           \
> -})
> -
>  /*
>   * Pre-validate a guest handle.
>   * Allows use of faster __copy_* functions.
> @@ -113,38 +31,6 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf,
>  #define guest_handle_okay(hnd, nr) (1)
>  #define guest_handle_subrange_okay(hnd, first, last) (1)
>  
> -#define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
> -    const typeof(*(ptr)) *_s = (ptr);                   \
> -    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> -    /* Check that the handle is not for a const type */ \
> -    void *__maybe_unused _t = (hnd).p;                  \
> -    (void)((hnd).p == _s);                              \
> -    __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
> -})
> -
> -#define __clear_guest_offset(hnd, off, ptr, nr) ({      \
> -    __raw_clear_guest(_d+(off), nr);  \
> -})
> -
> -#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
> -    const typeof(*(ptr)) *_s = (hnd).p;                 \
> -    typeof(*(ptr)) *_d = (ptr);                         \
> -    __raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
> -})
> -
> -#define __copy_field_to_guest(hnd, ptr, field) ({       \
> -    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
> -    void *_d = &(hnd).p->field;                         \
> -    (void)(&(hnd).p->field == _s);                      \
> -    __raw_copy_to_guest(_d, _s, sizeof(*_s));           \
> -})
> -
> -#define __copy_field_from_guest(ptr, hnd, field) ({     \
> -    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
> -    typeof(&(ptr)->field) _d = &(ptr)->field;           \
> -    __raw_copy_from_guest(_d, _s, sizeof(*_d));         \
> -})
> -
>  #endif /* __ASM_ARM_GUEST_ACCESS_H__ */
>  /*
>   * Local variables:
> diff --git a/xen/include/asm-x86/guest_access.h b/xen/include/asm-x86/guest_access.h
> index 3ffde205f6a1..08c9fbbc78e1 100644
> --- a/xen/include/asm-x86/guest_access.h
> +++ b/xen/include/asm-x86/guest_access.h
> @@ -38,81 +38,6 @@
>       clear_user_hvm((dst), (len)) :             \
>       clear_user((dst), (len)))
>  
> -/* Is the guest handle a NULL reference? */
> -#define guest_handle_is_null(hnd)        ((hnd).p == NULL)
> -
> -/* Offset the given guest handle into the array it refers to. */
> -#define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
> -#define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
> -
> -/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
> - * to the specified type of XEN_GUEST_HANDLE_PARAM. */
> -#define guest_handle_cast(hnd, type) ({         \
> -    type *_x = (hnd).p;                         \
> -    (XEN_GUEST_HANDLE_PARAM(type)) { _x };            \
> -})
> -
> -/* Convert a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
> -#define guest_handle_to_param(hnd, type) ({                  \
> -    /* type checking: make sure that the pointers inside     \
> -     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
> -     * the same type, then return hnd */                     \
> -    (void)((typeof(&(hnd).p)) 0 ==                           \
> -        (typeof(&((XEN_GUEST_HANDLE_PARAM(type)) {}).p)) 0); \
> -    (hnd);                                                   \
> -})
> -
> -#define guest_handle_for_field(hnd, type, fld)          \
> -    ((XEN_GUEST_HANDLE(type)) { &(hnd).p->fld })
> -
> -#define guest_handle_from_ptr(ptr, type)        \
> -    ((XEN_GUEST_HANDLE_PARAM(type)) { (type *)ptr })
> -#define const_guest_handle_from_ptr(ptr, type)  \
> -    ((XEN_GUEST_HANDLE_PARAM(const_##type)) { (const type *)ptr })
> -
> -/*
> - * Copy an array of objects to guest context via a guest handle,
> - * specifying an offset into the guest array.
> - */
> -#define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
> -    const typeof(*(ptr)) *_s = (ptr);                   \
> -    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> -    /* Check that the handle is not for a const type */ \
> -    void *__maybe_unused _t = (hnd).p;                  \
> -    (void)((hnd).p == _s);                              \
> -    raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
> -})
> -
> -/*
> - * Copy an array of objects from guest context via a guest handle,
> - * specifying an offset into the guest array.
> - */
> -#define copy_from_guest_offset(ptr, hnd, off, nr) ({    \
> -    const typeof(*(ptr)) *_s = (hnd).p;                 \
> -    typeof(*(ptr)) *_d = (ptr);                         \
> -    raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
> -})
> -
> -#define clear_guest_offset(hnd, off, nr) ({    \
> -    void *_d = (hnd).p;                        \
> -    raw_clear_guest(_d+(off), nr);             \
> -})
> -
> -/* Copy sub-field of a structure to guest context via a guest handle. */
> -#define copy_field_to_guest(hnd, ptr, field) ({         \
> -    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
> -    void *_d = &(hnd).p->field;                         \
> -    (void)(&(hnd).p->field == _s);                      \
> -    raw_copy_to_guest(_d, _s, sizeof(*_s));             \
> -})
> -
> -/* Copy sub-field of a structure from guest context via a guest handle. */
> -#define copy_field_from_guest(ptr, hnd, field) ({       \
> -    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
> -    typeof(&(ptr)->field) _d = &(ptr)->field;           \
> -    raw_copy_from_guest(_d, _s, sizeof(*_d));           \
> -})
> -
>  /*
>   * Pre-validate a guest handle.
>   * Allows use of faster __copy_* functions.
> @@ -126,39 +51,6 @@
>                       (last)-(first)+1,                  \
>                       sizeof(*(hnd).p)))
>  
> -#define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
> -    const typeof(*(ptr)) *_s = (ptr);                   \
> -    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> -    /* Check that the handle is not for a const type */ \
> -    void *__maybe_unused _t = (hnd).p;                  \
> -    (void)((hnd).p == _s);                              \
> -    __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
> -})
> -
> -#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
> -    const typeof(*(ptr)) *_s = (hnd).p;                 \
> -    typeof(*(ptr)) *_d = (ptr);                         \
> -    __raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
> -})
> -
> -#define __clear_guest_offset(hnd, off, nr) ({    \
> -    void *_d = (hnd).p;                          \
> -    __raw_clear_guest(_d+(off), nr);             \
> -})
> -
> -#define __copy_field_to_guest(hnd, ptr, field) ({       \
> -    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
> -    void *_d = &(hnd).p->field;                         \
> -    (void)(&(hnd).p->field == _s);                      \
> -    __raw_copy_to_guest(_d, _s, sizeof(*_s));           \
> -})
> -
> -#define __copy_field_from_guest(ptr, hnd, field) ({     \
> -    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
> -    typeof(&(ptr)->field) _d = &(ptr)->field;           \
> -    __raw_copy_from_guest(_d, _s, sizeof(*_d));         \
> -})
> -
>  #endif /* __ASM_X86_GUEST_ACCESS_H__ */
>  /*
>   * Local variables:
> diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
> index ef9aaa3efcfe..4957b8d1f2b8 100644
> --- a/xen/include/xen/guest_access.h
> +++ b/xen/include/xen/guest_access.h
> @@ -11,6 +11,86 @@
>  #include <xen/types.h>
>  #include <public/xen.h>
>  
> +/* Is the guest handle a NULL reference? */
> +#define guest_handle_is_null(hnd)        ((hnd).p == NULL)
> +
> +/* Offset the given guest handle into the array it refers to. */
> +#define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
> +#define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
> +
> +/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
> + * to the specified type of XEN_GUEST_HANDLE_PARAM. */
> +#define guest_handle_cast(hnd, type) ({         \
> +    type *_x = (hnd).p;                         \
> +    (XEN_GUEST_HANDLE_PARAM(type)) { _x };            \
> +})
> +
> +/* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
> +#define guest_handle_to_param(hnd, type) ({                  \
> +    typeof((hnd).p) _x = (hnd).p;                            \
> +    XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
> +    /* type checking: make sure that the pointers inside     \
> +     * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
> +     * the same type, then return hnd */                     \
> +    (void)(&_x == &_y.p);                                    \
> +    _y;                                                      \
> +})
> +
> +#define guest_handle_for_field(hnd, type, fld)          \
> +    ((XEN_GUEST_HANDLE(type)) { &(hnd).p->fld })
> +
> +#define guest_handle_from_ptr(ptr, type)        \
> +    ((XEN_GUEST_HANDLE_PARAM(type)) { (type *)ptr })
> +#define const_guest_handle_from_ptr(ptr, type)  \
> +    ((XEN_GUEST_HANDLE_PARAM(const_##type)) { (const type *)ptr })
> +
> +/*
> + * Copy an array of objects to guest context via a guest handle,
> + * specifying an offset into the guest array.
> + */
> +#define copy_to_guest_offset(hnd, off, ptr, nr) ({      \
> +    const typeof(*(ptr)) *_s = (ptr);                   \
> +    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> +    /* Check that the handle is not for a const type */ \
> +    void *__maybe_unused _t = (hnd).p;                  \
> +    (void)((hnd).p == _s);                              \
> +    raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
> +})
> +
> +/*
> + * Clear an array of objects in guest context via a guest handle,
> + * specifying an offset into the guest array.
> + */
> +#define clear_guest_offset(hnd, off, nr) ({    \
> +    void *_d = (hnd).p;                        \
> +    raw_clear_guest(_d+(off), nr);             \
> +})
> +
> +/*
> + * Copy an array of objects from guest context via a guest handle,
> + * specifying an offset into the guest array.
> + */
> +#define copy_from_guest_offset(ptr, hnd, off, nr) ({    \
> +    const typeof(*(ptr)) *_s = (hnd).p;                 \
> +    typeof(*(ptr)) *_d = (ptr);                         \
> +    raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
> +})
> +
> +/* Copy sub-field of a structure to guest context via a guest handle. */
> +#define copy_field_to_guest(hnd, ptr, field) ({         \
> +    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
> +    void *_d = &(hnd).p->field;                         \
> +    (void)(&(hnd).p->field == _s);                      \
> +    raw_copy_to_guest(_d, _s, sizeof(*_s));             \
> +})
> +
> +/* Copy sub-field of a structure from guest context via a guest handle. */
> +#define copy_field_from_guest(ptr, hnd, field) ({       \
> +    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
> +    typeof(&(ptr)->field) _d = &(ptr)->field;           \
> +    raw_copy_from_guest(_d, _s, sizeof(*_d));           \
> +})
> +
>  #define copy_to_guest(hnd, ptr, nr)                     \
>      copy_to_guest_offset(hnd, 0, ptr, nr)
>  
> @@ -20,6 +100,45 @@
>  #define clear_guest(hnd, nr)                            \
>      clear_guest_offset(hnd, 0, nr)
>  
> +/*
> + * The __copy_* functions should only be used after the guest handle has
> + * been pre-validated via guest_handle_okay() and
> + * guest_handle_subrange_okay().
> + */
> +
> +#define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
> +    const typeof(*(ptr)) *_s = (ptr);                   \
> +    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> +    /* Check that the handle is not for a const type */ \
> +    void *__maybe_unused _t = (hnd).p;                  \
> +    (void)((hnd).p == _s);                              \
> +    __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
> +})
> +
> +#define __clear_guest_offset(hnd, off, nr) ({    \
> +    void *_d = (hnd).p;                          \
> +    __raw_clear_guest(_d + (off), nr);           \
> +})
> +
> +#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
> +    const typeof(*(ptr)) *_s = (hnd).p;                 \
> +    typeof(*(ptr)) *_d = (ptr);                         \
> +    __raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
> +})
> +
> +#define __copy_field_to_guest(hnd, ptr, field) ({       \
> +    const typeof(&(ptr)->field) _s = &(ptr)->field;     \
> +    void *_d = &(hnd).p->field;                         \
> +    (void)(&(hnd).p->field == _s);                      \
> +    __raw_copy_to_guest(_d, _s, sizeof(*_s));           \
> +})
> +
> +#define __copy_field_from_guest(ptr, hnd, field) ({     \
> +    const typeof(&(ptr)->field) _s = &(hnd).p->field;   \
> +    typeof(&(ptr)->field) _d = &(ptr)->field;           \
> +    __raw_copy_from_guest(_d, _s, sizeof(*_d));         \
> +})
> +
>  #define __copy_to_guest(hnd, ptr, nr)                   \
>      __copy_to_guest_offset(hnd, 0, ptr, nr)
>  
> -- 
> 2.17.1
> 

Re: [RESEND][PATCH v2 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Julien Grall]

Hi Stefano,

On 30/07/2020 20:37, Stefano Stabellini wrote:
> On Thu, 30 Jul 2020, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Most of the helpers to access guest memory are implemented the same way
>> on Arm and x86. The only differences are:
>>      - guest_handle_{from, to}_param(): while on x86 XEN_GUEST_HANDLE()
> 
> It is actually just guest_handle_to_param() ?

Yes. I forgot Jan recently removed the helper in commit dd5520f9df05 
"x86: adjustments to guest handle treatment". I will update the commit 
message.

> 
> 
>>        and XEN_GUEST_HANDLE_PARAM() are the same, they are not on Arm. It
>>        is still fine to use the Arm implementation on x86.
>>      - __clear_guest_offset(): Interestingly the prototype does not match
>>        between the x86 and Arm. However, the Arm one is bogus. So the x86
>>        implementation can be used.
>>      - guest_handle{,_subrange}_okay(): They are validly differing
>>        because Arm is only supporting auto-translated guest and therefore
>>        handles are always valid.
>>
>> In the past, the ia64 and ppc64 port use a different model to access
>> guest parameter. They have been long gone now.
>>
>> Given Xen currently only support 2 archictures, it is too soon to have a
>> directory asm-generic as it is not possible to differentiate it with the
>> existing directory xen/. If/When there is a 3rd port, we can decide to
>> create the new directory if that new port decide to use a different way
>> to access guest parameter.
>>
>> For now, consolidate it in xen/guest_access.h.
>>
>> While it would be possible to adjust the coding style at the same, this
>> is left for a follow-up patch so 'diff' can be used to check the
>> consolidation was done correctly.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Looks good to me
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Thank you!

Cheers,

-- 
Julien Grall

Re: [RESEND][PATCH v2 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Jan Beulich]

On 30.07.2020 20:18, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Most of the helpers to access guest memory are implemented the same way
> on Arm and x86. The only differences are:
>     - guest_handle_{from, to}_param(): while on x86 XEN_GUEST_HANDLE()
>       and XEN_GUEST_HANDLE_PARAM() are the same, they are not on Arm. It
>       is still fine to use the Arm implementation on x86.

Is the description stale? I don't think there's any guest_handle_from_param()
anymore.

>     - __clear_guest_offset(): Interestingly the prototype does not match
>       between the x86 and Arm. However, the Arm one is bogus. So the x86
>       implementation can be used.
>     - guest_handle{,_subrange}_okay(): They are validly differing
>       because Arm is only supporting auto-translated guest and therefore
>       handles are always valid.
> 
> In the past, the ia64 and ppc64 port use a different model to access
> guest parameter. They have been long gone now.
> 
> Given Xen currently only support 2 archictures, it is too soon to have a
> directory asm-generic as it is not possible to differentiate it with the
> existing directory xen/. If/When there is a 3rd port, we can decide to
> create the new directory if that new port decide to use a different way
> to access guest parameter.
> 
> For now, consolidate it in xen/guest_access.h.
> 
> While it would be possible to adjust the coding style at the same, this
> is left for a follow-up patch so 'diff' can be used to check the
> consolidation was done correctly.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Apart from the above
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [RESEND][PATCH v2 6/7] xen/guest_access: Consolidate guest access helpers in xen/guest_access.h

[Julien Grall]

Hi Jan,

On 31/07/2020 12:45, Jan Beulich wrote:
> On 30.07.2020 20:18, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Most of the helpers to access guest memory are implemented the same way
>> on Arm and x86. The only differences are:
>>      - guest_handle_{from, to}_param(): while on x86 XEN_GUEST_HANDLE()
>>        and XEN_GUEST_HANDLE_PARAM() are the same, they are not on Arm. It
>>        is still fine to use the Arm implementation on x86.
> 
> Is the description stale? I don't think there's any guest_handle_from_param()
> anymore.

Yes, I forgot you removed it. I will update the commit message.

> 
>>      - __clear_guest_offset(): Interestingly the prototype does not match
>>        between the x86 and Arm. However, the Arm one is bogus. So the x86
>>        implementation can be used.
>>      - guest_handle{,_subrange}_okay(): They are validly differing
>>        because Arm is only supporting auto-translated guest and therefore
>>        handles are always valid.
>>
>> In the past, the ia64 and ppc64 port use a different model to access
>> guest parameter. They have been long gone now.
>>
>> Given Xen currently only support 2 archictures, it is too soon to have a
>> directory asm-generic as it is not possible to differentiate it with the
>> existing directory xen/. If/When there is a 3rd port, we can decide to
>> create the new directory if that new port decide to use a different way
>> to access guest parameter.
>>
>> For now, consolidate it in xen/guest_access.h.
>>
>> While it would be possible to adjust the coding style at the same, this
>> is left for a follow-up patch so 'diff' can be used to check the
>> consolidation was done correctly.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Apart from the above
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks!

Cheers,

-- 
Julien Grall

[RESEND][PATCH v2 7/7] xen/guest_access: Fix coding style in xen/guest_access.h

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

    * Add space before and after operator
    * Align \
    * Format comments

No functional changes expected.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/xen/guest_access.h | 36 +++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
index 4957b8d1f2b8..52fc7a063249 100644
--- a/xen/include/xen/guest_access.h
+++ b/xen/include/xen/guest_access.h
@@ -18,20 +18,24 @@
 #define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
 #define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
 
-/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
- * to the specified type of XEN_GUEST_HANDLE_PARAM. */
+/*
+ * Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
+ * to the specified type of XEN_GUEST_HANDLE_PARAM.
+ */
 #define guest_handle_cast(hnd, type) ({         \
     type *_x = (hnd).p;                         \
-    (XEN_GUEST_HANDLE_PARAM(type)) { _x };            \
+    (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
 })
 
 /* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
 #define guest_handle_to_param(hnd, type) ({                  \
     typeof((hnd).p) _x = (hnd).p;                            \
     XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
-    /* type checking: make sure that the pointers inside     \
+    /*                                                       \
+     * type checking: make sure that the pointers inside     \
      * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
-     * the same type, then return hnd */                     \
+     * the same type, then return hnd.                       \
+     */                                                      \
     (void)(&_x == &_y.p);                                    \
     _y;                                                      \
 })
@@ -106,13 +110,13 @@
  * guest_handle_subrange_okay().
  */
 
-#define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
-    const typeof(*(ptr)) *_s = (ptr);                   \
-    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
-    /* Check that the handle is not for a const type */ \
-    void *__maybe_unused _t = (hnd).p;                  \
-    (void)((hnd).p == _s);                              \
-    __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
+#define __copy_to_guest_offset(hnd, off, ptr, nr) ({        \
+    const typeof(*(ptr)) *_s = (ptr);                       \
+    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;              \
+    /* Check that the handle is not for a const type */     \
+    void *__maybe_unused _t = (hnd).p;                      \
+    (void)((hnd).p == _s);                                  \
+    __raw_copy_to_guest(_d + (off), _s, sizeof(*_s) * (nr));\
 })
 
 #define __clear_guest_offset(hnd, off, nr) ({    \
@@ -120,10 +124,10 @@
     __raw_clear_guest(_d + (off), nr);           \
 })
 
-#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
-    const typeof(*(ptr)) *_s = (hnd).p;                 \
-    typeof(*(ptr)) *_d = (ptr);                         \
-    __raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
+#define __copy_from_guest_offset(ptr, hnd, off, nr) ({          \
+    const typeof(*(ptr)) *_s = (hnd).p;                         \
+    typeof(*(ptr)) *_d = (ptr);                                 \
+    __raw_copy_from_guest(_d, _s + (off), sizeof (*_d) * (nr)); \
 })
 
 #define __copy_field_to_guest(hnd, ptr, field) ({       \
-- 
2.17.1

Re: [RESEND][PATCH v2 7/7] xen/guest_access: Fix coding style in xen/guest_access.h

[Stefano Stabellini]

On Thu, 30 Jul 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
>     * Add space before and after operator
>     * Align \
>     * Format comments
> 
> No functional changes expected.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/include/xen/guest_access.h | 36 +++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
> index 4957b8d1f2b8..52fc7a063249 100644
> --- a/xen/include/xen/guest_access.h
> +++ b/xen/include/xen/guest_access.h
> @@ -18,20 +18,24 @@
>  #define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
>  #define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
>  
> -/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
> - * to the specified type of XEN_GUEST_HANDLE_PARAM. */
> +/*
> + * Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
> + * to the specified type of XEN_GUEST_HANDLE_PARAM.
> + */
>  #define guest_handle_cast(hnd, type) ({         \
>      type *_x = (hnd).p;                         \
> -    (XEN_GUEST_HANDLE_PARAM(type)) { _x };            \
> +    (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
>  })
>  
>  /* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
>  #define guest_handle_to_param(hnd, type) ({                  \
>      typeof((hnd).p) _x = (hnd).p;                            \
>      XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
> -    /* type checking: make sure that the pointers inside     \
> +    /*                                                       \
> +     * type checking: make sure that the pointers inside     \
>       * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
> -     * the same type, then return hnd */                     \
> +     * the same type, then return hnd.                       \
> +     */                                                      \
>      (void)(&_x == &_y.p);                                    \
>      _y;                                                      \
>  })
> @@ -106,13 +110,13 @@
>   * guest_handle_subrange_okay().
>   */
>  
> -#define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
> -    const typeof(*(ptr)) *_s = (ptr);                   \
> -    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> -    /* Check that the handle is not for a const type */ \
> -    void *__maybe_unused _t = (hnd).p;                  \
> -    (void)((hnd).p == _s);                              \
> -    __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
> +#define __copy_to_guest_offset(hnd, off, ptr, nr) ({        \
> +    const typeof(*(ptr)) *_s = (ptr);                       \
> +    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;              \
> +    /* Check that the handle is not for a const type */     \
> +    void *__maybe_unused _t = (hnd).p;                      \
> +    (void)((hnd).p == _s);                                  \
> +    __raw_copy_to_guest(_d + (off), _s, sizeof(*_s) * (nr));\
>  })
>  
>  #define __clear_guest_offset(hnd, off, nr) ({    \
> @@ -120,10 +124,10 @@
>      __raw_clear_guest(_d + (off), nr);           \
>  })
>  
> -#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
> -    const typeof(*(ptr)) *_s = (hnd).p;                 \
> -    typeof(*(ptr)) *_d = (ptr);                         \
> -    __raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
> +#define __copy_from_guest_offset(ptr, hnd, off, nr) ({          \
> +    const typeof(*(ptr)) *_s = (hnd).p;                         \
> +    typeof(*(ptr)) *_d = (ptr);                                 \
> +    __raw_copy_from_guest(_d, _s + (off), sizeof (*_d) * (nr)); \
>  })
>  
>  #define __copy_field_to_guest(hnd, ptr, field) ({       \
> -- 
> 2.17.1
> 

Re: [RESEND][PATCH v2 7/7] xen/guest_access: Fix coding style in xen/guest_access.h

[Jan Beulich]

On 30.07.2020 20:18, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
>     * Add space before and after operator
>     * Align \
>     * Format comments

How about also

    * remove/replace leading underscores

?

Jan

Re: [RESEND][PATCH v2 7/7] xen/guest_access: Fix coding style in xen/guest_access.h

[Julien Grall]

Hi Jan,

On 31/07/2020 12:41, Jan Beulich wrote:
> On 30.07.2020 20:18, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>>      * Add space before and after operator
>>      * Align \
>>      * Format comments
> 
> How about also
> 
>      * remove/replace leading underscores
> 
> ?

I don't have any plan for this. You are welcome to send a patch for this.

Cheers,

-- 
Julien Grall

Re: [RESEND][PATCH v2 7/7] xen/guest_access: Fix coding style in xen/guest_access.h

[Jan Beulich]

On 14.08.2020 21:18, Julien Grall wrote:
> On 31/07/2020 12:41, Jan Beulich wrote:
>> On 30.07.2020 20:18, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>>      * Add space before and after operator
>>>      * Align \
>>>      * Format comments
>>
>> How about also
>>
>>      * remove/replace leading underscores
>>
>> ?
> 
> I don't have any plan for this. You are welcome to send a patch for this.

In which case may I ask that you replace "Fix" by "Improve" or some
such in the title?

Jan

Re: [RESEND][PATCH v2 7/7] xen/guest_access: Fix coding style in xen/guest_access.h

[Julien Grall]

Hi Jan,

On 18/08/2020 09:52, Jan Beulich wrote:
> On 14.08.2020 21:18, Julien Grall wrote:
>> On 31/07/2020 12:41, Jan Beulich wrote:
>>> On 30.07.2020 20:18, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>>       * Add space before and after operator
>>>>       * Align \
>>>>       * Format comments
>>>
>>> How about also
>>>
>>>       * remove/replace leading underscores
>>>
>>> ?
>>
>> I don't have any plan for this. You are welcome to send a patch for this.
> 
> In which case may I ask that you replace "Fix" by "Improve" or some
> such in the title?

I will do it if. Although, this doesn't mean I agree with your implicit 
coding style.

Cheers,

-- 
Julien Grall

Re: [RESEND][PATCH v2 7/7] xen/guest_access: Fix coding style in xen/guest_access.h

[Bertrand Marquis]

> On 30 Jul 2020, at 20:18, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
>    * Add space before and after operator
>    * Align \
>    * Format comments
> 
> No functional changes expected.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

> ---
> xen/include/xen/guest_access.h | 36 +++++++++++++++++++---------------
> 1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
> index 4957b8d1f2b8..52fc7a063249 100644
> --- a/xen/include/xen/guest_access.h
> +++ b/xen/include/xen/guest_access.h
> @@ -18,20 +18,24 @@
> #define guest_handle_add_offset(hnd, nr) ((hnd).p += (nr))
> #define guest_handle_subtract_offset(hnd, nr) ((hnd).p -= (nr))
> 
> -/* Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
> - * to the specified type of XEN_GUEST_HANDLE_PARAM. */
> +/*
> + * Cast a guest handle (either XEN_GUEST_HANDLE or XEN_GUEST_HANDLE_PARAM)
> + * to the specified type of XEN_GUEST_HANDLE_PARAM.
> + */
> #define guest_handle_cast(hnd, type) ({         \
>     type *_x = (hnd).p;                         \
> -    (XEN_GUEST_HANDLE_PARAM(type)) { _x };            \
> +    (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
> })
> 
> /* Cast a XEN_GUEST_HANDLE to XEN_GUEST_HANDLE_PARAM */
> #define guest_handle_to_param(hnd, type) ({                  \
>     typeof((hnd).p) _x = (hnd).p;                            \
>     XEN_GUEST_HANDLE_PARAM(type) _y = { _x };                \
> -    /* type checking: make sure that the pointers inside     \
> +    /*                                                       \
> +     * type checking: make sure that the pointers inside     \
>      * XEN_GUEST_HANDLE and XEN_GUEST_HANDLE_PARAM are of    \
> -     * the same type, then return hnd */                     \
> +     * the same type, then return hnd.                       \
> +     */                                                      \
>     (void)(&_x == &_y.p);                                    \
>     _y;                                                      \
> })
> @@ -106,13 +110,13 @@
>  * guest_handle_subrange_okay().
>  */
> 
> -#define __copy_to_guest_offset(hnd, off, ptr, nr) ({    \
> -    const typeof(*(ptr)) *_s = (ptr);                   \
> -    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
> -    /* Check that the handle is not for a const type */ \
> -    void *__maybe_unused _t = (hnd).p;                  \
> -    (void)((hnd).p == _s);                              \
> -    __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
> +#define __copy_to_guest_offset(hnd, off, ptr, nr) ({        \
> +    const typeof(*(ptr)) *_s = (ptr);                       \
> +    char (*_d)[sizeof(*_s)] = (void *)(hnd).p;              \
> +    /* Check that the handle is not for a const type */     \
> +    void *__maybe_unused _t = (hnd).p;                      \
> +    (void)((hnd).p == _s);                                  \
> +    __raw_copy_to_guest(_d + (off), _s, sizeof(*_s) * (nr));\
> })
> 
> #define __clear_guest_offset(hnd, off, nr) ({    \
> @@ -120,10 +124,10 @@
>     __raw_clear_guest(_d + (off), nr);           \
> })
> 
> -#define __copy_from_guest_offset(ptr, hnd, off, nr) ({  \
> -    const typeof(*(ptr)) *_s = (hnd).p;                 \
> -    typeof(*(ptr)) *_d = (ptr);                         \
> -    __raw_copy_from_guest(_d, _s+(off), sizeof(*_d)*(nr));\
> +#define __copy_from_guest_offset(ptr, hnd, off, nr) ({          \
> +    const typeof(*(ptr)) *_s = (hnd).p;                         \
> +    typeof(*(ptr)) *_d = (ptr);                                 \
> +    __raw_copy_from_guest(_d, _s + (off), sizeof (*_d) * (nr)); \
> })
> 
> #define __copy_field_to_guest(hnd, ptr, field) ({       \
> -- 
> 2.17.1
> 
>