On Mon, 2021-09-13 at 08:57 +0200, Vitaly Kuznetsov wrote:
> Maxim Levitsky <mlevitsk@xxxxxxxxxx> writes:
>
> > On Fri, 2021-09-10 at 18:06 +0200, Vitaly Kuznetsov wrote:
> > > Introduce a flag to keep track of whether MSR bitmap for L2 needs to be
> > > rebuilt due to changes in MSR bitmap for L1 or switching to a different
> > > L2. This information will be used for Enlightened MSR Bitmap feature for
> > > Hyper-V guests.
> > >
> > > Note, setting msr_bitmap_changed to 'true' from set_current_vmptr() is
> > > not really needed for Enlightened MSR Bitmap as the feature can only
> > > be used in conjunction with Enlightened VMCS but let's keep tracking
> > > information complete, it's cheap and in the future similar PV feature can
> > > easily be implemented for KVM on KVM too.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> > > ---
> > > arch/x86/kvm/vmx/nested.c | 9 ++++++++-
> > > arch/x86/kvm/vmx/vmx.c | 2 ++
> > > arch/x86/kvm/vmx/vmx.h | 6 ++++++
> > > 3 files changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > > index ccb03d69546c..42cd95611892 100644
> > > --- a/arch/x86/kvm/vmx/nested.c
> > > +++ b/arch/x86/kvm/vmx/nested.c
> > > @@ -2053,10 +2053,13 @@ static enum nested_evmptrld_status nested_vmx_handle_enlightened_vmptrld(
> > > * Clean fields data can't be used on VMLAUNCH and when we switch
> > > * between different L2 guests as KVM keeps a single VMCS12 per L1.
> > > */
> > > - if (from_launch || evmcs_gpa_changed)
> > > + if (from_launch || evmcs_gpa_changed) {
> > > vmx->nested.hv_evmcs->hv_clean_fields &=
> > > ~HV_VMX_ENLIGHTENED_CLEAN_FIELD_ALL;
> > >
> > > + vmx->nested.msr_bitmap_changed = true;
> > > + }
> > > +
> > > return EVMPTRLD_SUCCEEDED;
> > > }
> > >
> > > @@ -3240,6 +3243,8 @@ static bool nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
> > > else
> > > exec_controls_clearbit(vmx, CPU_BASED_USE_MSR_BITMAPS);
> > >
> > > + vmx->nested.msr_bitmap_changed = false;
> >
> > Very minor nitpick: Maybe I would put this into nested_vmx_prepare_msr_bitmap,
> > a bit closer to the action, but this is fine like this as well.
> >
>
> I don't have a strong preference here, can move it to nested_vmx_prepare_msr_bitmap().
>
> > > +
> > > return true;
> > > }
> > >
> > > @@ -5273,6 +5278,7 @@ static void set_current_vmptr(struct vcpu_vmx *vmx, gpa_t vmptr)
> > > vmx->nested.need_vmcs12_to_shadow_sync = true;
> > > }
> > > vmx->nested.dirty_vmcs12 = true;
> > > + vmx->nested.msr_bitmap_changed = true;
> > > }
> > >
> > > /* Emulate the VMPTRLD instruction */
> > > @@ -6393,6 +6399,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> > > goto error_guest_mode;
> > >
> > > vmx->nested.dirty_vmcs12 = true;
> > > + vmx->nested.msr_bitmap_changed = true;
> >
> > Is this needed? Setting the nested state should eventually trigger call to
> > nested_vmx_handle_enlightened_vmptrld.
> >
> >
>
> Strictly speaking - no (meaning that nothing is going to change if we
> just drop this hunk). My intention was to keep tracking information
> complete: after vmx_set_nested_state() we certainly need to re-build MSR
> Bitmap 02 and that's what 'msr_bitmap_changed' tracks. We can replace
> this with a comment if needed (but I'd slightly prefer to keep it -
> unless there's a reason not to).
Makes sense. let it be like that.
Best regards,
Maxim Levitsky
>
> > > ret = nested_vmx_enter_non_root_mode(vcpu, false);
> > > if (ret)
> > > goto error_guest_mode;
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index ad33032e8588..2dbfb5d838db 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -3734,6 +3734,8 @@ static void vmx_msr_bitmap_l01_changed(struct vcpu_vmx *vmx)
> > > */
> > > if (static_branch_unlikely(&enable_evmcs))
> > > evmcs_touch_msr_bitmap();
> > > +
> > > + vmx->nested.msr_bitmap_changed = true;
> > > }
> > >
> > > void vmx_disable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
> > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > > index 4858c5fd95f2..b6596fc2943a 100644
> > > --- a/arch/x86/kvm/vmx/vmx.h
> > > +++ b/arch/x86/kvm/vmx/vmx.h
> > > @@ -148,6 +148,12 @@ struct nested_vmx {
> > > bool need_vmcs12_to_shadow_sync;
> > > bool dirty_vmcs12;
> > >
> > > + /*
> > > + * Indicates whether MSR bitmap for L2 needs to be rebuilt due to
> > > + * changes in MSR bitmap for L1 or switching to a different L2.
> > > + */
> > > + bool msr_bitmap_changed;
> > > +
> > > /*
> > > * Indicates lazily loaded guest state has not yet been decached from
> > > * vmcs02.
> >
> > Best regards,
> > Maxim Levitsky
> >
