Message-ID: <ZwnBGtdbvmKHc4in@google.com>
Date: Fri, 11 Oct 2024 17:21:46 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Ivan Orlov <iorlov@...zon.com>
Cc: bp@...en8.de, dave.hansen@...ux.intel.com, mingo@...hat.com, 
	pbonzini@...hat.com, shuah@...nel.org, tglx@...utronix.de, hpa@...or.com, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, x86@...nel.org, jalliste@...zon.com, 
	nh-open-source@...zon.com, pdurrant@...zon.co.uk
Subject: Re: [PATCH 3/3] selftests: KVM: Add test case for MMIO during event delivery

On Fri, Sep 27, 2024, Ivan Orlov wrote:
> Extend the 'set_memory_region_test' with a test case which covers the
> MMIO during event delivery error handling. The test case
> 
> 1) Tries to set an IDT descriptor base to point to an MMIO address
> 2) Generates a #GP
> 3) Verifies that we got a correct exit reason (KVM_EXIT_INTERNAL_ERROR)
>    and suberror code (KVM_INTERNAL_ERROR_DELIVERY_EV)
> 4) Verifies that we got a corrent "faulty" GPA in internal.data[3]
> 
> Signed-off-by: Ivan Orlov <iorlov@...zon.com>
> ---
>  .../selftests/kvm/set_memory_region_test.c    | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
> index a8267628e9ed..e9e97346edf1 100644
> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c
> @@ -553,6 +553,51 @@ static void test_add_overlapping_private_memory_regions(void)
>  	close(memfd);
>  	kvm_vm_free(vm);
>  }
> +
> +static const struct desc_ptr faulty_idt_desc = {
> +	.address = MEM_REGION_GPA,
> +	.size = 0xFFF,
> +};

There's no reason this needs to be global, i.e. declare it in the function, on
the stack.

> +
> +static void guest_code_faulty_idt_desc(void)
> +{
> +	__asm__ __volatile__("lidt %0"::"m"(faulty_idt_desc));

It's not "faulty".  It specifically points at MMIO.  That is _very_ different
than a "faulty" address, because an actual fault when vectoring an event would
lead to triple fault shutdown.  And a benefit of declaring the descriptor locally
is that you don't need to come up with a descriptive name :-) E.g.

	const struct desc_ptr idt_desc = {
		.address = MEM_REGION_GPA,
		.size = 0xfff,
	};

And it's probably worth adding a lidt() helper in processor.h (in a separate
commit, because there's two other users that can be converted when it's added).

> +
> +	/* Generate a #GP by dereferencing a non-canonical address */
> +	*((uint8_t *)0xDEADBEEFDEADBEEFULL) = 0x1;

Hmm, I could have sworn KVM-Unit-Tests' NONCANONICAL got pulled into selftests.
Please do that as part of the test, e.g. add this to processor.h

#define NONCANONICAL	0xaaaaaaaaaaaaaaaaull

> +
> +	/* We should never reach this point */

No pronouns.  Yes, it's nitpicky, but "we" gets _very_ ambiguous when "we" could
mean the admin, the user, the VMM, KVM, the guest, etc.

> +	GUEST_ASSERT(0);
> +}
> +
> +/*
> + * This test tries to point the IDT descriptor base to an MMIO address.

There is no try.  Do, or do not :-)

Translation: just state what the code does, don't hedge.

> This action

Wrap at 89.

> + * should cause a KVM internal error, so the VMM could handle such situations gracefully.

Heh, don't editorialize what a VMM might do in comments.  For changelogs it's
often helpful, as it provides justification and context for _why_ that is the
behavior.  But for a selftest, just state what KVM's ABI is.  E.g. I guarantee
there are plenty of VMMs that don't handle this situation gracefully :-)

> + */
> +static void test_faulty_idt_desc(void)
> +{
> +	struct kvm_vm *vm;
> +	struct kvm_vcpu *vcpu;
> +
> +	pr_info("Testing a faulty IDT descriptor pointing to an MMIO address\n");
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, guest_code_faulty_idt_desc);
> +	virt_map(vm, MEM_REGION_GPA, MEM_REGION_GPA, 1);
> +
> +	vcpu_run(vcpu);
> +	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_INTERNAL_ERROR);
> +	TEST_ASSERT(vcpu->run->internal.suberror == KVM_INTERNAL_ERROR_DELIVERY_EV,
> +		    "Unexpected suberror = %d", vcpu->run->internal.suberror);
> +	TEST_ASSERT(vcpu->run->internal.ndata > 4, "Unexpected internal error data array size = %d",
> +		    vcpu->run->internal.ndata);

Capture "run", or maybe event "internal" in a local variable.  Doing so will
shorten these lines and make the code easier to read.  I'd probably vote for
grabbing "internal" since TEST_ASSERT_KVM_EXIT_REASON() takes care of asserting
on the bits outside of "internal".

> +	/* The "faulty" GPA address should be = IDT base + offset of the GP vector */

GPA address is redundant.  GPA is Guest Physical Address.

Again, avoid "faulty".  "reported" works nicely.  And try not to mix code with
human language (though it's ok for math, e.g. the '+' is totally fine and
preferred).  The '=' is hard to read because it looks like a typo.  And in this
case, there's no need to actually say "equal to".  And similar to writing changelogs
for humans instead of giving a play-by-play of the code, do the same for comments, e.g.

	/* The reported GPA should be the address of the #GP entry in the IDT. */

> +	TEST_ASSERT(vcpu->run->internal.data[3] == MEM_REGION_GPA +
> +		    GP_VECTOR * sizeof(struct idt_entry),

Put the math on one line, i.e.

		    vcpu->run->internal.data[3] ==
		    MEM_REGION_GPA + GP_VECTOR * sizeof(struct idt_entry),

> +		    "Unexpected GPA = %llx", vcpu->run->internal.data[3]);

Print what GPA was expected, so that the user doesn't have to manually figure
that out.

> +
> +	kvm_vm_free(vm);
> +}
>  #endif
>  
>  int main(int argc, char *argv[])
> @@ -568,6 +613,7 @@ int main(int argc, char *argv[])
>  	 * KVM_RUN fails with ENOEXEC or EFAULT.
>  	 */
>  	test_zero_memory_regions();
> +	test_faulty_idt_desc();
>  #endif
>  
>  	test_invalid_memory_region_flags();
> -- 
> 2.43.0
> 

Powered by blists - more mailing lists