Re: [kvm-unit-tests PATCH v2] x86: Test illegal LEA handling

[Sean Christopherson <seanjc@google.com>]

On Wed, Aug 03, 2022, Michal Luczaj wrote:
> On 8/1/22 18:44, Sean Christopherson wrote:
> > On Sun, Jul 31, 2022, Michal Luczaj wrote:
> >> +{
> >> +	exceptions = 0;
> >> +	handle_exception(UD_VECTOR, illegal_lea_handler);
> > 
> > No need to use a custom handler (ignore any patterns in emulator.c that suggest
> > it's "mandatory", emulator is one of the oldest test).  ASM_TRY() can handle all
> > of this without any globals.
> > ...
> > static void test_illegal_lea(void)
> > {
> > 	unsigned int vector;
> > 
> > 	asm volatile (ASM_TRY("1f")
> > 		      KVM_FEP ".byte 0x8d; .byte 0xc0\n\t"
> > 		      "1:"
> > 		      : : : "memory", "eax");
> > 
> > 	vector = exception_vector();
> > 	report(vector == UD_VECTOR,
> > 	       "Wanted #UD on LEA with /reg, got vector = %d", vector);
> > }
> 
> I must be missing something important. There is
> `handle_exception(UD_VECTOR, 0)` early in `main()` which simply undoes
> `handle_exception(6, check_exception_table)` set by `setup_idt()`. If
> there's no more exception table walk for #UD, `ASM_TRY` alone can't
> possibly work, am I corrent?

Argh, you're correct, I didn't realize the test zapped the IDT entry.  That's a
bug, the test shouldn't zap entries, the whole point of handle_exception() returning
the old handler is so that the caller can restore it.  Grr.

> If so, am I supposed to restore the `check_exception_table()` handler? Or
> maybe using `test_for_exception()` would be more elegant:

Hmm, I prefer ASM_TRY() over test_for_exception(), having to define a function
just to emit a single instruction is silly.  What I'd really prefer is that we
wouldn't have so many ways for doing the same basic thing (obviously not your
fault, just ranting/whining).

If you have bandwidth, can you create a small series to clean up emulator.c to at
least take a step in the right direction?

  1. Save/restore the handlers.
  2. Use ASM_TRY for the UD_VECTOR cases (KVM_FEP probing and illegal MOVBE)
  3. Add this testcase as described above.

Ideally the test wouldn't use handle_exception() at all, but that's a much bigger
mess and a future problem.