diff mbox series

[RESEND,v2,for-4.14] pvcalls: Document correctly and explicitely the padding for all arches

Message ID 20200516102157.1928-1-julien@xen.org (mailing list archive)
State Superseded
Headers show
Series [RESEND,v2,for-4.14] pvcalls: Document correctly and explicitely the padding for all arches | expand

Commit Message

Julien Grall May 16, 2020, 10:21 a.m. UTC 
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(-)

Comments

Jan Beulich May 18, 2020, 11:51 a.m. UTC | #1 
On 16.05.2020 12:21, Julien Grall wrote:
> --- 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

There's no concept of CONFIG_* in the public headers, the dependency
(as you'll find elsewhere) is on __i386__ / __x86_64__. Also whether
there's any padding really doesn't depend directly on the architecture,
but instead on __alignof__(uint64_t) (i.e. a future port to a 32-bit
arch, even if - like on x86 - just a guest bitness, may similarly
want / need / have no padding here).

Jan
Julien Grall May 18, 2020, 1:15 p.m. UTC | #2 
Hi Jan,

On 18/05/2020 12:51, Jan Beulich wrote:
> On 16.05.2020 12:21, Julien Grall wrote:
>> --- 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
> 
> There's no concept of CONFIG_* in the public headers, the dependency
> (as you'll find elsewhere) is on __i386__ / __x86_64__.

Doh, I forgot it. I will fix it.

> Also whether
> there's any padding really doesn't depend directly on the architecture,
> but instead on __alignof__(uint64_t) (i.e. a future port to a 32-bit
> arch, even if - like on x86 - just a guest bitness, may similarly
> want / need / have no padding here).

Lets imagine someone decide to introduce 32-bit and then later on 
64-bit. Both have different padding requirements. This would result to 
the same mess as on x86.

So I think we shouldn't depend on __alignof__(uint64_t) to avoid any 
more screw up. Obviously extra care would need to be taken if the 
padding is higher, but it is also true in many other place of Xen headers.

Cheers,
diff mbox series

Patch

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;