| Message ID | 20220209170020.1775368-7-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: MMU: do not unload MMU roots on all role changes | expand |
On Wed, Feb 09, 2022, Paolo Bonzini wrote: > The name of kvm_mmu_reload is very confusing for two reasons: > first, KVM_REQ_MMU_RELOAD actually does not call it; second, > it only does anything if there is no valid root. > > Rename it to kvm_mmu_ensure_valid_root, which matches the actual > behavior better. 100% agree that kvm_mmu_reload() is a terrible name, but kvm_mmu_ensure_valid_root() isn't much better, e.g. it sounds like a sanity check and nothing more. Maybe just be very literalal? kvm_mmu_load_if_necessary() kvm_mmu_load_if_invalid() Or follow cond_sched()? kvm_mmu_cond_load()
On 2/11/22 01:27, Sean Christopherson wrote: > On Wed, Feb 09, 2022, Paolo Bonzini wrote: >> The name of kvm_mmu_reload is very confusing for two reasons: >> first, KVM_REQ_MMU_RELOAD actually does not call it; second, >> it only does anything if there is no valid root. >> >> Rename it to kvm_mmu_ensure_valid_root, which matches the actual >> behavior better. > > 100% agree that kvm_mmu_reload() is a terrible name, but kvm_mmu_ensure_valid_root() > isn't much better, e.g. it sounds like a sanity check and nothing more. I would have thought that would be more of a check_valid_root(). There are other functions in the kernel following the idea that "ensure" means idempotency: skb_ensure_writable, perf_cgroup_ensure_storage, btf_ensure_modifiable and libbpf_ensure_mem in libbpf. I'm not a native speaker but, at least in computing, "ensure" seems to mean not just "to make certain that (something) will be true", but also taking steps if that's not already the case. I also thought of "establish_valid_root", but it has the opposite problem---it does not convey well, if at all, that the root could be valid already. Paolo > > Maybe just be very literalal? > > kvm_mmu_load_if_necessary() > kvm_mmu_load_if_invalid() > > Or follow cond_sched()? > > kvm_mmu_cond_load() >
On Fri, Feb 11, 2022, Paolo Bonzini wrote: > On 2/11/22 01:27, Sean Christopherson wrote: > > On Wed, Feb 09, 2022, Paolo Bonzini wrote: > > > The name of kvm_mmu_reload is very confusing for two reasons: > > > first, KVM_REQ_MMU_RELOAD actually does not call it; second, > > > it only does anything if there is no valid root. > > > > > > Rename it to kvm_mmu_ensure_valid_root, which matches the actual > > > behavior better. > > > > 100% agree that kvm_mmu_reload() is a terrible name, but kvm_mmu_ensure_valid_root() > > isn't much better, e.g. it sounds like a sanity check and nothing more. > > I would have thought that would be more of a check_valid_root(). There are > other functions in the kernel following the idea that "ensure" means > idempotency: skb_ensure_writable, perf_cgroup_ensure_storage, > btf_ensure_modifiable and libbpf_ensure_mem in libbpf. I'm not a native > speaker but, at least in computing, "ensure" seems to mean not just "to make > certain that (something) will be true", but also taking steps if that's not > already the case. There's no ambiguity on the "make certain that <x> will be true", it's the second part about taking steps that's ambiguous. Specifically, it doesn't convey any information about _what_ steps will be taken, e.g. the below implementation is also a possibility since it ensures the root is valid by preventing forward progress if the root is invalid. static inline int kvm_mmu_ensure_valid_root(struct kvm_vcpu *vcpu) { if (unlikely(vcpu->arch.mmu->root.hpa != INVALID_PAGE)) return -EFAULT; return 0; } Existing example of that interpretation are input_dev_ensure_poller() and rtnl_ensure_unique_netns(). The other nuance that I want to avoid is the implication that KVM is checking for a valid root because it doesn't trust what has happened before, i.e. that the call is there as a safeguard. That's misleading for the most common path, vcpu_enter_guest(), because when the helper does do some work, it's usually because KVM deliberately invalidated the root. > I also thought of "establish_valid_root", but it has the opposite > problem---it does not convey well, if at all, that the root could be valid > already. Heh, I agree that "establish" would imply the root is always invalid, but amusingly "establish" is listed as a synonym for "ensure" on the few sites of checked. Yay English. I was going to suggest we just open code it in vcpu_enter_guest, but async #PF uses it too :-/ Can we put this on the backburner for now? IMO, KVM_REQ_MMU_RELOAD is far more misleading than kvm_mmu_reload(), and I posted a series to remedy that (though I need to check if it's still viable since you vetoed adding the check for a pending request in the page fault handler). https://lore.kernel.org/all/20211209060552.2956723-1-seanjc@google.com
On 2/11/22 17:16, Sean Christopherson wrote: > The other nuance that I want to avoid is the implication that KVM is checking for > a valid root because it doesn't trust what has happened before, i.e. that the call > is there as a safeguard. That's misleading for the most common path, vcpu_enter_guest(), > because when the helper does do some work, it's usually because KVM deliberately > invalidated the root. Fair enough. >> I also thought of "establish_valid_root", but it has the opposite >> problem---it does not convey well, if at all, that the root could be valid >> already. > > Heh, I agree that "establish" would imply the root is always invalid, but amusingly > "establish" is listed as a synonym for "ensure" on the few sites of checked. Yay English. Well, synonyms rarely have a perfectly matching meaning. > Can we put this on the backburner for now? Yes, of course. Paolo > IMO, KVM_REQ_MMU_RELOAD is far more > misleading than kvm_mmu_reload(), and I posted a series to remedy that (though I > need to check if it's still viable since you vetoed adding the check for a pending > request in the page fault handler). > > https://lore.kernel.org/all/20211209060552.2956723-1-seanjc@google.com
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h index b9d06a218b2c..c9f1c2162ade 100644 --- a/arch/x86/kvm/mmu.h +++ b/arch/x86/kvm/mmu.h @@ -104,7 +104,7 @@ void kvm_mmu_unload(struct kvm_vcpu *vcpu); void kvm_mmu_sync_roots(struct kvm_vcpu *vcpu); void kvm_mmu_sync_prev_roots(struct kvm_vcpu *vcpu); -static inline int kvm_mmu_reload(struct kvm_vcpu *vcpu) +static inline int kvm_mmu_ensure_valid_root(struct kvm_vcpu *vcpu) { if (likely(vcpu->arch.mmu->root_hpa != INVALID_PAGE)) return 0; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 98aca0f2af12..2685fb62807e 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -9976,7 +9976,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) } } - r = kvm_mmu_reload(vcpu); + r = kvm_mmu_ensure_valid_root(vcpu); if (unlikely(r)) { goto cancel_injection; } @@ -12164,7 +12164,7 @@ void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, struct kvm_async_pf *work) work->wakeup_all) return; - r = kvm_mmu_reload(vcpu); + r = kvm_mmu_ensure_valid_root(vcpu); if (unlikely(r)) return;
The name of kvm_mmu_reload is very confusing for two reasons: first, KVM_REQ_MMU_RELOAD actually does not call it; second, it only does anything if there is no valid root. Rename it to kvm_mmu_ensure_valid_root, which matches the actual behavior better. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/mmu.h | 2 +- arch/x86/kvm/x86.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)