[PATCH v4 for-4.14 0/2] pvcalls: Reconciliate the spec and the

[PATCH v4 for-4.14 0/2] pvcalls: Reconciliate the spec and the

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

This is v4 to try to reconciliate the pvcalls spec and the header. The
series gain an extra patch (#1) to make clear the header is just a
reference.

This series is candidate for 4.14 to avoid continuing to ship wrong
header even they are just a reference.

For all the changes see in each patch.

Best regards,


Julien Grall (2):
  pvcalls: Clearly spell out that the header is just a reference
  pvcalls: Document correctly and explicitely the padding for all arches

 docs/misc/pvcalls.pandoc        | 8 --------
 xen/include/public/io/pvcalls.h | 7 +++++++
 2 files changed, 7 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH v4 for-4.14 1/2] pvcalls: Clearly spell out that the header is just a reference

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

A recent thread on xen-devel [1] pointed out that the header was
provided as a reference for the specification.

Unfortunately, this was never written down in xen.git so for an external
user (or a reviewer) it is not clear whether the spec or the header
shoudl be followed when there is a conflict.

To avoid more confusion, a paragraph is added at the top of the header
to clearly spell out it is only provided for reference.

[1] https://lore.kernel.org/xen-devel/alpine.DEB.2.21.2006151343430.9074@sstabellini-ThinkPad-T480s/

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

---
    Changes in v4:
        - New patch
---
 xen/include/public/io/pvcalls.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
index cb8171275c13..905880735dda 100644
--- a/xen/include/public/io/pvcalls.h
+++ b/xen/include/public/io/pvcalls.h
@@ -3,6 +3,9 @@
  *
  * Refer to docs/misc/pvcalls.markdown for the specification
  *
+ * The header is provided as a C reference for the specification. In
+ * case of conflict, the specification is authoritative.
+ *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to
  * deal in the Software without restriction, including without limitation the
-- 
2.17.1

Re: [PATCH v4 for-4.14 1/2] pvcalls: Clearly spell out that the header is just a reference

[Jürgen Groß]

On 27.06.20 11:55, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> A recent thread on xen-devel [1] pointed out that the header was
> provided as a reference for the specification.
> 
> Unfortunately, this was never written down in xen.git so for an external
> user (or a reviewer) it is not clear whether the spec or the header
> shoudl be followed when there is a conflict.

should

> 
> To avoid more confusion, a paragraph is added at the top of the header
> to clearly spell out it is only provided for reference.
> 
> [1] https://lore.kernel.org/xen-devel/alpine.DEB.2.21.2006151343430.9074@sstabellini-ThinkPad-T480s/
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

RE: [PATCH v4 for-4.14 1/2] pvcalls: Clearly spell out that the header is just a reference

[Paul Durrant]

> -----Original Message-----
> From: Jürgen Groß <jgross@suse.com>
> Sent: 27 June 2020 12:55
> To: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org
> Cc: paul@xen.org; Julien Grall <jgrall@amazon.com>
> Subject: Re: [PATCH v4 for-4.14 1/2] pvcalls: Clearly spell out that the header is just a reference
> 
> On 27.06.20 11:55, Julien Grall wrote:
> > From: Julien Grall <jgrall@amazon.com>
> >
> > A recent thread on xen-devel [1] pointed out that the header was
> > provided as a reference for the specification.
> >
> > Unfortunately, this was never written down in xen.git so for an external
> > user (or a reviewer) it is not clear whether the spec or the header
> > shoudl be followed when there is a conflict.
> 
> should
> 
> >
> > To avoid more confusion, a paragraph is added at the top of the header
> > to clearly spell out it is only provided for reference.
> >
> > [1] https://lore.kernel.org/xen-devel/alpine.DEB.2.21.2006151343430.9074@sstabellini-ThinkPad-T480s/
> >
> > Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>
> 

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

> 
> Juergen

[PATCH v4 for-4.14 2/2] pvcalls: Document correctly and explicitely the padding for all arches

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

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

