Re: [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sat, 28 Nov 2020, Julien Grall wrote:
> Hi Stefano,
> 
> On 24/11/2020 00:25, Stefano Stabellini wrote:
> > On Mon, 23 Nov 2020, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 23/11/2020 22:27, Stefano Stabellini wrote:
> > > > On Fri, 20 Nov 2020, Julien Grall wrote:
> > > > > > >         /*
> > > > > > >          * For arm32, page-tables are different on each CPUs. Yet,
> > > > > > > they
> > > > > > > share
> > > > > > > @@ -1265,14 +1287,43 @@ static int xen_pt_update(unsigned long
> > > > > > > virt,
> > > > > > >           spin_lock(&xen_pt_lock);
> > > > > > >     -    for ( ; addr < addr_end; addr += PAGE_SIZE )
> > > > > > > +    while ( left )
> > > > > > >         {
> > > > > > > -        rc = xen_pt_update_entry(root, addr, mfn, flags);
> > > > > > > +        unsigned int order;
> > > > > > > +        unsigned long mask;
> > > > > > > +
> > > > > > > +        /*
> > > > > > > +         * Don't take into account the MFN when removing mapping
> > > > > > > (i.e
> > > > > > > +         * MFN_INVALID) to calculate the correct target order.
> > > > > > > +         *
> > > > > > > +         * XXX: Support superpage mappings if nr is not aligned
> > > > > > > to a
> > > > > > > +         * superpage size.
> > > > > > 
> > > > > > It would be good to add another sentence to explain that the checks
> > > > > > below are simply based on masks and rely on the mfn, vfn, and also
> > > > > > nr_mfn to be superpage aligned. (It took me some time to figure it
> > > > > > out.)
> > > > > 
> > > > > I am not sure to understand what you wrote here. Could you suggest a
> > > > > sentence?
> > > > 
> > > > Something like the following:
> > > > 
> > > > /*
> > > >    * Don't take into account the MFN when removing mapping (i.e
> > > >    * MFN_INVALID) to calculate the correct target order.
> > > >    *
> > > >    * This loop relies on mfn, vfn, and nr_mfn, to be all superpage
> > > >    * aligned, and it uses `mask' to check for that.
> > > 
> > > Unfortunately, I am still not sure to understand this comment.
> > > The loop can deal with any (super)page size (4KB, 2MB, 1GB). There are no
> > > assumption on any alignment for mfn, vfn and nr_mfn.
> > > 
> > > By OR-ing the 3 components together, we can use it to find out the maximum
> > > size that can be used for the mapping.
> > > 
> > > So can you clarify what you mean?
> > 
> > In pseudo-code:
> > 
> >    mask = mfn | vfn | nr_mfns;
> >    if (mask & ((1<<FIRST_ORDER) - 1))
> >    if (mask & ((1<<SECOND_ORDER) - 1))
> >    if (mask & ((1<<THIRD_ORDER) - 1))
> >    ...
> > 
> > As you wrote the mask is used to find the max size that can be used for
> > the mapping.
> > 
> > But let's take nr_mfns out of the equation for a moment for clarity:
> > 
> >    mask = mfn | vfn;
> >    if (mask & ((1<<FIRST_ORDER) - 1))
> >    if (mask & ((1<<SECOND_ORDER) - 1))
> >    if (mask & ((1<<THIRD_ORDER) - 1))
> >    ...
> > 
> > How would you describe this check? I'd call this an alignment check,
> > is it not?
> If you take the ``if`` alone, yes they are alignment check. But if you take
> the overall code, then it will just compute which mapping size can be used.
> 
> However, what I am disputing here is "rely" because there are no assumption
> made on the alignment in the loop (we are able to cater any size). In fact,
> the fact mfn and vfn should be aligned to the mapping size is a requirement
> from the hardware and not the implementation.

OK, maybe the "rely" gives a bad impression. What about:

This loop relies on mfn, vfn, and nr_mfn, to be all superpage aligned
(mfn and vfn have to be architecturally), and it uses `mask' to check
for that.

Feel free to reword it differently if you have a better idea.