On Sat, Nov 04, 2023, Jim Mattson wrote:
> On Fri, Nov 3, 2023 at 5:02 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> >
> > From: Jinrong Liang <cloudliang@xxxxxxxxxxx>
> >
> > Add vcpu_set_cpuid_property() helper function for setting properties, and
> > use it instead of open coding an equivalent for MAX_PHY_ADDR. Future vPMU
> > testcases will also need to stuff various CPUID properties.
> >
> > Signed-off-by: Jinrong Liang <cloudliang@xxxxxxxxxxx>
> > Co-developed-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > ---
> > .../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/tools/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 maxphyaddr);
> > +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 maxphyaddr)
> > +void vcpu_set_cpuid_property(struct kvm_vcpu *vcpu,
> > + struct kvm_x86_cpu_property property,
> > + uint32_t value)
> > {
> > - struct kvm_cpuid_entry2 *entry = vcpu_get_cpuid_entry(vcpu, 0x80000008);
> > + struct kvm_cpuid_entry2 *entry;
> > +
> > + entry = __vcpu_get_cpuid_entry(vcpu, property.function, property.index);
> > +
> > + (&entry->eax)[property.reg] &= ~GENMASK(property.hi_bit, property.lo_bit);
> > + (&entry->eax)[property.reg] |= value << (property.lo_bit);
>
> What if 'value' is too large?
>
> Perhaps:
> value <<= 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 where
shifting value would drop bits.
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 = __vcpu_get_cpuid_entry(vcpu, property.function, property.index);
(&entry->eax)[property.reg] &= ~GENMASK(property.hi_bit, property.lo_bit);
(&entry->eax)[property.reg] |= 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);
