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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sat, 2 Apr 2022, Julien Grall wrote:
> 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.

Good point


> > > +}
> > > +
> > >   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.

No, that's fine. Thanks for the explanation.


> > >                            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().

Sounds good