Re: [PATCH v2 01/10] KVM: selftests: Replace x86_page_size with PG_LEVEL_XX

From: Peter Xu <peterx@redhat.com>
To: David Matlack <dmatlack@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Ben Gardon <bgardon@google.com>,
	Sean Christopherson <seanjc@google.com>,
	Oliver Upton <oupton@google.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Andrew Jones <drjones@redhat.com>,
	"open list:KERNEL VIRTUAL MACHINE (KVM)" <kvm@vger.kernel.org>
Subject: Re: [PATCH v2 01/10] KVM: selftests: Replace x86_page_size with PG_LEVEL_XX
Date: Tue, 17 May 2022 16:26:54 -0400	[thread overview]
Message-ID: <YoQFDgS5mCDlIUoS@xz-m1.local> (raw)
In-Reply-To: <20220517190524.2202762-2-dmatlack@google.com>

On Tue, May 17, 2022 at 07:05:15PM +0000, David Matlack wrote:
> x86_page_size is an enum used to communicate the desired page size with
> which to map a range of memory. Under the hood they just encode the
> desired level at which to map the page. This ends up being clunky in a
> few ways:
> 
>  - The name suggests it encodes the size of the page rather than the
>    level.
>  - In other places in x86_64/processor.c we just use a raw int to encode
>    the level.
> 
> Simplify this by adopting the kernel style of PG_LEVEL_XX enums and pass
> around raw ints when referring to the level. This makes the code easier
> to understand since these macros are very common in KVM MMU code.
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  .../selftests/kvm/include/x86_64/processor.h  | 18 ++++++----
>  .../selftests/kvm/lib/x86_64/processor.c      | 33 ++++++++++---------
>  .../selftests/kvm/max_guest_memory_test.c     |  2 +-
>  .../selftests/kvm/x86_64/mmu_role_test.c      |  2 +-
>  4 files changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 37db341d4cc5..434a4f60f4d9 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -465,13 +465,19 @@ void vcpu_set_hv_cpuid(struct kvm_vm *vm, uint32_t vcpuid);
>  struct kvm_cpuid2 *vcpu_get_supported_hv_cpuid(struct kvm_vm *vm, uint32_t vcpuid);
>  void vm_xsave_req_perm(int bit);
>  
> -enum x86_page_size {
> -	X86_PAGE_SIZE_4K = 0,
> -	X86_PAGE_SIZE_2M,
> -	X86_PAGE_SIZE_1G,
> +enum pg_level {
> +	PG_LEVEL_NONE,
> +	PG_LEVEL_4K,
> +	PG_LEVEL_2M,
> +	PG_LEVEL_1G,
> +	PG_LEVEL_512G,
> +	PG_LEVEL_NUM
>  };

I still prefer PTE/PMD/PUD/... as I suggested, as that's how the kernel mm
handles these levels with arch-independent way across the kernel.  But
well.. I never fight hard on namings, because I know that's the major
complexity. :-)

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu