On 5/30/25 01:40, Sean Christopherson wrote:
> Access the MSRPM using u32/4-byte chunks (and appropriately adjusted
> offsets) only when merging L0 and L1 bitmaps as part of emulating VMRUN.
> The only reason to batch accesses to MSRPMs is to avoid the overhead of
> uaccess operations (e.g. STAC/CLAC and bounds checks) when reading L1's
> bitmap pointed at by vmcb12. For all other uses, either per-bit accesses
> are more than fast enough (no uaccess), or KVM is only accessing a single
> bit (nested_svm_exit_handled_msr()) and so there's nothing to batch.
>
> In addition to (hopefully) documenting the uniqueness of the merging code,
> restricting chunked access to _just_ the merging code will allow for
> increasing the chunk size (to unsigned long) with minimal risk.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/x86/kvm/svm/nested.c | 50 ++++++++++++++++-----------------------
> arch/x86/kvm/svm/svm.h | 18 ++++++++++----
> 2 files changed, 34 insertions(+), 34 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index e07e10fb52a5..a4e98ada732b 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -187,31 +187,19 @@ void recalc_intercepts(struct vcpu_svm *svm)
> static int nested_svm_msrpm_merge_offsets[6] __ro_after_init;
> static int nested_svm_nr_msrpm_merge_offsets __ro_after_init;
>
> -static const u32 msrpm_ranges[] = {
> - SVM_MSRPM_RANGE_0_BASE_MSR,
> - SVM_MSRPM_RANGE_1_BASE_MSR,
> - SVM_MSRPM_RANGE_2_BASE_MSR
> -};
> +#define SVM_BUILD_MSR_BYTE_NR_CASE(range_nr, msr) \
> + case SVM_MSRPM_FIRST_MSR(range_nr) ... SVM_MSRPM_LAST_MSR(range_nr): \
> + return SVM_MSRPM_BYTE_NR(range_nr, msr);
>
> static u32 svm_msrpm_offset(u32 msr)
> {
> - u32 offset;
> - int i;
> -
> - for (i = 0; i < ARRAY_SIZE(msrpm_ranges); i++) {
> - if (msr < msrpm_ranges[i] ||
> - msr >= msrpm_ranges[i] + SVM_MSRS_PER_RANGE)
> - continue;
> -
> - offset = (msr - msrpm_ranges[i]) / SVM_MSRS_PER_BYTE;
> - offset += (i * SVM_MSRPM_BYTES_PER_RANGE); /* add range offset */
> -
> - /* Now we have the u8 offset - but need the u32 offset */
> - return offset / 4;
> + switch (msr) {
> + SVM_BUILD_MSR_BYTE_NR_CASE(0, msr)
> + SVM_BUILD_MSR_BYTE_NR_CASE(1, msr)
> + SVM_BUILD_MSR_BYTE_NR_CASE(2, msr)
> + default:
> + return MSR_INVALID;
> }
> -
> - /* MSR not in any range */
> - return MSR_INVALID;
> }
>
> int __init nested_svm_init_msrpm_merge_offsets(void)
> @@ -245,6 +233,12 @@ int __init nested_svm_init_msrpm_merge_offsets(void)
> if (WARN_ON(offset == MSR_INVALID))
> return -EIO;
>
> + /*
> + * Merging is done in 32-bit chunks to reduce the number of
> + * accesses to L1's bitmap.
> + */
> + offset /= sizeof(u32);
> +
> for (j = 0; j < nested_svm_nr_msrpm_merge_offsets; j++) {
> if (nested_svm_msrpm_merge_offsets[j] == offset)
> break;
> @@ -1363,8 +1357,9 @@ void svm_leave_nested(struct kvm_vcpu *vcpu)
>
> static int nested_svm_exit_handled_msr(struct vcpu_svm *svm)
> {
> - u32 offset, msr, value;
> - int write, mask;
> + u32 offset, msr;
> + int write;
> + u8 value;
>
> if (!(vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
> return NESTED_EXIT_HOST;
> @@ -1372,18 +1367,15 @@ static int nested_svm_exit_handled_msr(struct vcpu_svm *svm)
> msr = svm->vcpu.arch.regs[VCPU_REGS_RCX];
> offset = svm_msrpm_offset(msr);
> write = svm->vmcb->control.exit_info_1 & 1;
> - mask = 1 << ((2 * (msr & 0xf)) + write);
This is wrong. The bit to read isn't always bit 0 or bit 1, therefore
mask needs to remain. But it can be written easily as:
msr = svm->vcpu.arch.regs[VCPU_REGS_RCX];
write = svm->vmcb->control.exit_info_1 & 1;
bit = svm_msrpm_bit(msr);
if (bit == MSR_INVALID)
return NESTED_EXIT_DONE;
offset = bit / BITS_PER_BYTE;
mask = BIT(write) << (bit & (BITS_PER_BYTE - 1));
and this even removes the need to use svm_msrpm_offset() in
nested_svm_exit_handled_msr().
At this point, it may even make sense to keep the adjustment for the
offset in svm_msrpm_offset(), like this:
static u32 svm_msrpm_offset(u32 msr)
{
u32 bit = svm_msr_bit(msr);
if (bit == MSR_INVALID)
return MSR_INVALID;
/*
* Merging is done in 32-bit chunks to reduce the number of
* accesses to L1's bitmap.
*/
return bit / (BITS_PER_BYTE * sizeof(u32));
}
I'll let you be the judge on this.
Paolo
> if (offset == MSR_INVALID)
> return NESTED_EXIT_DONE;
>
> - /* Offset is in 32 bit units but need in 8 bit units */
> - offset *= 4;
> -
> - if (kvm_vcpu_read_guest(&svm->vcpu, svm->nested.ctl.msrpm_base_pa + offset, &value, 4))
> + if (kvm_vcpu_read_guest(&svm->vcpu, svm->nested.ctl.msrpm_base_pa + offset,
> + &value, sizeof(value)))
> return NESTED_EXIT_DONE;
>
> - return (value & mask) ? NESTED_EXIT_DONE : NESTED_EXIT_HOST;
> + return (value & BIT(write)) ? NESTED_EXIT_DONE : NESTED_EXIT_HOST;
> }
>
> static int nested_svm_intercept_ioio(struct vcpu_svm *svm)
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 77287c870967..155b6089fcd2 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -634,15 +634,23 @@ static_assert(SVM_MSRS_PER_RANGE == 8192);
> (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) \
> +#define SVM_MSRPM_BYTE_NR(range_nr, msr) \
> + (range_nr * SVM_MSRPM_BYTES_PER_RANGE + \
> + (msr - SVM_MSRPM_RANGE_## range_nr ##_BASE_MSR) / SVM_MSRS_PER_BYTE)
> +
> +#define SVM_MSRPM_SANITY_CHECK_BITS_AND_BYTES(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);
> + range_nr * 2048 * 8 + 14); \
> +static_assert(SVM_MSRPM_BYTE_NR(range_nr, SVM_MSRPM_FIRST_MSR(range_nr) + 1) == \
> + range_nr * 2048); \
> +static_assert(SVM_MSRPM_BYTE_NR(range_nr, SVM_MSRPM_FIRST_MSR(range_nr) + 7) == \
> + range_nr * 2048 + 1);
>
> -SVM_MSRPM_SANITY_CHECK_BITS(0);
> -SVM_MSRPM_SANITY_CHECK_BITS(1);
> -SVM_MSRPM_SANITY_CHECK_BITS(2);
> +SVM_MSRPM_SANITY_CHECK_BITS_AND_BYTES(0);
> +SVM_MSRPM_SANITY_CHECK_BITS_AND_BYTES(1);
> +SVM_MSRPM_SANITY_CHECK_BITS_AND_BYTES(2);
>
> #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): \