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
