x86/paravirt: merge activate_mm and dup_mmap callbacks

  The two paravirt callbacks .mmu.activate_mm and .mmu.dup_mmap are
sharing the same implementations in all cases: for Xen PV guests they
are pinning the PGD of the new mm_struct, and for all other cases
they are a NOP.

So merge them to a common callback .mmu.enter_mmap (in contrast to the
corresponding already existing .mmu.exit_mmap).

As the first parameter of the old callbacks isn't used, drop it from
the replacement one.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/include/asm/mmu_context.h    |  4 ++--
 arch/x86/include/asm/paravirt.h       | 14 +++-----------
 arch/x86/include/asm/paravirt_types.h |  7 ++-----
 arch/x86/kernel/paravirt.c            |  3 +--
 arch/x86/xen/mmu_pv.c                 | 12 ++----------
 5 files changed, 10 insertions(+), 30 deletions(-)
  

On 1/12/23 10:21 AM, Juergen Gross wrote:
> The two paravirt callbacks .mmu.activate_mm and .mmu.dup_mmap are
> sharing the same implementations in all cases: for Xen PV guests they
> are pinning the PGD of the new mm_struct, and for all other cases
> they are a NOP.
>
> So merge them to a common callback .mmu.enter_mmap (in contrast to the
> corresponding already existing .mmu.exit_mmap).
>
> As the first parameter of the old callbacks isn't used, drop it from
> the replacement one.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
  

Hi Juergen,

On 1/12/23 7:21 AM, Juergen Gross wrote:
> The two paravirt callbacks .mmu.activate_mm and .mmu.dup_mmap are
> sharing the same implementations in all cases: for Xen PV guests they
> are pinning the PGD of the new mm_struct, and for all other cases
> they are a NOP.
> 

I was expecting that the duplicated functions xen_activate_mm() and
xen_dup_mmap() would be merged into a common one, and that both
.mmu.activate_mm and .mmu.dup_mmap callbacks would be mapped to that
common implementation for Xen PV.

> So merge them to a common callback .mmu.enter_mmap (in contrast to the
> corresponding already existing .mmu.exit_mmap).
> 

Instead, this patch seems to be merging the callbacks themselves...

I see that's not an issue right now since there is no other actual
user for these callbacks. But are we sure that merging the callbacks
just because the current user (Xen PV) has the same implementation for
both is a good idea? The callbacks are invoked at distinct points from
fork/exec, so wouldn't it be valuable to retain that distinction in
semantics in the callbacks as well?

However, if you believe that two separate callbacks are not really
required here (because there is no significant difference in what they
mean, rather than because their callback implementations happen to be
the same right now), then could you please expand on this and call it
out in the commit message, please?

Thank you!

Regards,
Srivatsa
VMware Photon OS

