[3/3] selftests: KVM: Add test case for MMIO during event delivery

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@amazon.com>
---
 .../selftests/kvm/set_memory_region_test.c    | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)

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@amazon.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
>

On Fri, 2024-10-11 at 17:21 -0700, Sean Christopherson wrote:
> 
> > +
> > +       /* 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);


Is there really *any* way that can be interpreted as anything other
than "the CPU executing this code will never get to this point and
that's why there's an ASSERT(0) right after this comment"?

I don't believe there's *any* way that particular pronoun can be
ambiguous, and now we've got to the point of fetishising the bizarre
"no pronouns" rule just for the sake of it.

I get it, especially for some individuals it *can* be difficult to take
context into account, and the wilful use of pronouns instead of
spelling things out explicitly *every* *single* *time* can sometimes
help. But at a cost of conciseness and brevity.

On Thu, Oct 17, 2024, David Woodhouse wrote:
> On Fri, 2024-10-11 at 17:21 -0700, Sean Christopherson wrote:
> > 
> > > +
> > > +       /* 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);
> 
> 
> Is there really *any* way that can be interpreted as anything other
> than "the CPU executing this code will never get to this point and
> that's why there's an ASSERT(0) right after this comment"?
> 
> I don't believe there's *any* way that particular pronoun can be
> ambiguous, and now we've got to the point of fetishising the bizarre
> "no pronouns" rule just for the sake of it.

No, it's not just for the sake of it.  In this case, "we" isn't all that ambiguous,
(though my interpretation of it is "the test", not "the CPU"), but only because the
comment is utterly useless.  The GUEST_ASSERT(0) communicates very clearly that it's
supposed to be unreachable.

And if the comment were rewritten to explain _why_ the code is unreachable, then
"we" is all bug guaranateed to become ambiguous, because explaining "why" likely
means preciesly describing the behavior the userspace side, the guest side, and/or
KVM.  In other words, using "we" or "us" is often a hint that either the statement
is likely ambiguous or doesn't add value.

And irrespective of whether or not you agree with the above, having a hard rule of
"no we, no us" eliminates all subjectivity, and for me that is sufficient reason
to enforce the rule.

> I get it, especially for some individuals it *can* be difficult to take
> context into account, and the wilful use of pronouns instead of
> spelling things out explicitly *every* *single* *time* can sometimes
> help. But at a cost of conciseness and brevity.

In this particular case, I am more than willing to sacrifice brevity.  I 100%
agree that there is value in having to-the-point comments and changelogs, but I
can't recall a single time where avoiding a "we" or "us" made a statement
meaningfully harder to read and understand.  On ther hand, I can recall many, many
changelogs I had to re-read multiple times because I struggled to figure out how
the author _intended_ "we" or "us" to be interpreted.