Sean Christopherson <seanjc@xxxxxxxxxx> writes:
> 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;
> + }
With only one evmcs_touch_msr_bitmap() user in the tree, I think such
mixing of VMX and eVMCS is fine but honestly I like Alexandru's v3 more
as the "struct hv_enlightened_vmcs *" cast is the only thing which
"leaks" into VMX. Either way, no big deal)
>
> vmx->nested.force_msr_bitmap_recalc = true;
> }
>
> base-commit: 68bfbbf518a25856c2a3f07ea9d0c626f1b001fb
--
Vitaly
