lkml.org 
[lkml]   [2022]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 6/6] KVM: selftests: Test Hyper-V invariant TSC control
On Wed, Oct 12, 2022, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> 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 == ???;
}

\
 
 \ /
  Last update: 2022-10-12 18:53    [W:0.057 / U:0.300 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site