Re: [PATCH 5.15.y v4 03/11] KVM: MMU: update comment on the number of page role combinations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] kvm: mmu: incorrect comment on page role bit count
--

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

KVM: MMU: update comment on the number of page role combinations

This commit updates the comment explaining the kvm_mmu_page_role bitmask
to clarify why certain bit combinations are redundant.

> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 710c9c87cdf2e..2c378064a1db9 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -292,19 +292,25 @@ struct kvm_kernel_irq_routing_entry;
>   * the number of unique SPs that can theoretically be created is 2^n, where n
>   * is the number of bits that are used to compute the role.
>   *
> - * But, even though there are 18 bits in the mask below, not all combinations
> - * of modes and flags are possible.  The maximum number of possible upper-level
> - * shadow pages for a single gfn is in the neighborhood of 2^13.
> + * But, even though there are 19 bits in the mask below, not all combinations
> + * of modes and flags are possible:
>   *
> - *   - invalid shadow pages are not accounted.
> - *   - level is effectively limited to four combinations, not 16 as the number
> - *     bits would imply, as 4k SPs are not tracked (allowed to go unsync).
> - *   - level is effectively unused for non-PAE paging because there is exactly
> - *     one upper level (see 4k SP exception above).
> - *   - quadrant is used only for non-PAE paging and is exclusive with
> - *     gpte_is_8_bytes.
> - *   - execonly and ad_disabled are used only for nested EPT, which makes it
> - *     exclusive with quadrant.
> + *   - invalid shadow pages are not accounted, so the bits are effectively 18
> + *
> + *   - quadrant will only be used if gpte_is_8_bytes=0 (non-PAE paging);
> + *     execonly and ad_disabled are only used for nested EPT which has
> + *     gpte_is_8_bytes=1.  Therefore, 2 bits are always unused.

[Severity: Low]
Is this updated comment accurate for the current struct definition? The text
claims there are 19 bits in the mask and references an execonly bit.

However, looking at the union kvm_mmu_page_role definition immediately
following this comment:

union kvm_mmu_page_role {
	u32 word;
	struct {
		unsigned level:4;
		unsigned gpte_is_8_bytes:1;
		unsigned quadrant:2;
		unsigned direct:1;
		unsigned access:3;
		unsigned invalid:1;
		unsigned efer_nx:1;
		unsigned cr0_wp:1;
		unsigned smep_andnot_wp:1;
		unsigned smap_andnot_wp:1;
		unsigned ad_disabled:1;
		unsigned guest_mode:1;
        ...

Summing these bitfields yields exactly 18 bits, and the execonly bit appears
to be missing, having been moved to kvm_mmu_extended_role.

Could the mathematical breakdown in the comment be adjusted to reflect the
actual 18 bits present in the structure?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928164427.4699-1-k@mgml.me?part=3