On Wed, Jun 04, 2025, Paolo Bonzini wrote:
> On 5/30/25 01:39, Sean Christopherson wrote:
> > diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> > index 47a36a9a7fe5..e432cd7a7889 100644
> > --- a/arch/x86/kvm/svm/svm.h
> > +++ b/arch/x86/kvm/svm/svm.h
> > @@ -628,6 +628,50 @@ static_assert(SVM_MSRS_PER_RANGE == 8192);
> > #define SVM_MSRPM_RANGE_1_BASE_MSR 0xc0000000
> > #define SVM_MSRPM_RANGE_2_BASE_MSR 0xc0010000
> > +#define SVM_MSRPM_FIRST_MSR(range_nr) \
> > + (SVM_MSRPM_RANGE_## range_nr ##_BASE_MSR)
> > +#define SVM_MSRPM_LAST_MSR(range_nr) \
> > + (SVM_MSRPM_RANGE_## range_nr ##_BASE_MSR + SVM_MSRS_PER_RANGE - 1)
> > +
> > +#define SVM_MSRPM_BIT_NR(range_nr, msr) \
> > + (range_nr * SVM_MSRPM_BYTES_PER_RANGE * BITS_PER_BYTE + \
> > + (msr - SVM_MSRPM_RANGE_## range_nr ##_BASE_MSR) * SVM_BITS_PER_MSR)
> > +
> > +#define SVM_MSRPM_SANITY_CHECK_BITS(range_nr) \
> > +static_assert(SVM_MSRPM_BIT_NR(range_nr, SVM_MSRPM_FIRST_MSR(range_nr) + 1) == \
> > + range_nr * 2048 * 8 + 2); \
> > +static_assert(SVM_MSRPM_BIT_NR(range_nr, SVM_MSRPM_FIRST_MSR(range_nr) + 7) == \
> > + range_nr * 2048 * 8 + 14);
> > +
> > +SVM_MSRPM_SANITY_CHECK_BITS(0);
> > +SVM_MSRPM_SANITY_CHECK_BITS(1);
> > +SVM_MSRPM_SANITY_CHECK_BITS(2);
>
> Replying here for patches 11/25/26. None of this is needed, just write a
> function like this:
>
> static inline u32 svm_msr_bit(u32 msr)
> {
> u32 msr_base = msr & ~(SVM_MSRS_PER_RANGE - 1);
Ooh, clever.
> if (msr_base == SVM_MSRPM_RANGE_0_BASE_MSR)
> return SVM_MSRPM_BIT_NR(0, msr);
> if (msr_base == SVM_MSRPM_RANGE_1_BASE_MSR)
> return SVM_MSRPM_BIT_NR(1, msr);
> if (msr_base == SVM_MSRPM_RANGE_2_BASE_MSR)
> return SVM_MSRPM_BIT_NR(2, msr);
> return MSR_INVALID;
I initially had something like this, but I don't like the potential for typos,
e.g. to fat finger something like:
if (msr_base == SVM_MSRPM_RANGE_2_BASE_MSR)
return SVM_MSRPM_BIT_NR(1, msr);
Which is how I ended up with the (admittedly ugly) CASE macros. Would you be ok
keeping that wrinkle? E.g.
#define SVM_MSR_BIT_NR_CASE(range_nr, msr) \
case SVM_MSRPM_RANGE_## range_nr ##_BASE_MSR: \
return range_nr * SVM_MSRPM_BYTES_PER_RANGE * BITS_PER_BYTE + \
(msr - SVM_MSRPM_RANGE_## range_nr ##_BASE_MSR) * SVM_BITS_PER_MSR);
static __always_inline int svm_msrpm_bit_nr(u32 msr)
{
switch (msr & ~(SVM_MSRS_PER_RANGE - 1)) {
SVM_BUILD_MSR_BITMAP_CASE(0, msr)
SVM_BUILD_MSR_BITMAP_CASE(1, msr)
SVM_BUILD_MSR_BITMAP_CASE(2, msr)
default:
return -EINVAL;
}
}
Actually, better idea! Hopefully. With your masking trick, there's no need to
do subtraction to get the offset within a range, which means getting the bit/byte
number for an MSR can be done entirely programmatically. And if we do that, then
the SVM_MSRPM_RANGE_xxx_BASE_MSR defines can go away, and the (very trivial)
copy+paste that I dislike also goes away.
Completely untested, but how about this?
#define SVM_MSRPM_OFFSET_MASK (SVM_MSRS_PER_RANGE - 1)
static __always_inline int svm_msrpm_bit_nr(u32 msr)
{
int range_nr;
switch (msr & ~SVM_MSRPM_OFFSET_MASK) {
case 0:
range_nr = 0;
break;
case 0xc0000000:
range_nr = 1;
break;
case 0xc0010000:
range_nr = 2;
break;
default:
return -EINVAL;
}
return range_nr * SVM_MSRPM_BYTES_PER_RANGE * BITS_PER_BYTE +
(msr & SVM_MSRPM_OFFSET_MASK) * SVM_BITS_PER_MSR)
}
static inline svm_msrpm_byte_nr(u32 msr)
{
return svm_msrpm_bit_nr(msr) / BITS_PER_BYTE;
}
The open coded literals aren't pretty, but VMX does the same thing, precisely
because I didn't want any code besides the innermost helper dealing with the
msr => offset math.
> }
>
> and you can throw away most of the other macros. For example:
>
> > +#define SVM_BUILD_MSR_BITMAP_CASE(bitmap, range_nr, msr, bitop, bit_rw) \
> > + case SVM_MSRPM_FIRST_MSR(range_nr) ... SVM_MSRPM_LAST_MSR(range_nr): \
> > + return bitop##_bit(SVM_MSRPM_BIT_NR(range_nr, msr) + bit_rw, bitmap);
>
> ... becomes a lot more lowercase:
>
> static inline rtype svm_##action##_msr_bitmap_##access(
> unsigned long *bitmap, u32 msr)
> {
> u32 bit = svm_msr_bit(msr);
> if (bit == MSR_INVALID)
> return true;
> return bitop##_bit(bit + bit_rw, bitmap);
Yeah, much cleaner.
> }
>
>
> In patch 25, also, you just get
>
> static u32 svm_msrpm_offset(u32 msr)
> {
> u32 bit = svm_msr_bit(msr);
> if (bit == MSR_INVALID)
> return MSR_INVALID;
> return bit / BITS_PER_BYTE;
> }
>
> And you change everything to -EINVAL in patch 26 to kill MSR_INVALID.
>
> Another nit...
>
> > +#define BUILD_SVM_MSR_BITMAP_HELPERS(ret_type, action, bitop) \
> > + __BUILD_SVM_MSR_BITMAP_HELPER(ret_type, action, bitop, read, 0) \
> > + __BUILD_SVM_MSR_BITMAP_HELPER(ret_type, action, bitop, write, 1)
> > +
> > +BUILD_SVM_MSR_BITMAP_HELPERS(bool, test, test)
> > +BUILD_SVM_MSR_BITMAP_HELPERS(void, clear, __clear)
> > +BUILD_SVM_MSR_BITMAP_HELPERS(void, set, __set)
> Yes it's a bit duplication, but no need for the nesting, just do:
I don't have a super strong preference, but I do want to be consistent between
VMX and SVM, and VMX has the nesting (unsurprisingly, also written by me). And
for that, the nested macros add a bit more value due to reads vs writes being in
entirely different areas of the bitmap.
#define BUILD_VMX_MSR_BITMAP_HELPERS(ret_type, action, bitop) \
__BUILD_VMX_MSR_BITMAP_HELPER(ret_type, action, bitop, read, 0x0) \
__BUILD_VMX_MSR_BITMAP_HELPER(ret_type, action, bitop, write, 0x800)
BUILD_VMX_MSR_BITMAP_HELPERS(bool, test, test)
BUILD_VMX_MSR_BITMAP_HELPERS(void, clear, __clear)
BUILD_VMX_MSR_BITMAP_HELPERS(void, set, __set)
That could be mitigated to some extent by adding a #define to communicate the
offset, but IMO the nested macros are less ugly than that.
> BUILD_SVM_MSR_BITMAP_HELPERS(bool, test, test, read, 0)
> BUILD_SVM_MSR_BITMAP_HELPERS(bool, test, test, write, 1)
> BUILD_SVM_MSR_BITMAP_HELPERS(void, clear, __clear, read, 0)
> BUILD_SVM_MSR_BITMAP_HELPERS(void, clear, __clear, write, 1)
> BUILD_SVM_MSR_BITMAP_HELPERS(void, set, __set, read, 0)
> BUILD_SVM_MSR_BITMAP_HELPERS(void, set, __set, write, 1)
>
> Otherwise, really nice.
>
> Paolo
>