[PATCH] pvcalls: Document correctly and explicitely the padding for all arches

Julien Grall posted 1 patch 22 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20200516101953.1235-1-julien@xen.org
Maintainers: Andrew Cooper <andrew.cooper3@citrix.com>, Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>, Ian Jackson <ian.jackson@eu.citrix.com>, Juergen Gross <jgross@suse.com>, George Dunlap <george.dunlap@citrix.com>, Julien Grall <julien@xen.org>, Jan Beulich <jbeulich@suse.com>
docs/misc/pvcalls.pandoc        | 18 ++++++++++--------
xen/include/public/io/pvcalls.h | 14 ++++++++++++++
2 files changed, 24 insertions(+), 8 deletions(-)

[PATCH] pvcalls: Document correctly and explicitely the padding for all arches

Posted by Julien Grall 22 weeks ago
From: Julien Grall <jgrall@amazon.com>

The documentation of pvcalls suggests there is padding for 32-bit x86
at the end of most the structure. However, they are not described in
in the public header.

Because of that all the structures would be 32-bit aligned and not
64-bit aligned for 32-bit x86.

For all the other architectures supported (Arm and 64-bit x86), the
structure are aligned to 64-bit because they contain uint64_t field.
Therefore all the structures contain implicit padding.

The paddings are now corrected for 32-bit x86 and written explicitly for
all the architectures.

While the structure size between 32-bit and 64-bit x86 is different, it
shouldn't cause any incompatibility between a 32-bit and 64-bit
frontend/backend because the commands are always 56 bits and the padding
are at the end of the structure.

As an aside, the padding sadly cannot be mandated to be 0 as they are
already present. So it is not going to be possible to use the padding
for extending a command in the future.

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

---
    Changes in v2:
        - It is not possible to use the same padding for 32-bit x86 and
        all the other supported architectures.
---
 docs/misc/pvcalls.pandoc        | 18 ++++++++++--------
 xen/include/public/io/pvcalls.h | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc
index 665dad556c39..c25412868f5d 100644
--- a/docs/misc/pvcalls.pandoc
+++ b/docs/misc/pvcalls.pandoc
@@ -246,9 +246,9 @@ The format is defined as follows:
     			uint32_t domain;
     			uint32_t type;
     			uint32_t protocol;
-    			#ifdef CONFIG_X86_32
+			#ifndef CONFIG_X86_32
     			uint8_t pad[4];
