From: Julien Grall <julien@xen.org>
To: Andrew Cooper <Andrew.Cooper3@citrix.com>,
Jan Beulich <jbeulich@suse.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>, Wei Liu <wl@xen.org>,
Stefano Stabellini <sstabellini@kernel.org>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
Bertrand Marquis <bertrand.marquis@arm.com>,
Henry Wang <Henry.Wang@arm.com>,
Anthony Perard <anthony.perard@citrix.com>,
Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size
Date: Wed, 26 Oct 2022 22:24:50 +0100 [thread overview]
Message-ID: <4c713750-670b-eac1-5f5b-376de79192eb@xen.org> (raw)
In-Reply-To: <0f048bd2-d08c-8bd5-2a20-7e49e794c679@citrix.com>
Hi Andrew,
On 26/10/2022 20:22, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/mm/hap/hap.c
>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>> @@ -345,6 +345,16 @@ unsigned int hap_get_allocation(struct domain *d)
>>> + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0));
>>> }
>>>
>>> +int hap_get_allocation_bytes(struct domain *d, uint64_t *size)
>>> +{
>>> + unsigned long pages = (d->arch.paging.hap.total_pages +
>>> + d->arch.paging.hap.p2m_pages);
>> Unlike for Arm no ACCESS_ONCE() here? Also the addition can in
>> principle overflow, because being done only in 32 bits.
>
> I'm not actually convinced ARM needs ACCESS_ONCE() to begin with. I
> can't see any legal transformation of that logic which could result in a
> torn load.
AFAIU, ACCESS_ONCE() is not only about torn load but also making sure
that the compiler will only read the value once.
When LTO is enabled (not yet supported) in Xen, can we guarantee the
compiler will not try to access total_pages twice (obviously it would be
caller dependent)?
With that in mind, when LTO is enabled on Linux arm64, the
implementation of READ_ONCE() is not a simple (volatile *) to prevent
the compiler to do harmful convertion. Possibly something we will need
to consider in Xen in the future if we enable LTO. In this context, the
ACCESS_ONCE() would make sense because we don't know (or should not
assume) how the caller will use it.
Regardless that, I think using ACCESS_ONCE() help to document how the
variable should be used. This will reduce the risk that someone decides
to add a new use total_pages like below:
val = d->arch.paging.total_pages;
if ( val == 0 )
return ...
/* use val */
AFAIU, a compiler would be allow to read total_pages twice here. Which
is not what we would want. I am ready to bet this will be missed.
So consistency here is IMO much better. An alternative would be to
document why we think the compiler would not be naughty.
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2022-10-26 21:25 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-26 10:20 [PATCH for-4.17 0/4] XSA-409 fixes Andrew Cooper
2022-10-26 10:20 ` [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size Andrew Cooper
2022-10-26 13:42 ` Jan Beulich
2022-10-26 19:22 ` Andrew Cooper
2022-10-26 21:24 ` Julien Grall [this message]
2022-10-27 6:56 ` Jan Beulich
2022-10-27 9:27 ` Julien Grall
2022-10-27 7:11 ` Jan Beulich
2022-10-28 15:27 ` George Dunlap
2022-10-31 9:26 ` Jan Beulich
2022-10-31 10:12 ` George Dunlap
2022-11-16 1:19 ` Stefano Stabellini
2022-11-16 8:26 ` Jan Beulich
2022-10-27 7:42 ` Jan Beulich
2022-10-26 10:20 ` [PATCH 2/4] tools/tests: Unit test for " Andrew Cooper
2022-10-26 14:24 ` Jan Beulich
2022-10-26 14:35 ` Andrew Cooper
2022-10-26 10:20 ` [PATCH 3/4] xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls Andrew Cooper
2022-10-26 13:22 ` Jason Andryuk
2022-10-26 13:25 ` Andrew Cooper
2022-11-16 1:37 ` Stefano Stabellini
2022-11-16 1:48 ` Andrew Cooper
2022-11-16 2:00 ` Stefano Stabellini
2022-11-16 2:39 ` Henry Wang
2022-11-16 8:30 ` Jan Beulich
2022-11-16 23:41 ` Andrew Cooper
2022-11-16 23:44 ` Stefano Stabellini
2022-11-16 23:51 ` Julien Grall
2022-11-16 23:56 ` Stefano Stabellini
2022-11-17 8:18 ` Jan Beulich
2022-10-26 10:20 ` [PATCH 4/4] xen/arm: Correct the p2m pool size calculations Andrew Cooper
2022-11-11 10:11 ` Henry Wang
2022-11-11 10:54 ` 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=4c713750-670b-eac1-5f5b-376de79192eb@xen.org \
--to=julien@xen.org \
--cc=Andrew.Cooper3@citrix.com \
--cc=Henry.Wang@arm.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=anthony.perard@citrix.com \
--cc=bertrand.marquis@arm.com \
--cc=jbeulich@suse.com \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.