> As the first parameter of the old callbacks isn't used, drop it from
> the replacement one.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/include/asm/mmu_context.h    |  4 ++--
>  arch/x86/include/asm/paravirt.h       | 14 +++-----------
>  arch/x86/include/asm/paravirt_types.h |  7 ++-----
>  arch/x86/kernel/paravirt.c            |  3 +--
>  arch/x86/xen/mmu_pv.c                 | 12 ++----------
>  5 files changed, 10 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
> index b8d40ddeab00..6a14b6c2165c 100644
> --- a/arch/x86/include/asm/mmu_context.h
> +++ b/arch/x86/include/asm/mmu_context.h
> @@ -134,7 +134,7 @@ extern void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
>  
>  #define activate_mm(prev, next)			\
>  do {						\
> -	paravirt_activate_mm((prev), (next));	\
> +	paravirt_enter_mmap(next);		\
>  	switch_mm((prev), (next), NULL);	\
>  } while (0);
>  
> @@ -167,7 +167,7 @@ static inline void arch_dup_pkeys(struct mm_struct *oldmm,
>  static inline int arch_dup_mmap(struct mm_struct *oldmm, struct mm_struct *mm)
>  {
>  	arch_dup_pkeys(oldmm, mm);
> -	paravirt_arch_dup_mmap(oldmm, mm);
> +	paravirt_enter_mmap(mm);
>  	return ldt_dup_context(oldmm, mm);
>  }
>  
> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> index 73e9522db7c1..07bbdceaf35a 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -332,16 +332,9 @@ static inline void tss_update_io_bitmap(void)
>  }
>  #endif
>  
> -static inline void paravirt_activate_mm(struct mm_struct *prev,
> -					struct mm_struct *next)
> +static inline void paravirt_enter_mmap(struct mm_struct *next)
>  {
> -	PVOP_VCALL2(mmu.activate_mm, prev, next);
> -}
> -
> -static inline void paravirt_arch_dup_mmap(struct mm_struct *oldmm,
> -					  struct mm_struct *mm)
> -{
> -	PVOP_VCALL2(mmu.dup_mmap, oldmm, mm);
> +	PVOP_VCALL1(mmu.enter_mmap, next);
>  }
>  
>  static inline int paravirt_pgd_alloc(struct mm_struct *mm)
> @@ -787,8 +780,7 @@ extern void default_banner(void);
>  
>  #ifndef __ASSEMBLY__
>  #ifndef CONFIG_PARAVIRT_XXL
> -static inline void paravirt_arch_dup_mmap(struct mm_struct *oldmm,
> -					  struct mm_struct *mm)
> +static inline void paravirt_enter_mmap(struct mm_struct *mm)
>  {
>  }
>  #endif
> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
> index 8c1da419260f..71bf64b963df 100644
> --- a/arch/x86/include/asm/paravirt_types.h
> +++ b/arch/x86/include/asm/paravirt_types.h
> @@ -164,11 +164,8 @@ struct pv_mmu_ops {
>  	unsigned long (*read_cr3)(void);
>  	void (*write_cr3)(unsigned long);
>  
> -	/* Hooks for intercepting the creation/use of an mm_struct. */
> -	void (*activate_mm)(struct mm_struct *prev,
> -			    struct mm_struct *next);
> -	void (*dup_mmap)(struct mm_struct *oldmm,
> -			 struct mm_struct *mm);
> +	/* Hook for intercepting the creation/use of an mm_struct. */
> +	void (*enter_mmap)(struct mm_struct *mm);
>  
>  	/* Hooks for allocating and freeing a pagetable top-level */
>  	int  (*pgd_alloc)(struct mm_struct *mm);
> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
> index 327757afb027..ff1109b9c6cd 100644
> --- a/arch/x86/kernel/paravirt.c
> +++ b/arch/x86/kernel/paravirt.c
> @@ -352,8 +352,7 @@ struct paravirt_patch_template pv_ops = {
>  	.mmu.make_pte		= PTE_IDENT,
>  	.mmu.make_pgd		= PTE_IDENT,
>  
> -	.mmu.dup_mmap		= paravirt_nop,
> -	.mmu.activate_mm	= paravirt_nop,
> +	.mmu.enter_mmap		= paravirt_nop,
>  
>  	.mmu.lazy_mode = {
>  		.enter		= paravirt_nop,
> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> index ee29fb558f2e..b3b8d289b9ab 100644
> --- a/arch/x86/xen/mmu_pv.c
> +++ b/arch/x86/xen/mmu_pv.c
> @@ -885,14 +885,7 @@ void xen_mm_unpin_all(void)
>  	spin_unlock(&pgd_lock);
>  }
>  
> -static void xen_activate_mm(struct mm_struct *prev, struct mm_struct *next)
> -{
> -	spin_lock(&next->page_table_lock);
> -	xen_pgd_pin(next);
> -	spin_unlock(&next->page_table_lock);
> -}
> -
> -static void xen_dup_mmap(struct mm_struct *oldmm, struct mm_struct *mm)
> +static void xen_enter_mmap(struct mm_struct *mm)
>  {
>  	spin_lock(&mm->page_table_lock);
>  	xen_pgd_pin(mm);
> @@ -2153,8 +2146,7 @@ static const typeof(pv_ops) xen_mmu_ops __initconst = {
>  		.make_p4d = PV_CALLEE_SAVE(xen_make_p4d),
>  #endif
>  
> -		.activate_mm = xen_activate_mm,
> -		.dup_mmap = xen_dup_mmap,
> +		.enter_mmap = xen_enter_mmap,
>  		.exit_mmap = xen_exit_mmap,
>  
>  		.lazy_mode = {
>
  

On 16.01.23 05:27, Srivatsa S. Bhat wrote:
> 
> Hi Juergen,
> 
> On 1/12/23 7:21 AM, Juergen Gross wrote:
>> The two paravirt callbacks .mmu.activate_mm and .mmu.dup_mmap are
>> sharing the same implementations in all cases: for Xen PV guests they
>> are pinning the PGD of the new mm_struct, and for all other cases
>> they are a NOP.
>>
> 
> I was expecting that the duplicated functions xen_activate_mm() and
> xen_dup_mmap() would be merged into a common one, and that both
> .mmu.activate_mm and .mmu.dup_mmap callbacks would be mapped to that
> common implementation for Xen PV.
> 
>> So merge them to a common callback .mmu.enter_mmap (in contrast to the
>> corresponding already existing .mmu.exit_mmap).
>>
> 
> Instead, this patch seems to be merging the callbacks themselves...
> 
> I see that's not an issue right now since there is no other actual
> user for these callbacks. But are we sure that merging the callbacks
> just because the current user (Xen PV) has the same implementation for
> both is a good idea? The callbacks are invoked at distinct points from
> fork/exec, so wouldn't it be valuable to retain that distinction in
> semantics in the callbacks as well?
> 
> However, if you believe that two separate callbacks are not really
> required here (because there is no significant difference in what they
> mean, rather than because their callback implementations happen to be
> the same right now), then could you please expand on this and call it
> out in the commit message, please?

Would you be fine with:

   In the end both callbacks are meant to register an address space with the
   underlying hypervisor, so there needs to be only a single callback for that
   purpose.


Juergen
  

On 1/15/23 10:43 PM, Juergen Gross wrote:
> On 16.01.23 05:27, Srivatsa S. Bhat wrote:
>>
>> Hi Juergen,
>>
>> On 1/12/23 7:21 AM, Juergen Gross wrote:
>>> The two paravirt callbacks .mmu.activate_mm and .mmu.dup_mmap are
>>> sharing the same implementations in all cases: for Xen PV guests they
>>> are pinning the PGD of the new mm_struct, and for all other cases
>>> they are a NOP.
>>>
>>
>> I was expecting that the duplicated functions xen_activate_mm() and
>> xen_dup_mmap() would be merged into a common one, and that both
>> .mmu.activate_mm and .mmu.dup_mmap callbacks would be mapped to that
>> common implementation for Xen PV.
>>
>>> So merge them to a common callback .mmu.enter_mmap (in contrast to the
>>> corresponding already existing .mmu.exit_mmap).
>>>
>>
>> Instead, this patch seems to be merging the callbacks themselves...
>>
>> I see that's not an issue right now since there is no other actual
>> user for these callbacks. But are we sure that merging the callbacks
>> just because the current user (Xen PV) has the same implementation for
>> both is a good idea? The callbacks are invoked at distinct points from
>> fork/exec, so wouldn't it be valuable to retain that distinction in
>> semantics in the callbacks as well?
>>
>> However, if you believe that two separate callbacks are not really
>> required here (because there is no significant difference in what they
>> mean, rather than because their callback implementations happen to be
>> the same right now), then could you please expand on this and call it
>> out in the commit message, please?
> 
> Would you be fine with:
> 
>   In the end both callbacks are meant to register an address space with the
>   underlying hypervisor, so there needs to be only a single callback for that
>   purpose.
> 

Sure, that looks good. Thank you!
 
Regards,
Srivatsa
VMware Photon OS
  

On Sun, Jan 15, 2023 at 08:27:50PM -0800, Srivatsa S. Bhat wrote:

> I see that's not an issue right now since there is no other actual
> user for these callbacks. But are we sure that merging the callbacks
> just because the current user (Xen PV) has the same implementation for
> both is a good idea?

IIRC the pv_ops are part of the PARAVIRT_ME_HARDER (also spelled as
_XXL) suite of ops and they are only to be used by Xen PV, no new users
of these must happen.

The moment we can drop Xen PV (hopes and dreams etc..) all these things
go in the bin right along with it.