Sean Christopherson <seanjc@xxxxxxxxxx> writes:
> Bump the size of the sptes array by one and use the raw level of the
> SPTE to index into the sptes array. Using the SPTE level directly
> improves readability by eliminating the need to reason out why the level
> is being adjusted when indexing the array. The array is on the stack
> and is not explicitly initialized; bumping its size is nothing more than
> a superficial adjustment to the stack frame.
>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> arch/x86/kvm/mmu/mmu.c | 15 +++++++--------
> arch/x86/kvm/mmu/tdp_mmu.c | 2 +-
> 2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 52f36c879086..4798a4472066 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -3500,7 +3500,7 @@ static int get_walk(struct kvm_vcpu *vcpu, u64 addr, u64 *sptes, int *root_level
> leaf = iterator.level;
> spte = mmu_spte_get_lockless(iterator.sptep);
>
> - sptes[leaf - 1] = spte;
> + sptes[leaf] = spte;
>
> if (!is_shadow_present_pte(spte))
> break;
> @@ -3514,7 +3514,7 @@ static int get_walk(struct kvm_vcpu *vcpu, u64 addr, u64 *sptes, int *root_level
> /* return true if reserved bit is detected on spte. */
> static bool get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
> {
> - u64 sptes[PT64_ROOT_MAX_LEVEL];
> + u64 sptes[PT64_ROOT_MAX_LEVEL + 1];
> struct rsvd_bits_validate *rsvd_check;
> int root, leaf, level;
> bool reserved = false;
> @@ -3537,16 +3537,15 @@ static bool get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
> rsvd_check = &vcpu->arch.mmu->shadow_zero_check;
>
> for (level = root; level >= leaf; level--) {
> - if (!is_shadow_present_pte(sptes[level - 1]))
> + if (!is_shadow_present_pte(sptes[level]))
> break;
> /*
> * Use a bitwise-OR instead of a logical-OR to aggregate the
> * reserved bit and EPT's invalid memtype/XWR checks to avoid
> * adding a Jcc in the loop.
> */
> - reserved |= __is_bad_mt_xwr(rsvd_check, sptes[level - 1]) |
> - __is_rsvd_bits_set(rsvd_check, sptes[level - 1],
> - level);
> + reserved |= __is_bad_mt_xwr(rsvd_check, sptes[level]) |
> + __is_rsvd_bits_set(rsvd_check, sptes[level], level);
> }
>
> if (reserved) {
> @@ -3554,10 +3553,10 @@ static bool get_mmio_spte(struct kvm_vcpu *vcpu, u64 addr, u64 *sptep)
> __func__, addr);
> for (level = root; level >= leaf; level--)
> pr_err("------ spte 0x%llx level %d.\n",
> - sptes[level - 1], level);
> + sptes[level], level);
> }
>
> - *sptep = sptes[leaf - 1];
> + *sptep = sptes[leaf];
>
> return reserved;
> }
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index a4f9447f8327..efef571806ad 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -1160,7 +1160,7 @@ int kvm_tdp_mmu_get_walk(struct kvm_vcpu *vcpu, u64 addr, u64 *sptes,
>
> tdp_mmu_for_each_pte(iter, mmu, gfn, gfn + 1) {
> leaf = iter.level;
> - sptes[leaf - 1] = iter.old_spte;
> + sptes[leaf] = iter.old_spte;
> }
>
> return leaf;
An alretnitive solution would've been to reverse the array and fill it
like
sptes[PT64_ROOT_MAX_LEVEL - leaf] = iter.old_spte;
but this may not reach the goal of 'improved readability' :-)
Also, we may add an MMU_DEBUG ifdef-ed check that sptes[0] remains
intact.
Reviewed-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
--
Vitaly
