Re: [PATCH 1/2] kvm: x86: Allow userspace to handle emulation errors

[Sean Christopherson <seanjc@google.com>]

On Fri, Apr 16, 2021, Aaron Lewis wrote:
> +7.24 KVM_CAP_EXIT_ON_EMULATION_FAILURE
> +--------------------------------------
> +
> +:Architectures: x86
> +:Parameters: args[0] whether the feature should be enabled or not
> +
> +With this capability enabled, the in-kernel instruction emulator packs the exit
> +struct of KVM_INTERNAL_ERROR with the instruction length and instruction bytes
> +when an error occurs while emulating an instruction.  This allows userspace to
> +then take a look at the instruction and see if it is able to handle it more
> +gracefully than the in-kernel emulator.

As alluded to later in the thread, I don't think we should condition the extra
information on this capability.  By crafting the struct overlay to be backwards
compatibile, KVM can safely dump all the new information, even for old VMMs.
An old VMM may not programmatically use the data, but I suspect most VMMs at
least dump all info, e.g. QEMU does:

    if (kvm_check_extension(kvm_state, KVM_CAP_INTERNAL_ERROR_DATA)) {
        int i;

        for (i = 0; i < run->internal.ndata; ++i) {
            fprintf(stderr, "extra data[%d]: %"PRIx64"\n",
                    i, (uint64_t)run->internal.data[i]);
        }
    }

This would be a way to feed more info to the poor sod that has to debug
emulation failures :-)

> +
> +When this capability is enabled use the emulation_failure struct instead of the
> +internal struct for the exit struct.  They have the same layout, but the
> +emulation_failure struct matches the content better.

This documentation misses the arguably more important details of what exactly
"EXIT_ON_EMULATION_FAILURE" means.  E.g. it should call out the KVM still exits
on certain types (skip) even if this capability is not enabled, and that KVM
will _never_ exit if VMware #GP emulation fails.

> +
>  8. Other capabilities.
>  ======================
> @@ -7119,8 +7124,29 @@ void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)

...

>  }
>  EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt);
>  
> +static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
> +{
> +	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
> +	u64 insn_size = ctxt->fetch.end - ctxt->fetch.data;
> +	struct kvm *kvm = vcpu->kvm;
> +
> +	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;

Grab vcpu->run in a local variable.

> +	vcpu->run->emulation_failure.suberror = KVM_INTERNAL_ERROR_EMULATION;
> +	vcpu->run->emulation_failure.ndata = 0;
> +	if (kvm->arch.exit_on_emulation_error && insn_size > 0) {

I definitely think this should not be conditioned on exit_on_emulation_error.

No need for "> 0", it's an unsigned value.

> +		vcpu->run->emulation_failure.ndata = 3;
> +		vcpu->run->emulation_failure.flags =

Flags needs to be zeroed when insn_size==0.  And use |= for new flags so that,
if we add new flags, the existing code doesn't need to be modified.

> +			KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES;
> +		vcpu->run->emulation_failure.insn_size = insn_size;
> +		memcpy(vcpu->run->emulation_failure.insn_bytes,
> +		       ctxt->fetch.data, sizeof(ctxt->fetch.data));

Doesn't truly matter, but I think it's less confusing to copy over insn_size
bytes.

> +	}


	...
	struct kvm_run *kvm_run = vcpu->run;

	kvm_run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
	kvm_run->emulation_failure.suberror = KVM_INTERNAL_ERROR_EMULATION;
	kvm_run->emulation_failure.ndata = 1;
	kvm_run->emulation_failure.flags = 0;

	if (insn_size) {
		kvm_run->emulation_failure.ndata = 3;
		kvm_run->emulation_failure.flags |=
			KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES;
		kvm_run->emulation_failure.insn_size = insn_size;
		memcpy(kvm_run->emulation_failure.insn_bytes, ctxt->fetch.data, insn_size);
	}

> +}