Given the specification is authoriitative, the padding will the same for
the all architectures. The potential breakage of compatibility is ought
to be fine as pvcalls is still a tech preview.

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>

---
    This wants to be included in Xen 4.14 to avoid more users to rely on
    wrong public headers.

    Changes in v4:
        - Revert back to v1 for the patch and expand the commit message

    Changes in v3:
        - Use __i386__ rather than CONFIG_X86_32

    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        | 8 --------
 xen/include/public/io/pvcalls.h | 4 ++++
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc
index 665dad556c39..971cd8f4b122 100644
--- a/docs/misc/pvcalls.pandoc
+++ b/docs/misc/pvcalls.pandoc
@@ -246,9 +246,7 @@ The format is defined as follows:
     			uint32_t domain;
     			uint32_t type;
     			uint32_t protocol;
-    			#ifdef CONFIG_X86_32
     			uint8_t pad[4];
-    			#endif
     		} socket;
     		struct xen_pvcalls_connect {
     			uint64_t id;
@@ -257,16 +255,12 @@ The format is defined as follows:
     			uint32_t flags;
     			grant_ref_t ref;
     			uint32_t evtchn;
-    			#ifdef CONFIG_X86_32
     			uint8_t pad[4];
-    			#endif
     		} connect;
     		struct xen_pvcalls_release {
     			uint64_t id;
     			uint8_t reuse;
-    			#ifdef CONFIG_X86_32
     			uint8_t pad[7];
-    			#endif
     		} release;
     		struct xen_pvcalls_bind {
     			uint64_t id;
@@ -276,9 +270,7 @@ The format is defined as follows:
     		struct xen_pvcalls_listen {
     			uint64_t id;
     			uint32_t backlog;
-    			#ifdef CONFIG_X86_32
     			uint8_t pad[4];
-    			#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 905880735dda..6da6b5533a10 100644
--- a/xen/include/public/io/pvcalls.h
+++ b/xen/include/public/io/pvcalls.h
@@ -68,6 +68,7 @@ struct xen_pvcalls_request {
             uint32_t domain;
             uint32_t type;
             uint32_t protocol;
+            uint8_t pad[4];
         } socket;
         struct xen_pvcalls_connect {
             uint64_t id;
@@ -76,10 +77,12 @@ struct xen_pvcalls_request {
             uint32_t flags;
             grant_ref_t ref;
             uint32_t evtchn;
+            uint8_t pad[4];
         } connect;
         struct xen_pvcalls_release {
             uint64_t id;
             uint8_t reuse;
+            uint8_t pad[7];
         } release;
         struct xen_pvcalls_bind {
             uint64_t id;
@@ -89,6 +92,7 @@ struct xen_pvcalls_request {
         struct xen_pvcalls_listen {
             uint64_t id;
             uint32_t backlog;
+            uint8_t pad[4];
         } listen;
         struct xen_pvcalls_accept {
             uint64_t id;
-- 
2.17.1

Re: [PATCH v4 for-4.14 2/2] pvcalls: Document correctly and explicitely the padding for all arches

[Jürgen Groß]

On 27.06.20 11:55, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The specification 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.
> 
> Given the specification is authoriitative, the padding will the same for

s/authoriitative/authoritative/

> the all architectures. The potential breakage of compatibility is ought

s/the//

> to be fine as pvcalls is still a tech preview.
> 
> 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>

With above fixed:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

RE: [PATCH v4 for-4.14 2/2] pvcalls: Document correctly and explicitely the padding for all arches

[Paul Durrant]

> -----Original Message-----
> From: Jürgen Groß <jgross@suse.com>
> Sent: 27 June 2020 12:54
> To: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org
> Cc: paul@xen.org; Julien Grall <jgrall@amazon.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>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v4 for-4.14 2/2] pvcalls: Document correctly and explicitely the padding for all
> arches
> 
> On 27.06.20 11:55, Julien Grall wrote:
> > From: Julien Grall <jgrall@amazon.com>
> >
> > The specification 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.
> >
> > Given the specification is authoriitative, the padding will the same for
> 
> s/authoriitative/authoritative/
> 
> > the all architectures. The potential breakage of compatibility is ought
> 
> s/the//
> 
> > to be fine as pvcalls is still a tech preview.
> >
> > 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>
> 
> With above fixed:
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>
> 

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

> 
> Juergen

Re: [PATCH v4 for-4.14 2/2] pvcalls: Document correctly and explicitely the padding for all arches

[Jan Beulich]

On 27.06.2020 11:55, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The specification 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.

The added padding doesn't change the alignment. It's sizeof() which
gets corrected this way.

> 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.
> 
> Given the specification is authoriitative, the padding will the same for

Nit: ... will be the same ...

> the all architectures. The potential breakage of compatibility is ought

Nit: Drop "is".

> to be fine as pvcalls is still a tech preview.
> 
> 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.

Why is the other adjustment fine to make due to still being tech
preview, but this one wouldn't be for the same reason?

Jan

Re: [PATCH v4 for-4.14 2/2] pvcalls: Document correctly and explicitely the padding for all arches

[Julien Grall]

Hi Jan,

On 29/06/2020 09:28, Jan Beulich wrote:
> On 27.06.2020 11:55, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The specification 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.
> 
> The added padding doesn't change the alignment. It's sizeof() which
> gets corrected this way.

I will update the commit message.

> 
>> 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.
>>
>> Given the specification is authoriitative, the padding will the same for
> 
> Nit: ... will be the same ...

Ok.

> 
>> the all architectures. The potential breakage of compatibility is ought
> 
> Nit: Drop "is".

Ok.

>> to be fine as pvcalls is still a tech preview.
>>
>> 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.
> 
> Why is the other adjustment fine to make due to still being tech
> preview, but this one wouldn't be for the same reason?

This is mostly a left-over of the previous message. Although, I am not 
really inclined to address this myself any time soon.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 for-4.14 2/2] pvcalls: Document correctly and explicitely the padding for all arches

[Jan Beulich]

On 29.06.2020 12:03, Julien Grall wrote:
> On 29/06/2020 09:28, Jan Beulich wrote:
>> On 27.06.2020 11:55, Julien Grall wrote:
>>> 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.
>>
>> Why is the other adjustment fine to make due to still being tech
>> preview, but this one wouldn't be for the same reason?
> 
> This is mostly a left-over of the previous message. Although, I am not 
> really inclined to address this myself any time soon.

Sure, I didn't mean to indicate I might expect you to. But perhaps
here the wording could be slightly changed as well?

Jan

Re: [PATCH v4 for-4.14 2/2] pvcalls: Document correctly and explicitely the padding for all arches

[Julien Grall]

On 29/06/2020 12:22, Jan Beulich wrote:
> On 29.06.2020 12:03, Julien Grall wrote:
>> On 29/06/2020 09:28, Jan Beulich wrote:
>>> On 27.06.2020 11:55, Julien Grall wrote:
>>>> 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.
>>>
>>> Why is the other adjustment fine to make due to still being tech
>>> preview, but this one wouldn't be for the same reason?
>>
>> This is mostly a left-over of the previous message. Although, I am not
>> really inclined to address this myself any time soon.
> 
> Sure, I didn't mean to indicate I might expect you to. But perhaps
> here the wording could be slightly changed as well?

I am planning to remove the paragraph completely.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 for-4.14 2/2] pvcalls: Document correctly and explicitely the padding for all arches

[Stefano Stabellini]

On Sat, 27 Jun 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The specification 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.
> 
> Given the specification is authoriitative, the padding will the same for
> the all architectures. The potential breakage of compatibility is ought
> to be fine as pvcalls is still a tech preview.
> 
> 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>

Aside from typos other mentioned:

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


> ---
>     This wants to be included in Xen 4.14 to avoid more users to rely on
>     wrong public headers.
> 
>     Changes in v4:
>         - Revert back to v1 for the patch and expand the commit message
> 
>     Changes in v3:
>         - Use __i386__ rather than CONFIG_X86_32
> 
>     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        | 8 --------
>  xen/include/public/io/pvcalls.h | 4 ++++
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc
> index 665dad556c39..971cd8f4b122 100644
> --- a/docs/misc/pvcalls.pandoc
> +++ b/docs/misc/pvcalls.pandoc
> @@ -246,9 +246,7 @@ The format is defined as follows:
>      			uint32_t domain;
>      			uint32_t type;
>      			uint32_t protocol;
> -    			#ifdef CONFIG_X86_32
>      			uint8_t pad[4];
> -    			#endif
>      		} socket;
>      		struct xen_pvcalls_connect {
>      			uint64_t id;
> @@ -257,16 +255,12 @@ The format is defined as follows:
>      			uint32_t flags;
>      			grant_ref_t ref;
>      			uint32_t evtchn;
> -    			#ifdef CONFIG_X86_32
>      			uint8_t pad[4];
> -    			#endif
>      		} connect;
>      		struct xen_pvcalls_release {
>      			uint64_t id;
>      			uint8_t reuse;
> -    			#ifdef CONFIG_X86_32
>      			uint8_t pad[7];
> -    			#endif
>      		} release;
>      		struct xen_pvcalls_bind {
>      			uint64_t id;
> @@ -276,9 +270,7 @@ The format is defined as follows:
>      		struct xen_pvcalls_listen {
>      			uint64_t id;
>      			uint32_t backlog;
> -    			#ifdef CONFIG_X86_32
>      			uint8_t pad[4];
> -    			#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 905880735dda..6da6b5533a10 100644
> --- a/xen/include/public/io/pvcalls.h
> +++ b/xen/include/public/io/pvcalls.h
> @@ -68,6 +68,7 @@ struct xen_pvcalls_request {
>              uint32_t domain;
>              uint32_t type;
>              uint32_t protocol;
> +            uint8_t pad[4];
>          } socket;
>          struct xen_pvcalls_connect {
>              uint64_t id;
> @@ -76,10 +77,12 @@ struct xen_pvcalls_request {
>              uint32_t flags;
>              grant_ref_t ref;
>              uint32_t evtchn;
> +            uint8_t pad[4];
>          } connect;
>          struct xen_pvcalls_release {
>              uint64_t id;
>              uint8_t reuse;
> +            uint8_t pad[7];
>          } release;
>          struct xen_pvcalls_bind {
>              uint64_t id;
> @@ -89,6 +92,7 @@ struct xen_pvcalls_request {
>          struct xen_pvcalls_listen {
>              uint64_t id;
>              uint32_t backlog;
> +            uint8_t pad[4];
>          } listen;
>          struct xen_pvcalls_accept {
>              uint64_t id;
> -- 
> 2.17.1
> 

Re: [PATCH v4 for-4.14 2/2] pvcalls: Document correctly and explicitely the padding for all arches

[Julien Grall]

Hi,

On 27/06/2020 10:55, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The specification 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.
> 
> Given the specification is authoriitative, the padding will the same for
> the all architectures. The potential breakage of compatibility is ought
> to be fine as pvcalls is still a tech preview.
> 
> 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>

It looks like most of the comments are on the commit message. So rather 
than sending the series again, below a new version of the commit message:

"
The specification 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 have a different size between 
32-bit x86 and 64-bit x86.

For all the other architectures supported (Arm and 64-bit x86), the
structure have the sames sizes because they contain implicit padding 
thanks to the 64-bit alinment of the field uint64_t field.

Given the specification is authoritative, the padding will now be the 
same for all architectures. The potential breakage of compatibility is 
ought to be fine as pvcalls is still a tech preview.
"

Cheers,


-- 
Julien Grall

Re: [PATCH v4 for-4.14 2/2] pvcalls: Document correctly and explicitely the padding for all arches

[Julien Grall]

Hi,

Gentle ping.

Is the new commit message fine?

Cheers,

On 04/07/2020 16:29, Julien Grall wrote:
> Hi,
> 
> On 27/06/2020 10:55, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The specification 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.
>>
>> Given the specification is authoriitative, the padding will the same for
>> the all architectures. The potential breakage of compatibility is ought
>> to be fine as pvcalls is still a tech preview.
>>
>> 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>
> 
> It looks like most of the comments are on the commit message. So rather 
> than sending the series again, below a new version of the commit message:
> 
> "
> The specification 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 have a different size between 
> 32-bit x86 and 64-bit x86.
> 
> For all the other architectures supported (Arm and 64-bit x86), the
> structure have the sames sizes because they contain implicit padding 
> thanks to the 64-bit alinment of the field uint64_t field.
> 
> Given the specification is authoritative, the padding will now be the 
> same for all architectures. The potential breakage of compatibility is 
> ought to be fine as pvcalls is still a tech preview.
> "
> 
> Cheers,
> 
> 

-- 
Julien Grall

Re: [PATCH v4 for-4.14 2/2] pvcalls: Document correctly and explicitely the padding for all arches

[Stefano Stabellini]

On Thu, 9 Jul 2020, Julien Grall wrote:
> Hi,
> 
> Gentle ping.
> 
> Is the new commit message fine?
> 
> Cheers,
> 
> On 04/07/2020 16:29, Julien Grall wrote:
> > Hi,
> > 
> > On 27/06/2020 10:55, Julien Grall wrote:
> > > From: Julien Grall <jgrall@amazon.com>
> > > 
> > > The specification 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.
> > > 
> > > Given the specification is authoriitative, the padding will the same for
> > > the all architectures. The potential breakage of compatibility is ought
> > > to be fine as pvcalls is still a tech preview.
> > > 
> > > 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>
> > 
> > It looks like most of the comments are on the commit message. So rather than
> > sending the series again, below a new version of the commit message:
> > 
> > "
> > The specification 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 have a different size between
> > 32-bit x86 and 64-bit x86.
> > 
> > For all the other architectures supported (Arm and 64-bit x86), the
> > structure have the sames sizes because they contain implicit padding thanks
> > to the 64-bit alinment of the field uint64_t field.
> > 
> > Given the specification is authoritative, the padding will now be the same
> > for all architectures. The potential breakage of compatibility is ought to
> > be fine as pvcalls is still a tech preview.
> > "

Looks good to me

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

Re: [PATCH v4 for-4.14 2/2] pvcalls: Document correctly and explicitely the padding for all arches

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 2542 bytes --]

On Thu, 9 Jul 2020, 22:43 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Thu, 9 Jul 2020, Julien Grall wrote:
> > Hi,
> >
> > Gentle ping.
> >
> > Is the new commit message fine?
> >
> > Cheers,
> >
> > On 04/07/2020 16:29, Julien Grall wrote:
> > > Hi,
> > >
> > > On 27/06/2020 10:55, Julien Grall wrote:
> > > > From: Julien Grall <jgrall@amazon.com>
> > > >
> > > > The specification 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.
> > > >
> > > > Given the specification is authoriitative, the padding will the same
> for
> > > > the all architectures. The potential breakage of compatibility is
> ought
> > > > to be fine as pvcalls is still a tech preview.
> > > >
> > > > 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>
> > >
> > > It looks like most of the comments are on the commit message. So
> rather than
> > > sending the series again, below a new version of the commit message:
> > >
> > > "
> > > The specification 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 have a different size between
> > > 32-bit x86 and 64-bit x86.
> > >
> > > For all the other architectures supported (Arm and 64-bit x86), the
> > > structure have the sames sizes because they contain implicit padding
> thanks
> > > to the 64-bit alinment of the field uint64_t field.
> > >
> > > Given the specification is authoritative, the padding will now be the
> same
> > > for all architectures. The potential breakage of compatibility is
> ought to
> > > be fine as pvcalls is still a tech preview.
> > > "
>
> Looks good to me
>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>


Thanks! I don't have access to my work laptop today. Can any of the
committers merge it so it doesn't miss 4.14?

Cheers,

>

[-- Attachment #2: Type: text/html, Size: 3906 bytes --]