Re: [PATCH v4 2/6] KVM: x86: nSVM: implement nested LBR virtualization

[Maxim Levitsky <mlevitsk@redhat.com>]

On Thu, 2022-03-24 at 19:21 +0100, Paolo Bonzini wrote:
> On 3/22/22 18:40, Maxim Levitsky wrote:
> > +		/* Copy LBR related registers from vmcb12,
> > +		 * but make sure that we only pick LBR enable bit from the guest.
> > +		 */
> > +		svm_copy_lbrs(vmcb02, vmcb12);
> > +		vmcb02->save.dbgctl &= LBR_CTL_ENABLE_MASK;
> 
> I still do not understand why it is not copying all bits outside
> DEBUGCTL_RESERVED_BITS.  That is:

Honestly, you are right, I'll do this.
 
Note however about few issues that we have around MSR_IA32_DEBUGCTLMSR
which needs to be eventually fixed (and if I get to it first, I'll do this):
 

On SVM:
 
- without LBR virtualization supported (!lbrv) 
any attempt to set that msr is ignored and logged with pr_err_ratelimited.
 
Note that on AMD, MSR_IA32_DEBUGCTLMSR consists of:
 
bit 0 - 
     AMD's LBR bit
 
bit 1 - 
     BTF - when set, EFLAGS.TF flag causes debug exception
     only on control flow instructions, allowing you to do more efficient
     debugger controlled run of code under debug.
 
bit 2-5:
    exposes perf counters on external CPU pins. Very likely NOP
    on anything remotely modern.
 
- with LBR virtualization supported, the guest can set this msr to any value
as long as it doesn't set reserved bits and then read back the written value, 
but it is not used by the CPU, unless LBR bit is set in MSR_IA32_DEBUGCTLMSR, 
because only then LBR virtualization is enabled, which makes the CPU 
load the guest value on VM entry.
 
This means that MSR_IA32_DEBUGCTLMSR.BTF will magically start working when
MSR_IA32_DEBUGCTLMSR.LBR is set as well, and will not work otherwise.
 
On VMX, we also have something a bit related (but I didn't do any homework
on this):
 
If both LBR and BTF are set, they are both cleared and 
we also get vcpu_unimpl ratelimited message.

otherwise the value is written to GUEST_IA32_DEBUGCTL which I 
think isn't tied to LBR virtualization like on AMD 

(also intel's LBR implementation is much more useful, 
since it has multiple records).


So since the only bit in question is BTF, I was thinking, 
lets just pick the LBR bit.

But I have absolutely no issue to be bug-consistent with non 
nested treatment of MSR_IA32_DEBUGCTLMSR and passthrough all 
non reserved bits.

Or I might at least document this in the errata document you added
recently to KVM (which is a great idea).

Best regards,
	Maxim Levitsky

> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index c1baa3a68ce6..f1332d802ec8 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -589,11 +589,12 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
>   	}
>   
>   	if (unlikely(svm->lbrv_enabled && (svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK))) {
> -		/* Copy LBR related registers from vmcb12,
> -		 * but make sure that we only pick LBR enable bit from the guest.
> +		/*
> +		 * Reserved bits of DEBUGCTL are ignored.  Be consistent with
> +		 * svm_set_msr's definition of reserved bits.
>   		 */
>   		svm_copy_lbrs(vmcb02, vmcb12);
> -		vmcb02->save.dbgctl &= LBR_CTL_ENABLE_MASK;
> +		vmcb02->save.dbgctl &= ~DEBUGCTL_RESERVED_BITS;
>   		svm_update_lbrv(&svm->vcpu);
>   
>   	} else if (unlikely(vmcb01->control.virt_ext & LBR_CTL_ENABLE_MASK)) {
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 54fa048069b2..a6282be4e419 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -62,8 +62,6 @@ MODULE_DEVICE_TABLE(x86cpu, svm_cpu_id);
>   #define SEG_TYPE_LDT 2
>   #define SEG_TYPE_BUSY_TSS16 3
>   
> -#define DEBUGCTL_RESERVED_BITS (~(0x3fULL))
> -
>   static bool erratum_383_found __read_mostly;
>   
>   u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index cade032520cb..b687393e86ad 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -487,6 +487,8 @@ static inline bool nested_npt_enabled(struct vcpu_svm *svm)
>   /* svm.c */
>   #define MSR_INVALID				0xffffffffU
>   
> +#define DEBUGCTL_RESERVED_BITS (~(0x3fULL))
> +
>   extern bool dump_invalid_vmcb;
>   
>   u32 svm_msrpm_offset(u32 msr);
> 
> 
> > +		svm_update_lbrv(&svm->vcpu);
> > +
> > +	} else if (unlikely(vmcb01->control.virt_ext & LBR_CTL_ENABLE_MASK)) {
> >   		svm_copy_lbrs(vmcb02, vmcb01);