xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Ian Jackson <ian.jackson@citrix.com>, Wei Liu <wl@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [PATCH v3] evtchn/fifo: don't enforce higher than necessary alignment
Date: Wed, 21 Apr 2021 20:52:41 +0100	[thread overview]
Message-ID: <69766ecb-d234-eebb-9b31-1533389a502e@xen.org> (raw)
In-Reply-To: <e6ce03a5-cfec-42ea-91c4-b51849e2f299@suse.com>

Hi,

On 21/04/2021 15:36, Jan Beulich wrote:
> Neither the code nor the original commit provide any justification for
> the need to 8-byte align the struct in all cases. Enforce just as much
> alignment as the structure actually needs - 4 bytes - by using alignof()
> instead of a literal number.

I had another fresh look today at this patch. The 32-bit padding is 
right after the field 'ready'.

I can't for sure tell how the second half is going to ever be used and how.

However, one possibility would be to extend the field 'ready' to 64-bit. 
With the current code, we could easily make a single 64-bit access 
without having to know whether the guest is able to interpret the top half.

With your approach, we may need to have different path depending on the 
padding and ensure the new extension cannot be enabled if the padding is 
4-byte. Otherwise, the atomicity would be broken.

> While relaxation of the requirements is intended here, the primary goal
> is to simply get rid of the hard coded number as well its lack of
> connection to the structure that is is meant to apply to.

Based on what I wrote above, I think the relaxation should not be done 
to give us more flexibility about possible extension to the structure.

Although, I would be worth documenting the reasoning in the code.

Cheers,

-- 
Julien Grall


  reply	other threads:[~2021-04-21 19:53 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-22  8:13 [PATCH v2 0/2] common: XSA-327 follow-up Jan Beulich
2020-12-22  8:14 ` [PATCH v2 1/2] common: map_vcpu_info() cosmetics Jan Beulich
2021-04-01 16:02   ` Julien Grall
2020-12-22  8:15 ` [PATCH v2 2/2] evtchn/fifo: don't enforce higher than necessary alignment Jan Beulich
2021-04-21 14:36 ` [PATCH v3] " Jan Beulich
2021-04-21 19:52   ` Julien Grall [this message]
2021-04-22  9:19     ` Jan Beulich
2021-04-29 11:55       ` 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=69766ecb-d234-eebb-9b31-1533389a502e@xen.org \
    --to=julien@xen.org \
    --cc=George.Dunlap@eu.citrix.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=ian.jackson@citrix.com \
    --cc=jbeulich@suse.com \
    --cc=sstabellini@kernel.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH v3] evtchn/fifo: don'\''t enforce higher than necessary alignment' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).