Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Jun 2023 14:48:26 -0700
In-Reply-To: <20230608113420.14695-2-cloudliang@tencent.com>
Mime-Version: 1.0
References: <20230608113420.14695-1-cloudliang@tencent.com> <20230608113420.14695-2-cloudliang@tencent.com>
Message-ID: <ZJyqqg/UW3m0EHmB@google.com>
Subject: Re: [PATCH 1/2] KVM: selftests: Test consistency of setting MSR_IA32_DS_AREA
From:   Sean Christopherson <seanjc@google.com>
To:     Jinrong Liang <ljr.kernel@gmail.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Like Xu <like.xu.linux@gmail.com>,
        Jinrong Liang <cloudliang@tencent.com>,
        linux-kselftest@vger.kernel.org, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Thu, Jun 08, 2023, Jinrong Liang wrote:
> From: Jinrong Liang <cloudliang@tencent.com>
> 
> Tests have been added to this commit to check if setting
> MSR_IA32_DS_AREA with a non-classical address causes a fault. By
> verifying that KVM is correctly faulting non-classical addresses
> in MSR_IA32_DS_AREA, it helps ensure the accuracy and stability
> of the KVM subsystem.
> 
> Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> ---
>  .../selftests/kvm/x86_64/vmx_pmu_caps_test.c  | 100 ++++++++++++++++++
>  1 file changed, 100 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
> index 4c90f76930f9..02903084598f 100644
> --- a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
> @@ -19,6 +19,9 @@
>  #include "kvm_util.h"
>  #include "vmx.h"
>  
> +#define MAX_LINEAR_ADDR_MASK		GENMASK_ULL(15, 8)
> +#define ADDR_OFS_BIT			8

Similar to other comments, please spell this out.  Whatever "OFS" stands for, it
isn't common knowledge (unless I'm way out of the loop).

> +
>  union perf_capabilities {
>  	struct {
>  		u64	lbr_format:6;
> @@ -232,6 +235,102 @@ static void test_lbr_perf_capabilities(union perf_capabilities host_cap)
>  	kvm_vm_free(vm);
>  }
>  
> +/*
> + * Generate a non-canonical address for a given number of address bits.
> + * @addr_bits: The number of address bits used in the system.
> + *
> + * This function calculates a non-canonical address by setting the most
> + * significant bit to 1 and adding an offset equal to the maximum value
> + * that can be represented by the remaining bits. This ensures that the
> + * generated address is outside the valid address range and is consistent.
> + */
> +static inline uint64_t non_canonical_address(unsigned int addr_bits)
> +{
> +	return (1ULL << (addr_bits - 1)) + ((1ULL << (addr_bits - 1)) - 1);
> +}

Eh, I don't know that I would 

> +
> +static uint64_t get_addr_bits(struct kvm_vcpu *vcpu)
> +{
> +	const struct kvm_cpuid_entry2 *kvm_entry;
> +	unsigned int addr_bits;
> +	struct kvm_sregs sregs;
> +
> +	kvm_entry = get_cpuid_entry(kvm_get_supported_cpuid(), 0x80000008, 0);
> +	addr_bits = (kvm_entry->eax & MAX_LINEAR_ADDR_MASK) >> ADDR_OFS_BIT;

*sigh*

X86_PROPERTY_MAX_VIRT_ADDR, or even better, vcpu->vm->va_bits.

> +	/*
> +	 * Get the size of the virtual address space by checking the LA57 bit
> +	 * in the CR4 control register. If the LA57 bit is set, then the virtual
> +	 * address space is 57 bits. Otherwise, it's 48 bits.
> +	 */
> +	if (addr_bits != 32) {
> +		vcpu_sregs_get(vcpu, &sregs);
> +		addr_bits = (sregs.cr4 & X86_CR4_LA57) ? 57 : 48;
> +	}
> +
> +	return addr_bits;

(a) None of this is PMU specific.  (b) Selftests don't support LA57 (yet).
(c) IMO there's no reason to get this fancy.  Just hardcode a value that's
guaranteed to be non-canonical and call it good.

E.g.

#define NONCANONICAL	(0xaaaULL << 48)

That way the "bad" value can be used from the guest directly.

Or if you want to handle LA57 right now and verify that the guest can set values
that are canonical for LA57 but not for LA48, something like

static inline vm_vaddr_t get_noncanonical_address(bool is_la57)
{
	return is_la57 ? BIT(57) : BIT(48);
}

and then the guest can invoke it by reading and passing in the current CR4, i.e.
still doesn't require splitting logic across the guest and host.

> +}
> +
> +static void test_ds_guest_code(uint64_t bad_addr)
> +{
> +	uint8_t vector = 0;
> +
> +	vector = wrmsr_safe(MSR_IA32_DS_AREA, bad_addr);
> +	GUEST_SYNC(vector);

GUEST_ASSERT(vector == GP_VECTOR);

Aaron's fancy printf() stuff is coming, and even without that, splitting logic
across the guest and host is generally a bad idea as it makes it much harder to
understand what the test does.

> +}
> +
> +/* Check if setting MSR_IA32_DS_AREA in guest and kvm userspace will fail. */
> +static void test_ds_area_noncanonical_address(union perf_capabilities host_cap)
> +{
> +	struct kvm_vm *vm;
> +	struct kvm_vcpu *vcpu;
> +	unsigned int r, addr_bits;
> +	uint64_t bad_addr, without_pebs_fmt_caps;
> +	struct ucall uc;

Reverse xmas tree.

> +
> +	vm = vm_create_with_one_vcpu(&vcpu, test_ds_guest_code);
> +	vm_init_descriptor_tables(vm);
> +	vcpu_init_descriptor_tables(vcpu);
> +
> +	/* Check that Setting MSR_IA32_DS_AREA with 0 should succeed. */

Drop the comment, the assert covers everything.

> +	r = _vcpu_set_msr(vcpu, MSR_IA32_DS_AREA, 0);
> +	TEST_ASSERT(r, "Setting MSR_IA32_DS_AREA with 0 should succeed.");

Eh, I would just do vcpu_set_msr() and let it assert for you.

> +	/*
> +	 * Check that if PEBS_FMT is not set setting MSR_IA32_DS_AREA will
> +	 * succeed.

I don't understand what this comment is trying to say.  The below tests that
writing MSR_IA32_DS_AREA *fails*, not succeeds.
		
> +	 */
> +	without_pebs_fmt_caps = host_cap.capabilities & ~PERF_CAP_PEBS_FORMAT;
> +	vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, without_pebs_fmt_caps);
> +	r = _vcpu_set_msr(vcpu, MSR_IA32_DS_AREA, 1);
> +	TEST_ASSERT(r, "Setting MSR_IA32_DS_AREA with bad addr should fail.");

Print the actual "bad" value, and be more descriptive, i.e. state *why* '1' is
a bad value.

> +
> +	/*
> +	 * Check that setting MSR_IA32_DS_AREA in kvm userspace to use a
> +	 * non-canonical address should fail.
> +	 */
> +	vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.capabilities);
> +	addr_bits = get_addr_bits(vcpu);
> +	bad_addr = non_canonical_address(addr_bits);
> +	r = _vcpu_set_msr(vcpu, MSR_IA32_DS_AREA, bad_addr);
> +	TEST_ASSERT(!r, "Setting MSR_IA32_DS_AREA with bad addr should fail.");

Same comment here.