Re: [PATCH v3 02/18] x86/mm: purge unneeded destroy_perdomain_mapping()

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Wed, Oct 07, 2026 at 11:40:35AM +0100, George Dunlap wrote:
> From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> We want to change per-domain mappings to be per-vCPU mappings.  In
> preparation for that, we want to arrange that
> destroy_perdomain_mapping() work either with a single perdomain area,
> or with a per-vCPU perdomain area.
> 
> There are two calls made from domain-scoped contexts; both calls turn
> out to be unnecessary:
> 
> - destroy_perdomain_mapping() is not logically the undo of
>   create_perdomain_mapping(), as the name and its use in
>   hvm_domain_initialise() suggest.  create_ allocates a per-domain L3,
>   but destroy_ tears down mappings without freeing it; and since the
>   call here passes nr == 0, it tears down nothing at all.  The
>   per-domain L3 page is actually freed by free_perdomain_mappings(),
>   which hvm_domain_initialise()'s caller, arch_domain_create(),
>   already invokes on its failure path.
> 
> - The call in pv_domain_destroy() is redundant at both of its call
>   sites.  arch_domain_destroy() unconditionally calls
>   free_perdomain_mappings(), which tears down the same entries and
>   additionally frees the page-table structures; and the other caller,
>   the error path of pv_domain_initialise(), returns into
>   arch_domain_create()'s failure path, which invokes
>   free_perdomain_mappings() as well.
> 
> One vCPU-scoped caller, pv_destroy_gdt_ldt_l1tab(), is covered the
> same way: a vCPU is only ever destroyed together with its domain, or
> on the failure path of creating it, and both routes end in
> free_perdomain_mappings(); the GDT/LDT slots hold no area-owned pages,
> so there is nothing else for the call to do.  While we're getting rid
> of unneccessary callers, drop this one too.
> 
> Note that free_compat_arg_xlat() keeps its explicit teardown: in
> addition to vCPU teardown, it serves switch_compat()'s undo path,
> where the domain lives on as a 64-bit one and the argument-translation
> pages have to go.  Add a comment to make sure it isn't missed.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Assisted-by: Claude Code:claude-fable-5
> Signed-off-by: George Dunlap <gwd@xxxxxxxxxxxxxx>
> ---
> Changes in v3:
> - Note that pv_domain_destroy()'s other call site, the error path of
>   pv_domain_initialise(), is covered the same way (Jan).
> - Also drop pv_destroy_gdt_ldt_l1tab(), the per-vCPU teardown call
>   (Jan), and comment on why free_compat_arg_xlat() keeps its own.  The
>   Reviewed-by tags given for the previous form are dropped.
> 
> Changes in v2:
> - Added to the series
> 
> Changes since the previously posted version:
> - Reworked the commit message to make it more clear how it fits in
>   with the larger series.  No functional change.
> ---
>  xen/arch/x86/hvm/hvm.c   |  1 -
>  xen/arch/x86/pv/domain.c | 10 ----------
>  xen/arch/x86/x86_64/mm.c |  5 +++++
>  3 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 192309c2fc..3924c1b204 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -730,7 +730,6 @@ int hvm_domain_initialise(struct domain *d,
>      XFREE(d->arch.hvm.irq);
>   fail0:
>      hvm_destroy_cacheattr_region_list(d);
> -    destroy_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0);
>   fail:
>      hvm_domain_relinquish_resources(d);
>      XFREE(d->arch.hvm.io_handler);
> diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
> index 03262a4952..07ea4bb4d2 100644
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -332,12 +332,6 @@ static int pv_create_gdt_ldt_l1tab(struct vcpu *v)
>                                      NULL);
>  }
>  
> -static void pv_destroy_gdt_ldt_l1tab(struct vcpu *v)
> -{
> -    destroy_perdomain_mapping(v->domain, GDT_VIRT_START(v),
> -                              1U << GDT_LDT_VCPU_SHIFT);
> -}
> -
>  void pv_vcpu_destroy(struct vcpu *v)
>  {
>      if ( is_pv_32bit_vcpu(v) )
> @@ -346,7 +340,6 @@ void pv_vcpu_destroy(struct vcpu *v)
>          release_compat_l4(v);
>      }
>  
> -    pv_destroy_gdt_ldt_l1tab(v);
>      XFREE(v->arch.pv.trap_ctxt);
>  }
>  
> @@ -394,9 +387,6 @@ void pv_domain_destroy(struct domain *d)
>  {
>      pv_l1tf_domain_destroy(d);
>  
> -    destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
> -                              GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
> -
>      XFREE(d->arch.pv.cpuidmasks);
>  
>      FREE_XENHEAP_PAGE(d->arch.pv.gdt_ldt_l1tab);
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index 8eadab7933..2a1805964d 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -737,6 +737,11 @@ int setup_compat_arg_xlat(struct vcpu *v)
>                                      NULL, NIL(struct page_info *));
>  }
>  
> +/*
> + * Besides vCPU teardown, which free_perdomain_mappings() would cover, this
> + * serves switch_compat()'s undo path: the domain lives on as a 64-bit one
> + * there, so the translation area has to go right away.

I'm unsure we want to be that verbose, but if so I would mention PV
explicitly, possible: "the PV domain lives ..."

Otherwise LGTM, but I don't think I can RB or Ack it being also the
author.

Thanks, Roger.