Re: [Xen-devel] [PATCH 06/22] xen/arm: p2m: Use the typesafe MFN in mfn_to_p2m_entry

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 27 Jul 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 26/07/16 23:28, Stefano Stabellini wrote:
> > On Wed, 20 Jul 2016, Julien Grall wrote:
> > > @@ -411,7 +411,7 @@ static int p2m_create_table(struct domain *d, lpae_t
> > > *entry,
> > >      if ( splitting )
> > >      {
> > >          p2m_type_t t = entry->p2m.type;
> > > -        unsigned long base_pfn = entry->p2m.base;
> > > +        mfn_t mfn = _mfn(entry->p2m.base);
> > >          int i;
> > > 
> > >          /*
> > > @@ -420,8 +420,9 @@ static int p2m_create_table(struct domain *d, lpae_t
> > > *entry,
> > >           */
> > >           for ( i=0 ; i < LPAE_ENTRIES; i++ )
> > >           {
> > > -             pte = mfn_to_p2m_entry(base_pfn +
> > > (i<<(level_shift-LPAE_SHIFT)),
> > > -                                    MATTR_MEM, t, p2m->default_access);
> > > +             pte = mfn_to_p2m_entry(mfn, MATTR_MEM, t,
> > > p2m->default_access);
> > > +
> > > +             mfn = mfn_add(mfn, 1UL << (level_shift - LPAE_SHIFT));
> > 
> > Should we be incrementing mfn before calling mfn_to_p2m_entry?
> 
> No. The base of the superpage is mfn, after splitting the first entry will be
> equal to the base, the second entry base + level_size...

I understand what the patch is doing now, I confused "1" with "i" :-)
The patch is OK. It might be more obvious as the following:


  for ( i=0 ; i < LPAE_ENTRIES; i++ )
  {
     pte = mfn_to_p2m_entry(mfn_add(mfn, (i<<(level_shift-LPAE_SHIFT))),
                            MATTR_MEM, t, p2m->default_access);


However it's just a matter of taste, so I'll let you choose the way you
prefer.

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel