On Mon, Jan 23, 2023, Sean Christopherson wrote:
> On Mon, Jan 23, 2023, Alexandru Matei wrote:
> > > .. or, alternatively, you can directly pass
> > > (struct hv_enlightened_vmcs *)vmx->vmcs01.vmcs as e.g. 'current_evmcs'
> > > and avoid the conversion here.
> >
> > OK, sounds good, I'll pass hv_enlightened_vmcs * directly.
>
> Passing the eVMCS is silly, if we're going to bleed eVMCS details into vmx.c then
> we should just commit and expose all details. For this feature specifically, KVM
> already handles the enabling in vmx.c / vmx_vcpu_create():
>
> if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs) &&
> (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
> struct hv_enlightened_vmcs *evmcs = (void *)vmx->vmcs01.vmcs;
>
> evmcs->hv_enlightenments_control.msr_bitmap = 1;
> }
>
> And if we handle this fully in vmx_msr_bitmap_l01_changed(), then there's no need
> for a comment explaining that the feature is only enabled for vmcs01.
Oh, and the sanity check on a null pointer also goes away.
> If we want to maintain better separate between VMX and Hyper-V code, then just make
> the helper non-inline in hyperv.c, modifying MSR bitmaps will never be a hot path
> for any sane guest.
>
> I don't think I have a strong preference either way. In a perfect world we'd keep
> Hyper-V code separate, but practically speaking I think trying to move everything
> into hyperv.c would result in far too many stubs and some weird function names.
>
> Side topic, we should really have a wrapper for static_branch_unlikely(&enable_evmcs)
> so that the static key can be defined iff CONFIG_HYPERV=y. I'll send a patch.
I.e.
---
arch/x86/kvm/vmx/hyperv.h | 11 -----------
arch/x86/kvm/vmx/vmx.c | 9 +++++++--
2 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kvm/vmx/hyperv.h b/arch/x86/kvm/vmx/hyperv.h
index ab08a9b9ab7d..bac614e40078 100644
--- a/arch/x86/kvm/vmx/hyperv.h
+++ b/arch/x86/kvm/vmx/hyperv.h
@@ -250,16 +250,6 @@ static inline u16 evmcs_read16(unsigned long field)
return *(u16 *)((char *)current_evmcs + offset);
}
-static inline void evmcs_touch_msr_bitmap(void)
-{
- if (unlikely(!current_evmcs))
- return;
-
- if (current_evmcs->hv_enlightenments_control.msr_bitmap)
- current_evmcs->hv_clean_fields &=
- ~HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP;
-}
-
static inline void evmcs_load(u64 phys_addr)
{
struct hv_vp_assist_page *vp_ap =
@@ -280,7 +270,6 @@ static inline u64 evmcs_read64(unsigned long field) { return 0; }
static inline u32 evmcs_read32(unsigned long field) { return 0; }
static inline u16 evmcs_read16(unsigned long field) { return 0; }
static inline void evmcs_load(u64 phys_addr) {}
-static inline void evmcs_touch_msr_bitmap(void) {}
#endif /* IS_ENABLED(CONFIG_HYPERV) */
#define EVMPTR_INVALID (-1ULL)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c788aa382611..ed4051b54412 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3936,8 +3936,13 @@ static void vmx_msr_bitmap_l01_changed(struct vcpu_vmx *vmx)
* 'Enlightened MSR Bitmap' feature L0 needs to know that MSR
* bitmap has changed.
*/
- if (static_branch_unlikely(&enable_evmcs))
- evmcs_touch_msr_bitmap();
+ if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs)) {
+ struct hv_enlightened_vmcs *evmcs = (void *)vmx->vmcs01.vmcs;
+
+ if (evmcs->hv_enlightenments_control.msr_bitmap)
+ evmcs->hv_clean_fields &=
+ ~HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP;
+ }
vmx->nested.force_msr_bitmap_recalc = true;
}
base-commit: 68bfbbf518a25856c2a3f07ea9d0c626f1b001fb
--
