Re: [PATCH 5.15.y 5/8] KVM: x86/mmu: pull call to drop_large_spte() into __link_shadow_page()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The newly introduced `__link_shadow_page` function declares a `cache` parameter that is never used.

Pre-existing issues:
- [High] `drop_large_spte` flushes the wrong guest physical address range when dropping a large SPTE, potentially leaving stale hardware TLB entries for the unmapped guest memory.
--

commit 9820244be6c2ac65ed2abfdc79a98053af6c7970
Author: Paolo Bonzini <pbonzini@redhat.com>

KVM: x86/mmu: pull call to drop_large_spte() into __link_shadow_page()

This patch consolidates the removal of existing large SPTEs by moving the
drop_large_spte() call from multiple caller sites directly into
__link_shadow_page(). It also updates kvm_mmu_get_child_sp() to return
-EEXIST if a non-large shadow page is already present, avoiding double
linking.

> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
[ ... ]
> -static void drop_large_spte(struct kvm_vcpu *vcpu, u64 *sptep)
> +static void drop_large_spte(struct kvm *kvm, u64 *sptep)
>  {
> -	if (__drop_large_spte(vcpu->kvm, sptep)) {
> -		struct kvm_mmu_page *sp = sptep_to_sp(sptep);
> +	struct kvm_mmu_page *sp;
>  
> -		kvm_flush_remote_tlbs_with_address(vcpu->kvm, sp->gfn,
> +	sp = sptep_to_sp(sptep);
> +	WARN_ON(sp->role.level == PG_LEVEL_4K);
>  
> -	}
> +	drop_spte(kvm, sptep);
> +	kvm_flush_remote_tlbs_with_address(kvm, sp->gfn,
> +			KVM_PAGES_PER_HPAGE(sp->role.level));
>  }

[Severity: High]
This is a pre-existing issue, but does drop_large_spte() flush the wrong
guest physical address range here?

By using sp->gfn, the flush targets the base GFN of the entire parent page
table, or the guest page directory itself for indirect mappings.

If the dropped SPTE is at an index greater than 0, wouldn't this flush the
0th index instead of the actual memory mapped by the dropped SPTE?

Could this leave stale hardware TLB entries for the unmapped guest memory
on environments supporting precise range-based TLB flushes? Should this
retrieve the exact GFN using the offset of sptep within sp->spt instead?

[ ... ]
> -static void link_shadow_page(struct kvm_vcpu *vcpu, u64 *sptep,
> -			     struct kvm_mmu_page *sp)
> +static void __link_shadow_page(struct kvm_vcpu *vcpu,
> +			       struct kvm_mmu_memory_cache *cache, u64 *sptep,
> +			       struct kvm_mmu_page *sp)

[Severity: Low]
Is the new cache parameter actually used in this function?

It appears the cache parameter is passed in but never referenced. The
underlying code seems to use the cache from the vcpu struct directly
when adding a parent PTE.

Could this parameter be removed to keep the signature clean?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626112606.1778248-1-pbonzini@redhat.com?part=5