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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.04.2020 16:08, Hongyan Xia wrote:
> @@ -4998,31 +5005,40 @@ static l2_pgentry_t *virt_to_xen_l2e(unsigned long v)
>      if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) )
>      {
>          bool locking = system_state > SYS_STATE_boot;
> -        l2_pgentry_t *l2t = alloc_xen_pagetable();
> +        l2_pgentry_t *l2t;
> +        mfn_t l2mfn = alloc_xen_pagetable_new();
>  
> -        if ( !l2t )
> +        if ( mfn_eq(l2mfn, INVALID_MFN) )
> +        {
> +            UNMAP_DOMAIN_PAGE(pl3e);
>              return NULL;
> +        }
> +        l2t = map_domain_page(l2mfn);
>          clear_page(l2t);
> +        UNMAP_DOMAIN_PAGE(l2t);
>          if ( locking )
>              spin_lock(&map_pgdir_lock);
>          if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) )
>          {
> -            l3e_write(pl3e, l3e_from_paddr(__pa(l2t), __PAGE_HYPERVISOR));
> -            l2t = NULL;
> +            l3e_write(pl3e, l3e_from_mfn(l2mfn, __PAGE_HYPERVISOR));
> +            l2mfn = INVALID_MFN;
>          }
>          if ( locking )
>              spin_unlock(&map_pgdir_lock);
> -        if ( l2t )
> -            free_xen_pagetable(l2t);
> +        free_xen_pagetable_new(l2mfn);
>      }
>  
>      BUG_ON(l3e_get_flags(*pl3e) & _PAGE_PSE);
> -    return l3e_to_l2e(*pl3e) + l2_table_offset(v);
> +    pl2e = map_l2t_from_l3e(*pl3e) + l2_table_offset(v);
> +    unmap_domain_page(pl3e);

To avoid undue pressure on the number of active mappings I'd like
to ask that you unmap first, then establish the new mapping.

> @@ -5095,6 +5119,10 @@ int map_pages_to_xen(
>  
>      while ( nr_mfns != 0 )
>      {
> +        /* Clean up mappings mapped in the previous iteration. */
> +        UNMAP_DOMAIN_PAGE(pl3e);
> +        UNMAP_DOMAIN_PAGE(pl2e);
> +
>          pl3e = virt_to_xen_l3e(virt);

As you don't add a comment here (and at other call sites of
virt_to_xen_l<N>e()), ...

> @@ -5260,9 +5288,12 @@ int map_pages_to_xen(
>              /* Normal page mapping. */
>              if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
>              {
> +                /* This forces the mapping to be populated. */
>                  pl1e = virt_to_xen_l1e(virt);

... I don't see why one needs adding here.

> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -291,7 +291,13 @@ 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));   \
> +        unsigned long pfn_ = l1e_get_pfn(*pl1e_);                           \

l1e_get_mfn()?

> +        unmap_domain_page(pl1e_);                                           \
> +        _mfn(pfn_); })
> +
>  #define vmap_to_page(va)    mfn_to_page(vmap_to_mfn(va))
>  
>  #endif /* !defined(__ASSEMBLY__) */

Jan