Re: [PATCH v3 04/19] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Julien Grall <julien@xxxxxxx>]

Hi,

On 02/04/2022 00:35, Stefano Stabellini wrote:
> > +/* Return the level where mapping should be done */
> > +static int xen_pt_mapping_level(unsigned long vfn, mfn_t mfn, unsigned long nr,
> > +                                unsigned int flags)
> > +{
> > +    unsigned int level;
> > +    unsigned long mask;
> Shouldn't mask be 64-bit on aarch32?
The 3 variables we will use (mfn, vfn, nr) are unsigned long. So it is 
fine to define the mask as unsigned long.
> > +}
> > +
> >   static DEFINE_SPINLOCK(xen_pt_lock);
> >   
> >   static int xen_pt_update(unsigned long virt,
> >                            mfn_t mfn,
> > -                         unsigned long nr_mfns,
> > +                         const unsigned long nr_mfns,
> Why const? nr_mfns is an unsigned long so it is passed as value: it
> couldn't change the caller's parameter anyway. Just curious.
Because nr_mfns is used to flush the TLBs. In the original I made the 
mistake to decrement the variable and only discovered later on when the 
TLB contained the wrong entry.
Such bug tends to be very subtle and it is hard to find the root cause. 
So better mark the variable const to avoid any surprise.
The short version of what I wrote is in the commit message. I can write 
a small comment in the code if you want.
> >                            unsigned int flags)
> >   {
> >       int rc = 0;
> > -    unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
> > +    unsigned long vfn = virt >> PAGE_SHIFT;
> > +    unsigned long left = nr_mfns;
> >   
> >       /*
> >        * For arm32, page-tables are different on each CPUs. Yet, they share
> > @@ -1268,14 +1330,24 @@ 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, level;
> > +
> > +        level = xen_pt_mapping_level(vfn, mfn, left, flags);
> > +        order = XEN_PT_LEVEL_ORDER(level);
> > +
> > +        ASSERT(left >= BIT(order, UL));
> > +
> > +        rc = xen_pt_update_entry(root, pfn_to_paddr(vfn), mfn, level, flags);
> NIT: I know we don't have vfn_to_vaddr at the moment and there is no
> widespread usage of vfn in Xen anyway, but it looks off to use
> pfn_to_paddr on a vfn parameter. Maybe open-code pfn_to_paddr instead?
> Or introduce vfn_to_vaddr locally in this file?
To avoid inconsistency with mfn_to_maddr() and gfn_to_gaddr(), I don't 
want ot introduce vfn_to_vaddr() withtout the typesafe part. I think 
this is a bit over the top for now.
So I will open-code pfn_to_paddr().

Cheers,

--
Julien Grall