-    			#endif
+			#endif
     		} socket;
     		struct xen_pvcalls_connect {
     			uint64_t id;
@@ -257,16 +257,18 @@ The format is defined as follows:
     			uint32_t flags;
     			grant_ref_t ref;
     			uint32_t evtchn;
-    			#ifdef CONFIG_X86_32
+			#ifndef CONFIG_X86_32
     			uint8_t pad[4];
-    			#endif
+			#endif
     		} connect;
     		struct xen_pvcalls_release {
     			uint64_t id;
     			uint8_t reuse;
-    			#ifdef CONFIG_X86_32
+			#ifndef CONFIG_X86_32
     			uint8_t pad[7];
-    			#endif
+			#else
+			uint8_t pad[3];
+			#endif
     		} release;
     		struct xen_pvcalls_bind {
     			uint64_t id;
@@ -276,9 +278,9 @@ The format is defined as follows:
     		struct xen_pvcalls_listen {
     			uint64_t id;
     			uint32_t backlog;
-    			#ifdef CONFIG_X86_32
+			#ifndef CONFIG_X86_32
     			uint8_t pad[4];
-    			#endif
+			#endif
     		} listen;
     		struct xen_pvcalls_accept {
     			uint64_t id;
diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
index cb8171275c13..590c5e9e41aa 100644
--- a/xen/include/public/io/pvcalls.h
+++ b/xen/include/public/io/pvcalls.h
@@ -65,6 +65,9 @@ struct xen_pvcalls_request {
             uint32_t domain;
             uint32_t type;
             uint32_t protocol;
+#ifndef CONFIG_X86_32
+            uint8_t pad[4];
+#endif
         } socket;
         struct xen_pvcalls_connect {
             uint64_t id;
@@ -73,10 +76,18 @@ struct xen_pvcalls_request {
             uint32_t flags;
             grant_ref_t ref;
             uint32_t evtchn;
+#ifndef CONFIG_X86_32
+            uint8_t pad[4];
+#endif
         } connect;
         struct xen_pvcalls_release {
             uint64_t id;
             uint8_t reuse;
+#ifndef CONFIG_X86_32
+            uint8_t pad[7];
+#else
+            uint8_t pad[3];
+#endif
         } release;
         struct xen_pvcalls_bind {
             uint64_t id;
@@ -86,6 +97,9 @@ struct xen_pvcalls_request {
         struct xen_pvcalls_listen {
             uint64_t id;
             uint32_t backlog;
+#ifndef CONFIG_X86_32
+            uint8_t pad[4];
+#endif
         } listen;
         struct xen_pvcalls_accept {
             uint64_t id;
-- 
2.17.1


Re: [PATCH] pvcalls: Document correctly and explicitely the padding for all arches

Posted by Julien Grall 22 weeks ago
Sorry I forgot to CC the maintainers on the e-mail.

I will resent it.

On 16/05/2020 11:19, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The documentation of pvcalls suggests there is padding for 32-bit x86
> at the end of most the structure. However, they are not described in
> in the public header.
> 
> Because of that all the structures would be 32-bit aligned and not
> 64-bit aligned for 32-bit x86.
> 
> For all the other architectures supported (Arm and 64-bit x86), the
> structure are aligned to 64-bit because they contain uint64_t field.
> Therefore all the structures contain implicit padding.
> 
> The paddings are now corrected for 32-bit x86 and written explicitly for
> all the architectures.
> 
> While the structure size between 32-bit and 64-bit x86 is different, it
> shouldn't cause any incompatibility between a 32-bit and 64-bit
> frontend/backend because the commands are always 56 bits and the padding
> are at the end of the structure.
> 
> As an aside, the padding sadly cannot be mandated to be 0 as they are
> already present. So it is not going to be possible to use the padding
> for extending a command in the future.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
>      Changes in v2:
>          - It is not possible to use the same padding for 32-bit x86 and
>          all the other supported architectures.
> ---
>   docs/misc/pvcalls.pandoc        | 18 ++++++++++--------
>   xen/include/public/io/pvcalls.h | 14 ++++++++++++++
>   2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc
> index 665dad556c39..c25412868f5d 100644
> --- a/docs/misc/pvcalls.pandoc
> +++ b/docs/misc/pvcalls.pandoc
> @@ -246,9 +246,9 @@ The format is defined as follows:
>       			uint32_t domain;
>       			uint32_t type;
>       			uint32_t protocol;
> -    			#ifdef CONFIG_X86_32
> +			#ifndef CONFIG_X86_32
>       			uint8_t pad[4];
> -    			#endif
> +			#endif
>       		} socket;
>       		struct xen_pvcalls_connect {
>       			uint64_t id;
> @@ -257,16 +257,18 @@ The format is defined as follows:
>       			uint32_t flags;
>       			grant_ref_t ref;
>       			uint32_t evtchn;
> -    			#ifdef CONFIG_X86_32
> +			#ifndef CONFIG_X86_32
>       			uint8_t pad[4];
> -    			#endif
> +			#endif
>       		} connect;
>       		struct xen_pvcalls_release {
>       			uint64_t id;
>       			uint8_t reuse;
> -    			#ifdef CONFIG_X86_32
> +			#ifndef CONFIG_X86_32
>       			uint8_t pad[7];
> -    			#endif
> +			#else
> +			uint8_t pad[3];
> +			#endif
>       		} release;
>       		struct xen_pvcalls_bind {
>       			uint64_t id;
> @@ -276,9 +278,9 @@ The format is defined as follows:
>       		struct xen_pvcalls_listen {
>       			uint64_t id;
>       			uint32_t backlog;
> -    			#ifdef CONFIG_X86_32
> +			#ifndef CONFIG_X86_32
>       			uint8_t pad[4];
> -    			#endif
> +			#endif
>       		} listen;
>       		struct xen_pvcalls_accept {
>       			uint64_t id;
> diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
> index cb8171275c13..590c5e9e41aa 100644
> --- a/xen/include/public/io/pvcalls.h
> +++ b/xen/include/public/io/pvcalls.h
> @@ -65,6 +65,9 @@ struct xen_pvcalls_request {
>               uint32_t domain;
>               uint32_t type;
>               uint32_t protocol;
> +#ifndef CONFIG_X86_32
> +            uint8_t pad[4];
> +#endif
>           } socket;
>           struct xen_pvcalls_connect {
>               uint64_t id;
> @@ -73,10 +76,18 @@ struct xen_pvcalls_request {
>               uint32_t flags;
>               grant_ref_t ref;
>               uint32_t evtchn;
> +#ifndef CONFIG_X86_32
> +            uint8_t pad[4];
> +#endif
>           } connect;
>           struct xen_pvcalls_release {
>               uint64_t id;
>               uint8_t reuse;
> +#ifndef CONFIG_X86_32
> +            uint8_t pad[7];
> +#else
> +            uint8_t pad[3];
> +#endif
>           } release;
>           struct xen_pvcalls_bind {
>               uint64_t id;
> @@ -86,6 +97,9 @@ struct xen_pvcalls_request {
>           struct xen_pvcalls_listen {
>               uint64_t id;
>               uint32_t backlog;
> +#ifndef CONFIG_X86_32
> +            uint8_t pad[4];
> +#endif
>           } listen;
>           struct xen_pvcalls_accept {
>               uint64_t id;
> 

-- 
Julien Grall