Re: [PATCH 1/4] xen: Introduce non-broken hypercalls for the p2m pool size

[Julien Grall <julien@xxxxxxx>]

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