| Message ID | 20220921152436.3673454-15-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: hyper-v: Fine-grained TLB flush + L2 TLB flush features | expand |
On Wed, Sep 21, 2022, Vitaly Kuznetsov wrote: > Similar to nSVM, KVM needs to know L2's VM_ID/VP_ID and Partition > assist page address to handle L2 TLB flush requests. > > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/kvm/svm/hyperv.h | 16 ++++++++++++++++ > arch/x86/kvm/svm/nested.c | 2 ++ > 2 files changed, 18 insertions(+) > > diff --git a/arch/x86/kvm/svm/hyperv.h b/arch/x86/kvm/svm/hyperv.h > index 7d6d97968fb9..8cf702fed7e5 100644 > --- a/arch/x86/kvm/svm/hyperv.h > +++ b/arch/x86/kvm/svm/hyperv.h > @@ -9,6 +9,7 @@ > #include <asm/mshyperv.h> > > #include "../hyperv.h" > +#include "svm.h" > > /* > * Hyper-V uses the software reserved 32 bytes in VMCB > @@ -32,4 +33,19 @@ struct hv_enlightenments { > */ > #define VMCB_HV_NESTED_ENLIGHTENMENTS VMCB_SW > > +static inline void nested_svm_hv_update_vm_vp_ids(struct kvm_vcpu *vcpu) > +{ > + struct vcpu_svm *svm = to_svm(vcpu); > + struct hv_enlightenments *hve = > + (struct hv_enlightenments *)svm->nested.ctl.reserved_sw; Eww :-) I posted a small series to fix the casting[*], and as noted in the cover letter it's going to conflict mightily. Ignoring merge order for the moment, looking at the series as a whole, if the Hyper-V definitions are moved to hyperv-tlfs.h, then I'm tempted to say there's no need for svm/hyperv.h. There should never be users of this stuff outside of svm/nested.c, and IMO there's not enough stuff to warrant a separate set of files. nested_svm_hv_update_vp_assist() isn't SVM specific and fits better alongside kvm_hv_get_assist_page(). That leaves three functions and ~40 lines of code, which can easily go directly into svm/nested.c. I'm definitely not dead set against having hyperv.{ch}, but unless there's a high probability of SVM+Hyper-V getting to eVMCS levels of enlightenment, my vote is to put these helpers in svm/nested.c and move then if/when we do end up accumulating more SVM+Hyper-V code. As for merge order, I don't think there's a need for this series to take a dependency on the cleanup, especially if these helpers land in nested.c. Fixing up the casting and s/hv_enlightenments/hv_vmcb_enlightenments is straightforward. [*] https://lore.kernel.org/all/20220921201607.3156750-1-seanjc@google.com
Sean Christopherson <seanjc@google.com> writes: > On Wed, Sep 21, 2022, Vitaly Kuznetsov wrote: >> Similar to nSVM, KVM needs to know L2's VM_ID/VP_ID and Partition >> assist page address to handle L2 TLB flush requests. >> >> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> --- >> arch/x86/kvm/svm/hyperv.h | 16 ++++++++++++++++ >> arch/x86/kvm/svm/nested.c | 2 ++ >> 2 files changed, 18 insertions(+) >> >> diff --git a/arch/x86/kvm/svm/hyperv.h b/arch/x86/kvm/svm/hyperv.h >> index 7d6d97968fb9..8cf702fed7e5 100644 >> --- a/arch/x86/kvm/svm/hyperv.h >> +++ b/arch/x86/kvm/svm/hyperv.h >> @@ -9,6 +9,7 @@ >> #include <asm/mshyperv.h> >> >> #include "../hyperv.h" >> +#include "svm.h" >> >> /* >> * Hyper-V uses the software reserved 32 bytes in VMCB >> @@ -32,4 +33,19 @@ struct hv_enlightenments { >> */ >> #define VMCB_HV_NESTED_ENLIGHTENMENTS VMCB_SW >> >> +static inline void nested_svm_hv_update_vm_vp_ids(struct kvm_vcpu *vcpu) >> +{ >> + struct vcpu_svm *svm = to_svm(vcpu); >> + struct hv_enlightenments *hve = >> + (struct hv_enlightenments *)svm->nested.ctl.reserved_sw; > > Eww :-) > > I posted a small series to fix the casting[*], and as noted in the cover letter it's > going to conflict mightily. Ignoring merge order for the moment, looking at the > series as a whole, if the Hyper-V definitions are moved to hyperv-tlfs.h, then I'm > tempted to say there's no need for svm/hyperv.h. > > There should never be users of this stuff outside of svm/nested.c, and IMO there's > not enough stuff to warrant a separate set of files. nested_svm_hv_update_vp_assist() > isn't SVM specific and fits better alongside kvm_hv_get_assist_page(). > > That leaves three functions and ~40 lines of code, which can easily go directly > into svm/nested.c. > > I'm definitely not dead set against having hyperv.{ch}, but unless there's a high > probability of SVM+Hyper-V getting to eVMCS levels of enlightenment, my vote is > to put these helpers in svm/nested.c and move then if/when we do end up accumulating > more SVM+Hyper-V code. Well, there's more on the TODO list :-) There are even nSVM-only features like "enlightened TLB" (to split ASID invalidations into two stages) so I don't want to pollute 'nested.c'. In fact, I was thinking about renaming vmx/evmcs.{ch} into vmx/hyperv.{ch} as we're doing more than eVMCS there already. Also, having separate files help with the newly introduces 'KVM X86 HYPER-V (KVM/hyper-v)' MAINTAINERS entry. Does this sound like a good enough justification for keeping hyperv.{ch}? > > As for merge order, I don't think there's a need for this series to take a > dependency on the cleanup, especially if these helpers land in nested.c. Fixing > up the casting and s/hv_enlightenments/hv_vmcb_enlightenments is straightforward. > > [*] https://lore.kernel.org/all/20220921201607.3156750-1-seanjc@google.com > I'll take a look, thanks!
On Thu, Sep 22, 2022, Vitaly Kuznetsov wrote: > Sean Christopherson <seanjc@google.com> writes: > > I'm definitely not dead set against having hyperv.{ch}, but unless there's a high > > probability of SVM+Hyper-V getting to eVMCS levels of enlightenment, my vote is > > to put these helpers in svm/nested.c and move then if/when we do end up accumulating > > more SVM+Hyper-V code. > > Well, there's more on the TODO list :-) There are even nSVM-only > features like "enlightened TLB" (to split ASID invalidations into two > stages) so I don't want to pollute 'nested.c'. In fact, I was thinking > about renaming vmx/evmcs.{ch} into vmx/hyperv.{ch} as we're doing more > than eVMCS there already. Also, having separate files help with the > newly introduces 'KVM X86 HYPER-V (KVM/hyper-v)' MAINTAINERS entry. Ya, there is that. > Does this sound like a good enough justification for keeping hyperv.{ch}? Your call, I'm totally ok either way. If we do add svm/hyperv.{ch}, my vote is to also rename vmx/evmcs.{ch} as you suggested. I like symmetry :-)
diff --git a/arch/x86/kvm/svm/hyperv.h b/arch/x86/kvm/svm/hyperv.h index 7d6d97968fb9..8cf702fed7e5 100644 --- a/arch/x86/kvm/svm/hyperv.h +++ b/arch/x86/kvm/svm/hyperv.h @@ -9,6 +9,7 @@ #include <asm/mshyperv.h> #include "../hyperv.h" +#include "svm.h" /* * Hyper-V uses the software reserved 32 bytes in VMCB @@ -32,4 +33,19 @@ struct hv_enlightenments { */ #define VMCB_HV_NESTED_ENLIGHTENMENTS VMCB_SW +static inline void nested_svm_hv_update_vm_vp_ids(struct kvm_vcpu *vcpu) +{ + struct vcpu_svm *svm = to_svm(vcpu); + struct hv_enlightenments *hve = + (struct hv_enlightenments *)svm->nested.ctl.reserved_sw; + struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu); + + if (!hv_vcpu) + return; + + hv_vcpu->nested.pa_page_gpa = hve->partition_assist_page; + hv_vcpu->nested.vm_id = hve->hv_vm_id; + hv_vcpu->nested.vp_id = hve->hv_vp_id; +} + #endif /* __ARCH_X86_KVM_SVM_HYPERV_H__ */ diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index 4c620999d230..9fd75d45b31b 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -800,6 +800,8 @@ int enter_svm_guest_mode(struct kvm_vcpu *vcpu, u64 vmcb12_gpa, if (kvm_vcpu_apicv_active(vcpu)) kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu); + nested_svm_hv_update_vm_vp_ids(vcpu); + return 0; }