Re: [PATCH v2 2/8] KVM: selftests: Explicitly require instructions bytes

[Sean Christopherson <seanjc@google.com>]

On Tue, Oct 18, 2022, David Matlack wrote:
> Explicitly require instruction bytes to be available in
> run->emulation_failure by asserting that they are present. Note that the
> test already requires the instruction bytes to be present because that's
> the only way the test will advance the RIP past the flds and get to
> GUEST_DONE().
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  .../smaller_maxphyaddr_emulation_test.c       | 47 +++++++++----------
>  1 file changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
> index 6ed996988a5a..c5353ad0e06d 100644
> --- a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
> @@ -65,30 +65,29 @@ static void process_exit_on_emulation_error(struct kvm_vcpu *vcpu)
>  		    "Unexpected suberror: %u",
>  		    run->emulation_failure.suberror);
>  
> -	if (run->emulation_failure.ndata >= 1) {
> -		flags = run->emulation_failure.flags;
> -		if ((flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES) &&
> -		    run->emulation_failure.ndata >= 3) {
> -			insn_size = run->emulation_failure.insn_size;
> -			insn_bytes = run->emulation_failure.insn_bytes;
> -
> -			TEST_ASSERT(insn_size <= 15 && insn_size > 0,
> -				    "Unexpected instruction size: %u",
> -				    insn_size);
> -
> -			TEST_ASSERT(is_flds(insn_bytes, insn_size),
> -				    "Unexpected instruction.  Expected 'flds' (0xd9 /0)");
> -
> -			/*
> -			 * If is_flds() succeeded then the instruction bytes
> -			 * contained an flds instruction that is 2-bytes in
> -			 * length (ie: no prefix, no SIB, no displacement).
> -			 */
> -			vcpu_regs_get(vcpu, &regs);
> -			regs.rip += 2;
> -			vcpu_regs_set(vcpu, &regs);
> -		}
> -	}
> +	flags = run->emulation_failure.flags;
> +	TEST_ASSERT(run->emulation_failure.ndata >= 3 &&
> +		    flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES,
> +		    "run->emulation_failure is missing instruction bytes");
> +
> +	insn_size = run->emulation_failure.insn_size;
> +	insn_bytes = run->emulation_failure.insn_bytes;
> +
> +	TEST_ASSERT(insn_size <= 15 && insn_size > 0,
> +		    "Unexpected instruction size: %u",
> +		    insn_size);

Unnecessary newline, insn_size fits comfortably on the line above.

> +
> +	TEST_ASSERT(is_flds(insn_bytes, insn_size),
> +		    "Unexpected instruction.  Expected 'flds' (0xd9 /0)");
> +
> +	/*
> +	 * If is_flds() succeeded then the instruction bytes contained an flds
> +	 * instruction that is 2-bytes in length (ie: no prefix, no SIB, no
> +	 * displacement).
> +	 */
> +	vcpu_regs_get(vcpu, &regs);
> +	regs.rip += 2;

This whole sequence is silly.  Assert that size > 0 but < 15, then assert that
it's >= 2, then skip exactly two bytes and effective "assert" that it's '2.

And while I appreciate the ModR/M decoding, IMO it does more harm than good.  If
someone can follow the ModR/M decoding, they can figure out a hardcode opcode.
E.g. IMO this is much simpler and will be easier to debug.

#define FLDS_MEM_EAX ".byte 0xd9, 0x00"

static void guest_code(void)
{
	__asm__ __volatile__(FLDS_MEM_EAX
			     :: "a"(MEM_REGION_GVA));

	GUEST_DONE();
}

static void process_exit_on_emulation_error(struct kvm_vcpu *vcpu)
{
	struct kvm_run *run = vcpu->run;
	struct kvm_regs regs;
	uint8_t *insn_bytes;
	uint64_t flags;

	TEST_ASSERT(run->exit_reason == KVM_EXIT_INTERNAL_ERROR,
		    "Unexpected exit reason: %u (%s)",
		    run->exit_reason,
		    exit_reason_str(run->exit_reason));

	TEST_ASSERT(run->emulation_failure.suberror == KVM_INTERNAL_ERROR_EMULATION,
		    "Unexpected suberror: %u",
		    run->emulation_failure.suberror);

	flags = run->emulation_failure.flags;
	TEST_ASSERT(run->emulation_failure.ndata >= 3 &&
		    flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES,
		    "run->emulation_failure is missing instruction bytes");

	TEST_ASSERT(run->emulation_failure.insn_size == 2,
		    "Expected a 2-byte opcode for 'flds', got %d bytes",
		    run->emulation_failure.insn_size);

	insn_bytes = run->emulation_failure.insn_bytes;
	TEST_ASSERT(insn_bytes[0] == 0xd9 && insn_bytes[1] == 0,
		    "Expected 'flds [eax]', opcode '0xd9 0x00', got opcode 0x%x 0x%x\n",
		    insn_bytes[0], insn_bytes[1]);

	vcpu_regs_get(vcpu, &regs);
	regs.rip += 2;
	vcpu_regs_set(vcpu, &regs);
}