* [PATCH] x86/paravirt: merge activate_mm and dup_mmap callbacks @ 2023-01-12 15:21 Juergen Gross via Virtualization 2023-01-12 19:44 ` Boris Ostrovsky 2023-01-16 4:27 ` Srivatsa S. Bhat 0 siblings, 2 replies; 6+ messages in thread From: Juergen Gross via Virtualization @ 2023-01-12 15:21 UTC (permalink / raw) To: linux-kernel, x86, virtualization Cc: Juergen Gross, Alexey Makhalov, VMware PV-Drivers Reviewers, Boris Ostrovsky, Dave Hansen, Ingo Molnar, Borislav Petkov, H. Peter Anvin, xen-devel, Thomas Gleixner 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(-) 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 = { -- 2.35.3 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/paravirt: merge activate_mm and dup_mmap callbacks 2023-01-12 15:21 [PATCH] x86/paravirt: merge activate_mm and dup_mmap callbacks Juergen Gross via Virtualization @ 2023-01-12 19:44 ` Boris Ostrovsky 2023-01-16 4:27 ` Srivatsa S. Bhat 1 sibling, 0 replies; 6+ messages in thread From: Boris Ostrovsky @ 2023-01-12 19:44 UTC (permalink / raw) To: Juergen Gross, linux-kernel, x86, virtualization Cc: Alexey Makhalov, VMware PV-Drivers Reviewers, Dave Hansen, Ingo Molnar, Borislav Petkov, H. Peter Anvin, xen-devel, Thomas Gleixner 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> _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/paravirt: merge activate_mm and dup_mmap callbacks 2023-01-12 15:21 [PATCH] x86/paravirt: merge activate_mm and dup_mmap callbacks Juergen Gross via Virtualization 2023-01-12 19:44 ` Boris Ostrovsky @ 2023-01-16 4:27 ` Srivatsa S. Bhat 2023-01-16 6:43 ` Juergen Gross via Virtualization 2023-01-17 12:22 ` Peter Zijlstra 1 sibling, 2 replies; 6+ messages in thread From: Srivatsa S. Bhat @ 2023-01-16 4:27 UTC (permalink / raw) To: Juergen Gross, linux-kernel, x86, virtualization Cc: H. Peter Anvin, VMware PV-Drivers Reviewers, Dave Hansen, Ingo Molnar, Borislav Petkov, Alexey Makhalov, xen-devel, Thomas Gleixner, Boris Ostrovsky 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 = { > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/paravirt: merge activate_mm and dup_mmap callbacks 2023-01-16 4:27 ` Srivatsa S. Bhat @ 2023-01-16 6:43 ` Juergen Gross via Virtualization 2023-01-16 16:10 ` Srivatsa S. Bhat 2023-01-17 12:22 ` Peter Zijlstra 1 sibling, 1 reply; 6+ messages in thread From: Juergen Gross via Virtualization @ 2023-01-16 6:43 UTC (permalink / raw) To: Srivatsa S. Bhat, linux-kernel, x86, virtualization Cc: H. Peter Anvin, VMware PV-Drivers Reviewers, Dave Hansen, Ingo Molnar, Borislav Petkov, Alexey Makhalov, xen-devel, Thomas Gleixner, Boris Ostrovsky [-- Attachment #1.1.1.1: Type: text/plain, Size: 1759 bytes --] 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 [-- Attachment #1.1.1.2: OpenPGP public key --] [-- Type: application/pgp-keys, Size: 3149 bytes --] [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 495 bytes --] [-- Attachment #2: Type: text/plain, Size: 183 bytes --] _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/paravirt: merge activate_mm and dup_mmap callbacks 2023-01-16 6:43 ` Juergen Gross via Virtualization @ 2023-01-16 16:10 ` Srivatsa S. Bhat 0 siblings, 0 replies; 6+ messages in thread From: Srivatsa S. Bhat @ 2023-01-16 16:10 UTC (permalink / raw) To: Juergen Gross, linux-kernel, x86, virtualization Cc: H. Peter Anvin, VMware PV-Drivers Reviewers, Dave Hansen, Ingo Molnar, Borislav Petkov, Alexey Makhalov, xen-devel, Thomas Gleixner, Boris Ostrovsky 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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/paravirt: merge activate_mm and dup_mmap callbacks 2023-01-16 4:27 ` Srivatsa S. Bhat 2023-01-16 6:43 ` Juergen Gross via Virtualization @ 2023-01-17 12:22 ` Peter Zijlstra 1 sibling, 0 replies; 6+ messages in thread From: Peter Zijlstra @ 2023-01-17 12:22 UTC (permalink / raw) To: Srivatsa S. Bhat Cc: Juergen Gross, Dave Hansen, Alexey Makhalov, VMware PV-Drivers Reviewers, x86, linux-kernel, virtualization, Ingo Molnar, Borislav Petkov, H. Peter Anvin, xen-devel, Thomas Gleixner, Boris Ostrovsky 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. _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-17 12:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-01-12 15:21 [PATCH] x86/paravirt: merge activate_mm and dup_mmap callbacks Juergen Gross via Virtualization 2023-01-12 19:44 ` Boris Ostrovsky 2023-01-16 4:27 ` Srivatsa S. Bhat 2023-01-16 6:43 ` Juergen Gross via Virtualization 2023-01-16 16:10 ` Srivatsa S. Bhat 2023-01-17 12:22 ` Peter Zijlstra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox