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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 27/10/2022 07:56, Jan Beulich wrote:
> On 26.10.2022 23:24, Julien Grall wrote:
> > 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)?
> Aren't all accesses (supposed to be) under paging lock? At which point
> there's no issue with multiple (or torn) accesses?
Not in the current code base for Arm. I haven't checked whether this is 
the case with the new version.
If it is suitably locked, then I think we should remove all the 
ACCESS_ONCE() and add an ASSERT(spin_is_locked(...)) to make clear this 
should be called with the lock held.
Cheers,

--
Julien Grall