Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien@xen.org>
Cc: Juergen Gross <jgross@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	paul@xen.org, Andrew Cooper <andrew.cooper3@citrix.com>,
	Julien Grall <jgrall@amazon.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches
Date: Mon, 15 Jun 2020 18:00:34 -0700 (PDT)
Message-ID: <alpine.DEB.2.21.2006151343430.9074@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <20200613184132.11880-1-julien@xen.org>

On Sat, 13 Jun 2020, 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 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        | 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..caa71b36d78b 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 __i386__
>      			uint8_t pad[4];
> -    			#endif
> +			#endif


Hi Julien,

Thank you for doing this, and sorry for having missed v2 of this patch, I
should have replied earlier.

The intention of the #ifdef blocks like:

  #ifdef CONFIG_X86_32
    uint8_t pad[4];
  #endif

in pvcalls.pandoc was to make sure that these structs would be 64bit
aligned on x86_32 too.

I realize that the public header doesn't match, but the spec is the
"master copy". We have been saying it for a while (Andy in particular)
that the specification documents are the one that define the protocol,
not the public headers. This is the very first time we get to act on
that statement. What a special occasion this is :-)

So I think we should keep the specification as is and fix the public
header instead. Something like v1 of this patch.


  parent reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-13 18:41 Julien Grall
2020-06-15  8:26 ` Paul Durrant
2020-06-16  1:00 ` Stefano Stabellini [this message]
2020-06-16  7:13   ` Jan Beulich
2020-06-16  9:39   ` Julien Grall
2020-06-16 20:57     ` Stefano Stabellini
2020-06-16 21:31       ` Julien Grall
2020-06-18  1:34         ` Stefano Stabellini
2020-06-18 15:00           ` Julien Grall
2020-06-24 11:29             ` [INPUT REQUESTED][PATCH " Julien Grall
2020-06-24 12:05               ` Ian Jackson
2020-06-24 12:04             ` [PATCH " Ian Jackson
2020-06-26 17:46               ` Julien Grall
2020-06-16  8:26 ` Jan Beulich
2020-06-16  9:19   ` Julien Grall
2020-06-16  9:36     ` Jan Beulich
2020-06-16  9:42       ` Julien Grall

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.21.2006151343430.9074@sstabellini-ThinkPad-T480s \
    --to=sstabellini@kernel.org \
    --cc=andrew.cooper3@citrix.com \
    --cc=george.dunlap@citrix.com \
    --cc=ian.jackson@eu.citrix.com \
    --cc=jbeulich@suse.com \
    --cc=jgrall@amazon.com \
    --cc=jgross@suse.com \
    --cc=julien@xen.org \
    --cc=paul@xen.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git