Re: [PATCH 1/2] x86: drop unnecessary page table walking in compat r/o M2P handling

[Hongyan Xia <hx242@xxxxxxx>]

On Wed, 2020-04-15 at 10:23 +0200, Jan Beulich wrote:
> We have a global variable where the necessary L2 table is recorded;
> no
> need to inspect L4 and L3 tables (and this way a few less places will
> eventually need adjustment when we want to support 5-level page
> tables).
> Also avoid setting up the L3 entry, as the address range never gets
> used
> anyway (it'll be dropped altogether in a subsequent patch).
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -219,9 +219,7 @@ static void destroy_compat_m2p_mapping(s
>  {
>      unsigned long i, va, rwva, pt_pfn;
>      unsigned long smap = info->spfn, emap = info->spfn;
> -
> -    l3_pgentry_t *l3_ro_mpt;
> -    l2_pgentry_t *l2_ro_mpt;
> +    l2_pgentry_t *l2_ro_mpt = compat_idle_pg_table_l2;
>  
>      if ( smap > ((RDWR_COMPAT_MPT_VIRT_END -
> RDWR_COMPAT_MPT_VIRT_START) >> 2) )
>          return;
> @@ -229,12 +227,6 @@ static void destroy_compat_m2p_mapping(s
>      if ( emap > ((RDWR_COMPAT_MPT_VIRT_END -
> RDWR_COMPAT_MPT_VIRT_START) >> 2) )
>          emap = (RDWR_COMPAT_MPT_VIRT_END -
> RDWR_COMPAT_MPT_VIRT_START) >> 2;
>  
> -    l3_ro_mpt =
> l4e_to_l3e(idle_pg_table[l4_table_offset(HIRO_COMPAT_MPT_VIRT_START)]
> );
> -
> -    ASSERT(l3e_get_flags(l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_V
> IRT_START)]) & _PAGE_PRESENT);
> -
> -    l2_ro_mpt =
> l3e_to_l2e(l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_VIRT_START)]);
> -
>      for ( i = smap; i < emap; )
>      {
>          va = HIRO_COMPAT_MPT_VIRT_START +
> @@ -323,7 +315,6 @@ static int setup_compat_m2p_table(struct
>      unsigned long i, va, smap, emap, rwva, epfn = info->epfn;
>      mfn_t mfn;
>      unsigned int n;
> -    l3_pgentry_t *l3_ro_mpt = NULL;
>      l2_pgentry_t *l2_ro_mpt = NULL;
>      int err = 0;
>  
> @@ -342,13 +333,7 @@ static int setup_compat_m2p_table(struct
>      emap = ( (epfn + ((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1 )) &
>                  ~((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1) );
>  
> -    va = HIRO_COMPAT_MPT_VIRT_START +
> -         smap * sizeof(*compat_machine_to_phys_mapping);
> -    l3_ro_mpt = l4e_to_l3e(idle_pg_table[l4_table_offset(va)]);
> -
> -    ASSERT(l3e_get_flags(l3_ro_mpt[l3_table_offset(va)]) &
> _PAGE_PRESENT);
> -
> -    l2_ro_mpt = l3e_to_l2e(l3_ro_mpt[l3_table_offset(va)]);
> +    l2_ro_mpt = compat_idle_pg_table_l2;
>  
>  #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned int))
>  #define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \
> @@ -627,16 +612,10 @@ void __init paging_init(void)
>  #undef MFN
>  
>      /* Create user-accessible L2 directory to map the MPT for compat
> guests. */
> -    BUILD_BUG_ON(l4_table_offset(RDWR_MPT_VIRT_START) !=
> -                 l4_table_offset(HIRO_COMPAT_MPT_VIRT_START));
> -    l3_ro_mpt = l4e_to_l3e(idle_pg_table[l4_table_offset(
> -        HIRO_COMPAT_MPT_VIRT_START)]);
>      if ( (l2_ro_mpt = alloc_xen_pagetable()) == NULL )
>          goto nomem;
>      compat_idle_pg_table_l2 = l2_ro_mpt;
>      clear_page(l2_ro_mpt);
> -    l3e_write(&l3_ro_mpt[l3_table_offset(HIRO_COMPAT_MPT_VIRT_START)
> ],
> -              l3e_from_paddr(__pa(l2_ro_mpt),
> __PAGE_HYPERVISOR_RO));
>      l2_ro_mpt += l2_table_offset(HIRO_COMPAT_MPT_VIRT_START);
>      /* Allocate and map the compatibility mode machine-to-phys
> table. */
>      mpt_size = (mpt_size >> 1) + (1UL << (L2_PAGETABLE_SHIFT - 1));

The code around here, I am wondering if there is a reason to put it in
this patch. If we bisect, we can end up in a commit which says the
address range of compat is still there in config.h, but actually it's
gone, so this probably belongs to the 2nd patch.

Apart from that,
Reviewed-by: Hongyan Xia <hongyxia@xxxxxxxxxx>

I would like to drop relevant map/unmap patches and replace them with
the new clean-up ones (and place them at the beginning of the series),
if there is no objection with that.

Hongyan