Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Mon, 6 Nov 2023 11:01:07 -0800
In-Reply-To: <CALMp9eTQiom+0b5qPP_0u2tGqw9GcWbJVMNGeNZms8MTH8byuQ@mail.gmail.com>
Mime-Version: 1.0
References: <20231104000239.367005-1-seanjc@google.com> <20231104000239.367005-7-seanjc@google.com>
 <CALMp9eTQiom+0b5qPP_0u2tGqw9GcWbJVMNGeNZms8MTH8byuQ@mail.gmail.com>
Message-ID: <ZUk388PuvlcC8F2T@google.com>
Subject: Re: [PATCH v6 06/20] KVM: selftests: Add vcpu_set_cpuid_property() to
 set properties
From:   Sean Christopherson <seanjc@google.com>
To:     Jim Mattson <jmattson@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Kan Liang <kan.liang@linux.intel.com>,
        Dapeng Mi <dapeng1.mi@linux.intel.com>,
        Jinrong Liang <cloudliang@tencent.com>,
        Like Xu <likexu@tencent.com>,
        Aaron Lewis <aaronlewis@google.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Nov 04, 2023, Jim Mattson wrote:
> On Fri, Nov 3, 2023 at 5:02=E2=80=AFPM Sean Christopherson <seanjc@google=
.com> wrote:
> >
> > From: Jinrong Liang <cloudliang@tencent.com>
> >
> > Add vcpu_set_cpuid_property() helper function for setting properties, a=
nd
> > use it instead of open coding an equivalent for MAX_PHY_ADDR.  Future v=
PMU
> > testcases will also need to stuff various CPUID properties.
> >
> > Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> > Co-developed-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  .../testing/selftests/kvm/include/x86_64/processor.h |  4 +++-
> >  tools/testing/selftests/kvm/lib/x86_64/processor.c   | 12 +++++++++---
> >  .../kvm/x86_64/smaller_maxphyaddr_emulation_test.c   |  2 +-
> >  3 files changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/t=
ools/testing/selftests/kvm/include/x86_64/processor.h
> > index 25bc61dac5fb..a01931f7d954 100644
> > --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> > +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> > @@ -994,7 +994,9 @@ static inline void vcpu_set_cpuid(struct kvm_vcpu *=
vcpu)
> >         vcpu_ioctl(vcpu, KVM_GET_CPUID2, vcpu->cpuid);
> >  }
> >
> > -void vcpu_set_cpuid_maxphyaddr(struct kvm_vcpu *vcpu, uint8_t maxphyad=
dr);
> > +void vcpu_set_cpuid_property(struct kvm_vcpu *vcpu,
> > +                            struct kvm_x86_cpu_property property,
> > +                            uint32_t value);
> >
> >  void vcpu_clear_cpuid_entry(struct kvm_vcpu *vcpu, uint32_t function);
> >  void vcpu_set_or_clear_cpuid_feature(struct kvm_vcpu *vcpu,
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools=
/testing/selftests/kvm/lib/x86_64/processor.c
> > index d8288374078e..9e717bc6bd6d 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > @@ -752,11 +752,17 @@ void vcpu_init_cpuid(struct kvm_vcpu *vcpu, const=
 struct kvm_cpuid2 *cpuid)
> >         vcpu_set_cpuid(vcpu);
> >  }
> >
> > -void vcpu_set_cpuid_maxphyaddr(struct kvm_vcpu *vcpu, uint8_t maxphyad=
dr)
> > +void vcpu_set_cpuid_property(struct kvm_vcpu *vcpu,
> > +                            struct kvm_x86_cpu_property property,
> > +                            uint32_t value)
> >  {
> > -       struct kvm_cpuid_entry2 *entry =3D vcpu_get_cpuid_entry(vcpu, 0=
x80000008);
> > +       struct kvm_cpuid_entry2 *entry;
> > +
> > +       entry =3D __vcpu_get_cpuid_entry(vcpu, property.function, prope=
rty.index);
> > +
> > +       (&entry->eax)[property.reg] &=3D ~GENMASK(property.hi_bit, prop=
erty.lo_bit);
> > +       (&entry->eax)[property.reg] |=3D value << (property.lo_bit);
>=20
> What if 'value' is too large?
>=20
> Perhaps:
>          value <<=3D property.lo_bit;
>          TEST_ASSERT(!(value & ~GENMASK(property.hi_bit,
> property.lo_bit)), "value is too large");

Heh, if the mask is something like bits 31:24, this would miss the case whe=
re
shifting value would drop bits.=20

Rather than explicitly detecting edge cases, I think the simplest approach =
is to
assert that kvm_cpuid_property() reads back @value, e.g.

	struct kvm_cpuid_entry2 *entry;

	entry =3D __vcpu_get_cpuid_entry(vcpu, property.function, property.index);

	(&entry->eax)[property.reg] &=3D ~GENMASK(property.hi_bit, property.lo_bit=
);
	(&entry->eax)[property.reg] |=3D value << property.lo_bit;

	vcpu_set_cpuid(vcpu);

	/* Sanity check that @value doesn't exceed the bounds in any way. */
	TEST_ASSERT_EQ(kvm_cpuid_property(vcpu->cpuid, property), value);