| Message ID | 24d8b606-f74b-9367-d67e-e952838c7048@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] x86/PV: remove unnecessary toggle_guest_pt() overhead | expand |
On Tue, May 05, 2020 at 08:16:03AM +0200, Jan Beulich wrote: > While the mere updating of ->pv_cr3 and ->root_pgt_changed aren't overly > expensive (but still needed only for the toggle_guest_mode() path), the > effect of the latter on the exit-to-guest path is not insignificant. > Move the logic into toggle_guest_mode(), on the basis that > toggle_guest_pt() will always be invoked in pairs, yet we can't safely > undo the setting of root_pgt_changed during the second of these > invocations. I'm not sure if it would be worth to add a comment to note the intended usage of toggle_guest_pt is to fetch data from the kernel page tables when running in user mode. The one about using it in pairs is certainly fine. > While at it, add a comment ahead of toggle_guest_pt() to clarify its > intended usage. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 05/05/2020 07:16, Jan Beulich wrote: > While the mere updating of ->pv_cr3 and ->root_pgt_changed aren't overly > expensive (but still needed only for the toggle_guest_mode() path), the > effect of the latter on the exit-to-guest path is not insignificant. > Move the logic into toggle_guest_mode(), on the basis that > toggle_guest_pt() will always be invoked in pairs, yet we can't safely > undo the setting of root_pgt_changed during the second of these > invocations. > > While at it, add a comment ahead of toggle_guest_pt() to clarify its > intended usage. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> I'm still of the opinion that the commit message wants rewriting to get the important points across clearly. And those are that toggle_guest_pt() is called in pairs specifically to read kernel data structures when emulating a userspace action, and that this doesn't modify cr3 from the guests point of view, and therefore doesn't need the resync on exit-to-guest path. ~Andrew
On 21.05.2020 18:46, Andrew Cooper wrote: > On 05/05/2020 07:16, Jan Beulich wrote: >> While the mere updating of ->pv_cr3 and ->root_pgt_changed aren't overly >> expensive (but still needed only for the toggle_guest_mode() path), the >> effect of the latter on the exit-to-guest path is not insignificant. >> Move the logic into toggle_guest_mode(), on the basis that >> toggle_guest_pt() will always be invoked in pairs, yet we can't safely >> undo the setting of root_pgt_changed during the second of these >> invocations. >> >> While at it, add a comment ahead of toggle_guest_pt() to clarify its >> intended usage. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > I'm still of the opinion that the commit message wants rewriting to get > the important points across clearly. > > And those are that toggle_guest_pt() is called in pairs specifically to > read kernel data structures when emulating a userspace action, and that > this doesn't modify cr3 from the guests point of view, and therefore > doesn't need the resync on exit-to-guest path. Is this "toggle_guest_pt() is called in pairs, to read guest kernel data structures when emulating a guest userspace action. Hence this doesn't modify cr3 from the guest's point of view, and therefore doesn't need any resync on the exit-to-guest path. Therefore move the updating of ->pv_cr3 and ->root_pgt_changed into toggle_guest_mode(), since undoing the changes during the second of these invocations wouldn't be a safe thing to do." any better? Jan
On 22/05/2020 11:07, Jan Beulich wrote: > On 21.05.2020 18:46, Andrew Cooper wrote: >> On 05/05/2020 07:16, Jan Beulich wrote: >>> While the mere updating of ->pv_cr3 and ->root_pgt_changed aren't overly >>> expensive (but still needed only for the toggle_guest_mode() path), the >>> effect of the latter on the exit-to-guest path is not insignificant. >>> Move the logic into toggle_guest_mode(), on the basis that >>> toggle_guest_pt() will always be invoked in pairs, yet we can't safely >>> undo the setting of root_pgt_changed during the second of these >>> invocations. >>> >>> While at it, add a comment ahead of toggle_guest_pt() to clarify its >>> intended usage. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> I'm still of the opinion that the commit message wants rewriting to get >> the important points across clearly. >> >> And those are that toggle_guest_pt() is called in pairs specifically to >> read kernel data structures when emulating a userspace action, and that >> this doesn't modify cr3 from the guests point of view, and therefore >> doesn't need the resync on exit-to-guest path. > Is this > > "toggle_guest_pt() is called in pairs, to read guest kernel data > structures when emulating a guest userspace action. Hence this doesn't > modify cr3 from the guest's point of view, and therefore doesn't need > any resync on the exit-to-guest path. Therefore move the updating of > ->pv_cr3 and ->root_pgt_changed into toggle_guest_mode(), since undoing > the changes during the second of these invocations wouldn't be a safe > thing to do." > > any better? Yes - that will do. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/pv/domain.c +++ b/xen/arch/x86/pv/domain.c @@ -393,18 +393,10 @@ bool __init xpti_pcid_enabled(void) static void _toggle_guest_pt(struct vcpu *v) { - const struct domain *d = v->domain; - struct cpu_info *cpu_info = get_cpu_info(); unsigned long cr3; v->arch.flags ^= TF_kernel_mode; update_cr3(v); - if ( d->arch.pv.xpti ) - { - cpu_info->root_pgt_changed = true; - cpu_info->pv_cr3 = __pa(this_cpu(root_pgt)) | - (d->arch.pv.pcid ? get_pcid_bits(v, true) : 0); - } /* * Don't flush user global mappings from the TLB. Don't tick TLB clock. @@ -412,15 +404,11 @@ static void _toggle_guest_pt(struct vcpu * In shadow mode, though, update_cr3() may need to be accompanied by a * TLB flush (for just the incoming PCID), as the top level page table may * have changed behind our backs. To be on the safe side, suppress the - * no-flush unconditionally in this case. The XPTI CR3 write, if enabled, - * will then need to be a flushing one too. + * no-flush unconditionally in this case. */ cr3 = v->arch.cr3; - if ( shadow_mode_enabled(d) ) - { + if ( shadow_mode_enabled(v->domain) ) cr3 &= ~X86_CR3_NOFLUSH; - cpu_info->pv_cr3 &= ~X86_CR3_NOFLUSH; - } write_cr3(cr3); if ( !(v->arch.flags & TF_kernel_mode) ) @@ -436,6 +424,8 @@ static void _toggle_guest_pt(struct vcpu void toggle_guest_mode(struct vcpu *v) { + const struct domain *d = v->domain; + ASSERT(!is_pv_32bit_vcpu(v)); /* %fs/%gs bases can only be stale if WR{FS,GS}BASE are usable. */ @@ -449,8 +439,27 @@ void toggle_guest_mode(struct vcpu *v) asm volatile ( "swapgs" ); _toggle_guest_pt(v); + + if ( d->arch.pv.xpti ) + { + struct cpu_info *cpu_info = get_cpu_info(); + + cpu_info->root_pgt_changed = true; + cpu_info->pv_cr3 = __pa(this_cpu(root_pgt)) | + (d->arch.pv.pcid ? get_pcid_bits(v, true) : 0); + /* + * As in _toggle_guest_pt() the XPTI CR3 write needs to be a TLB- + * flushing one too for shadow mode guests. + */ + if ( shadow_mode_enabled(d) ) + cpu_info->pv_cr3 &= ~X86_CR3_NOFLUSH; + } } +/* + * Must be called in matching pairs without returning to guest context + * inbetween. + */ void toggle_guest_pt(struct vcpu *v) { if ( !is_pv_32bit_vcpu(v) )
While the mere updating of ->pv_cr3 and ->root_pgt_changed aren't overly expensive (but still needed only for the toggle_guest_mode() path), the effect of the latter on the exit-to-guest path is not insignificant. Move the logic into toggle_guest_mode(), on the basis that toggle_guest_pt() will always be invoked in pairs, yet we can't safely undo the setting of root_pgt_changed during the second of these invocations. While at it, add a comment ahead of toggle_guest_pt() to clarify its intended usage. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v3: Add comment ahead of toggle_guest_pt(). v2: Extend description.