Re: [PATCH v4 6/6] KVM: selftests: Test Hyper-V invariant TSC control

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Wed, Oct 12, 2022, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@xxxxxxxxxx> writes:
> 
> > On Thu, Sep 22, 2022, Vitaly Kuznetsov wrote:
> >> diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> >> index d4bd18bc580d..18b44450dfb8 100644
> >> --- a/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> >> +++ b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> >> @@ -46,20 +46,33 @@ struct hcall_data {
> >>  
> >>  static void guest_msr(struct msr_data *msr)
> >>  {
> >> -	uint64_t ignored;
> >> +	uint64_t msr_val = 0;
> >>  	uint8_t vector;
> >>  
> >>  	GUEST_ASSERT(msr->idx);
> >>  
> >> -	if (!msr->write)
> >> -		vector = rdmsr_safe(msr->idx, &ignored);
> >> -	else
> >> +	if (!msr->write) {
> >> +		vector = rdmsr_safe(msr->idx, &msr_val);
> >
> > This is subtly going to do weird things if the RDMSR faults.  rdmsr_safe()
> > overwrites @val with whatever happens to be in EDX:EAX if the RDMSR faults, i.e.
> > this may yield garbage instead of '0'.  Arguably rdmsr_safe() is a bad API, but
> > at the same time the caller really shouldn't consume the result if RDMSR faults
> > (though aligning with the kernel is also valuable).
> >
> > Aha!  Idea.  Assuming none of the MSRs are write-only, what about adding a prep
> > patch to rework this code so that it verifies RDMSR returns what was written when
> > a fault didn't occur.
> >
> 
> There is at least one read-only MSR which comes to mind:
> HV_X64_MSR_EOI.

I assume s/read-only/write-only since it's EOI?

> Also, some of the MSRs don't preserve the written value,
> e.g. HV_X64_MSR_RESET which always reads as '0'.

Hrm, that's annoying.

> I do, however, like the additional check that RDMSR returns what was
> written to the MSR, we will just need an additional flag in 'struct
> msr_data' ('check_written_value' maybe?).

Rather than force the testcase to specify information that's intrinsic to the MSR,
what about adding helpers to communicate the types?  E.g.

        if (msr->write)
                vector = wrmsr_safe(msr->idx, msr->write_val);

        if (!vector && !is_write_only_msr(msr->idx))
                vector = rdmsr_safe(msr->idx, &msr_val);

        if (msr->fault_expected)
                GUEST_ASSERT_2(vector == GP_VECTOR, msr->idx, vector);
        else
                GUEST_ASSERT_2(!vector, msr->idx, vector);

	if (is_read_zero_msr(msr->idx))
		GUEST_ASSERT_2(msr_val == 0, msr_val, 0);
	else
		GUEST_ASSERT_2(msr_val == msr->write_val, msr_val, msr->write_val);

I think that'd make the code a bit less magical and easier to understand for folks
that aren't familiar with Hyper-V.  The number of special MSRs is likely very small,
so the helpers should be trivial, e.g.

static bool is_write_only_msr(uint32_t msr)
{
	return msr == HV_X64_MSR_EOI;
}

static bool is_read_zero_msr(uint32_t msr)
{
	return msr == HV_X64_MSR_RESET || msr == ???;
}