Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 1 Nov 2022 17:26:26 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Vitaly Kuznetsov <vkuznets@redhat.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Michael Kelley <mikelley@microsoft.com>,
        Siddharth Chandrasekaran <sidcha@amazon.de>,
        Yuan Yao <yuan.yao@linux.intel.com>,
        Maxim Levitsky <mlevitsk@redhat.com>,
        linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v13 45/48] KVM: selftests: Introduce rdmsr_from_l2() and
 use it for MSR-Bitmap tests
Message-ID: <Y2FWwkq1kv9uXLwt@google.com>
References: <20221101145426.251680-1-vkuznets@redhat.com>
 <20221101145426.251680-46-vkuznets@redhat.com>
 <Y2FFNO3Bu9Z3LtCW@google.com>
 <87bkpqskr2.fsf@ovpn-194-149.brq.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87bkpqskr2.fsf@ovpn-194-149.brq.redhat.com>
Precedence: bulk

On Tue, Nov 01, 2022, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Tue, Nov 01, 2022, Vitaly Kuznetsov wrote:
> >> Hyper-V MSR-Bitmap tests do RDMSR from L2 to exit to L1. While 'evmcs_test'
> >> correctly clobbers all GPRs (which are not preserved), 'hyperv_svm_test'
> >> does not. Introduce and use common rdmsr_from_l2() to avoid code
> >> duplication and remove hardcoding of MSRs.
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> ---
> >>  .../selftests/kvm/include/x86_64/processor.h  |  9 +++++++
> >>  .../testing/selftests/kvm/x86_64/evmcs_test.c | 24 ++++---------------
> >>  .../selftests/kvm/x86_64/hyperv_svm_test.c    |  8 +++----
> >>  3 files changed, 17 insertions(+), 24 deletions(-)
> >> 
> >> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> >> index fbaf0b6cec4b..a14b7e4ea7c4 100644
> >> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> >> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> >> @@ -520,6 +520,15 @@ static inline void cpu_relax(void)
> >>  		"hlt\n"	\
> >>  		)
> >>  
> >> +/* Exit to L1 from L2 with RDMSR instruction */
> >> +static inline void rdmsr_from_l2(uint32_t msr)
> >
> > I would prefer keeping this helper out of common x86-64 code, even if it means
> > duplicating code across multiple Hyper-V tests until the L1 VM-Enter/VM-Exit
> > sequences get cleaned up.  The name is misleading, e.g. it doesn't really read
> > the MSR since there are no outputs
> 
> It's somewhat similar to vmcall()/vmmcall() which are only used to exit
> from L2 to L1 (and thus nobody complained that all the register values
> are random) and not issue a hypercall and return some value.

Sort of.  VMCALL/VMMCALL are unique in that they have no meaning (ignoring VMX's
STM) other than what is given to them by the hypervisor/software on VM-Exit.  RDMSR
on the other hand (and literally every other instruction), has architecturally
defined behavior and thus expectations beyond generating a VM-Exit.

I do think we should clean up the VMCALL/VMMCALL code to remove the clobbers
if/when the VM-Enter/VM-Exit sequences are fixed, and maybe make them more generic,
e.g. to allow reusing helpers for L1 and L2.  But, because the meaning of VMCALL/VMMCALL
is software-defined, we'll always need a selftests specific L2=>L1 hypercall,
e.g. to ensure L0 forwards the hypercall to L1.