Re: [PATCH v8 03/15] x86/mm: rewrite virt_to_xen_l*e

[Hongyan Xia <hx242@xen.org>]

On Fri, 2020-08-07 at 16:05 +0200, Jan Beulich wrote:
> On 27.07.2020 16:21, Hongyan Xia wrote:
> > From: Wei Liu <wei.liu2@citrix.com>
> > 
> > Rewrite those functions to use the new APIs. Modify its callers to
> > unmap
> > the pointer returned. Since alloc_xen_pagetable_new() is almost
> > never
> > useful unless accompanied by page clearing and a mapping, introduce
> > a
> > helper alloc_map_clear_xen_pt() for this sequence.
> > 
> > Note that the change of virt_to_xen_l1e() also requires
> > vmap_to_mfn() to
> > unmap the page, which requires domain_page.h header in vmap.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > 
> > ---
> > Changed in v8:
> > - s/virtual address/linear address/.
> > - BUG_ON() on NULL return in vmap_to_mfn().
> 
> The justification for this should be recorded in the description. In

Will do.

> reply to v7 I did even suggest how to easily address the issue you
> did notice with large pages, as well as alternative behavior for
> vmap_to_mfn().

One thing about adding SMALL_PAGES is that vmap is common code and I am
not sure if the Arm side is happy with it.

> 
> > --- a/xen/include/asm-x86/page.h
> > +++ b/xen/include/asm-x86/page.h
> > @@ -291,7 +291,15 @@ void copy_page_sse2(void *, const void *);
> >  #define pfn_to_paddr(pfn)   __pfn_to_paddr(pfn)
> >  #define paddr_to_pfn(pa)    __paddr_to_pfn(pa)
> >  #define paddr_to_pdx(pa)    pfn_to_pdx(paddr_to_pfn(pa))
> > -#define
> > vmap_to_mfn(va)     _mfn(l1e_get_pfn(*virt_to_xen_l1e((unsigned
> > long)(va))))
> > +
> > +#define vmap_to_mfn(va)
> > ({                                                  \
> > +        const l1_pgentry_t *pl1e_ = virt_to_xen_l1e((unsigned
> > long)(va));   \
> > +        mfn_t
> > mfn_;                                                         \
> > +        BUG_ON(!pl1e_);                                           
> >           \
> > +        mfn_ =
> > l1e_get_mfn(*pl1e_);                                         \
> > +        unmap_domain_page(pl1e_);                                 
> >           \
> > +        mfn_; })
> 
> Additionally - no idea why I only notice this now, this wants some
> further formatting adjustment: Either
> 
> #define vmap_to_mfn(va)
> ({                                                \
>         const l1_pgentry_t *pl1e_ = virt_to_xen_l1e((unsigned
> long)(va)); \
>         mfn_t
> mfn_;                                                       \
>         BUG_ON(!pl1e_);                                              
>      \
>         mfn_ =
> l1e_get_mfn(*pl1e_);                                       \
>         unmap_domain_page(pl1e_);                                    
>      \
>         mfn_;                                                        
>      \
>     })
> 
> or (preferably imo)
> 
> #define vmap_to_mfn(va)
> ({                                            \
>     const l1_pgentry_t *pl1e_ = virt_to_xen_l1e((unsigned long)(va));
> \
>     mfn_t
> mfn_;                                                       \
>     BUG_ON(!pl1e_);                                                  
>  \
>     mfn_ =
> l1e_get_mfn(*pl1e_);                                       \
>     unmap_domain_page(pl1e_);                                        
>  \
>     mfn_;                                                            
>  \
> })

Will do so in the next rev.

Hongyan