Re: [Xen-devel] [PATCH RFC 18/55] x86/mm: switch to new APIs in modify_xen_mappings

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Mar 18, 2019 at 09:14:28PM +0000, Nuernberger, Stefan wrote:
> On Thu, 2019-02-07 at 16:44 +0000, Wei Liu wrote:
> > Page tables allocated in that function should be mapped and unmapped
> > now.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/mm.c | 31 ++++++++++++++++++++++---------
> >  1 file changed, 22 insertions(+), 9 deletions(-)
> > 
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 1ea2974c1f..18c7b43705 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -5436,6 +5436,7 @@ int modify_xen_mappings(unsigned long s,
> > unsigned long e, unsigned int nf)
> >          if ( l3e_get_flags(*pl3e) & _PAGE_PSE )
> >          {
> >              l2_pgentry_t *l2t;
> > +            mfn_t mfn;
> 
> nit: I wouldn't mind if these scoped mfns were caled l2t_mfn / l1t_mfn
> in this patch, too.
> 
> >  
> >              if ( l2_table_offset(v) == 0 &&
> >                   l1_table_offset(v) == 0 &&
> > @@ -5452,13 +5453,15 @@ int modify_xen_mappings(unsigned long s,
> > unsigned long e, unsigned int nf)
> >              }
> >  
> >              /* PAGE1GB: shatter the superpage and fall through. */
> > -            l2t = alloc_xen_pagetable();
> > -            if ( !l2t )
> > +            mfn = alloc_xen_pagetable_new();
> > +            if ( mfn_eq(mfn, INVALID_MFN) )
> >              {
> >                  ASSERT(rc == -ENOMEM);
> >                  goto out;
> >              }
> >  
> > +            l2t = map_xen_pagetable_new(mfn);
> 
> Is map_xen_pagetable always guaranteed to succeed on a valid mfn (also
> in the future)? Otherwise the validity check should be done on l2t as
> before instead of mfn. But it looks like map_xen_pagetable{_new} does
> not deal with invalid mfns.

It is guaranteed to succeed by design.

> 
> > +
> >              for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
> >                  l2e_write(l2t + i,
> >                            l2e_from_pfn(l3e_get_pfn(*pl3e) +
> > @@ -5469,14 +5472,17 @@ int modify_xen_mappings(unsigned long s,
> > unsigned long e, unsigned int nf)
> >              if ( (l3e_get_flags(*pl3e) & _PAGE_PRESENT) &&
> >                   (l3e_get_flags(*pl3e) & _PAGE_PSE) )
> >              {
> > -                l3e_write_atomic(pl3e,
> > l3e_from_mfn(virt_to_mfn(l2t),
> > -                                                    __PAGE_HYPERVISO
> > R));
> > +                l3e_write_atomic(pl3e, l3e_from_mfn(mfn,
> > __PAGE_HYPERVISOR));
> > +                UNMAP_XEN_PAGETABLE_NEW(l2t);
> >                  l2t = NULL;
> 
> That NULL assignment is redundant now. It's done by the UNMAP macro.

Not yet. This is left as-is intentionally. It depends on what we will do
regarding UNMAP_XEN_PAGETABLE.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel