On 12/01/21 07:37, Wei Huang wrote:
> static int gp_interception(struct vcpu_svm *svm)
> {
> struct kvm_vcpu *vcpu = &svm->vcpu;
> u32 error_code = svm->vmcb->control.exit_info_1;
> -
> - WARN_ON_ONCE(!enable_vmware_backdoor);
> + int rc;
>
> /*
> - * VMware backdoor emulation on #GP interception only handles IN{S},
> - * OUT{S}, and RDPMC, none of which generate a non-zero error code.
> + * Only VMware backdoor and SVM VME errata are handled. Neither of
> + * them has non-zero error codes.
> */
> if (error_code) {
> kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> return 1;
> }
> - return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP);
> +
> + rc = kvm_emulate_instruction(vcpu, EMULTYPE_PARAVIRT_GP);
> + if (rc > 1)
> + rc = svm_emulate_vm_instr(vcpu, rc);
> + return rc;
> }
>
Passing back the third byte is quick hacky. Instead of this change to
kvm_emulate_instruction, I'd rather check the instruction bytes in
gp_interception before calling kvm_emulate_instruction. That would be
something like:
- move "kvm_clear_exception_queue(vcpu);" inside the "if
(!(emulation_type & EMULTYPE_NO_DECODE))". It doesn't apply when you
are coming back from userspace.
- extract the "if (!(emulation_type & EMULTYPE_NO_DECODE))" body to a
new function x86_emulate_decoded_instruction. Call it from
gp_interception, we know this is not a pagefault and therefore
vcpu->arch.write_fault_to_shadow_pgtable must be false.
- check ctxt->insn_bytes for an SVM instruction
- if not an SVM instruction, call kvm_emulate_instruction(vcpu,
EMULTYPE_VMWARE_GP|EMULTYPE_NO_DECODE).
Thanks,
Paolo
On Tue, Jan 12, 2021, Paolo Bonzini wrote:
> On 12/01/21 07:37, Wei Huang wrote:
> > static int gp_interception(struct vcpu_svm *svm)
> > {
> > struct kvm_vcpu *vcpu = &svm->vcpu;
> > u32 error_code = svm->vmcb->control.exit_info_1;
> > -
> > - WARN_ON_ONCE(!enable_vmware_backdoor);
> > + int rc;
> > /*
> > - * VMware backdoor emulation on #GP interception only handles IN{S},
> > - * OUT{S}, and RDPMC, none of which generate a non-zero error code.
> > + * Only VMware backdoor and SVM VME errata are handled. Neither of
> > + * them has non-zero error codes.
> > */
> > if (error_code) {
> > kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> > return 1;
> > }
> > - return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP);
> > +
> > + rc = kvm_emulate_instruction(vcpu, EMULTYPE_PARAVIRT_GP);
> > + if (rc > 1)
> > + rc = svm_emulate_vm_instr(vcpu, rc);
> > + return rc;
> > }
>
> Passing back the third byte is quick hacky. Instead of this change to
> kvm_emulate_instruction, I'd rather check the instruction bytes in
> gp_interception before calling kvm_emulate_instruction.
Agreed. And I'd also prefer that any pure refactoring is done in separate
patch(es) so that the actual functional change is better isolated.
On a related topic, it feels like nested should be disabled by default on SVM
until it's truly ready for primetime, with the patch tagged for stable. That
way we don't have to worry about crafting non-trivial fixes (like this one) to
make them backport-friendly.
On 12/01/21 18:42, Sean Christopherson wrote:
> On a related topic, it feels like nested should be disabled by default on SVM
> until it's truly ready for primetime, with the patch tagged for stable. That
> way we don't have to worry about crafting non-trivial fixes (like this one) to
> make them backport-friendly.
Well, that's historical; I wish it had been disabled by default back in
the day.
However, after 10 years and after the shakedown last year, it's hard to
justify breaking backwards compatibility. Nested SVM is not any less
ready than nested VMX---just a little less optimized for things such as
TLB flushes and ASID/VPID---even without this fix. The erratum has
visible effects only on a minority of AMD systems (it depends on an
unlucky placement of TSEG on L0), and it is easy to work around it by
lowering the amount of <4G memory in L1.
Paolo
On 1/12/21 8:01 AM, Paolo Bonzini wrote:
> On 12/01/21 07:37, Wei Huang wrote:
>> static int gp_interception(struct vcpu_svm *svm)
>> {
>> struct kvm_vcpu *vcpu = &svm->vcpu;
>> u32 error_code = svm->vmcb->control.exit_info_1;
>> -
>> - WARN_ON_ONCE(!enable_vmware_backdoor);
>> + int rc;
>> /*
>> - * VMware backdoor emulation on #GP interception only handles IN{S},
>> - * OUT{S}, and RDPMC, none of which generate a non-zero error code.
>> + * Only VMware backdoor and SVM VME errata are handled. Neither of
>> + * them has non-zero error codes.
>> */
>> if (error_code) {
>> kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
>> return 1;
>> }
>> - return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP);
>> +
>> + rc = kvm_emulate_instruction(vcpu, EMULTYPE_PARAVIRT_GP);
>> + if (rc > 1)
>> + rc = svm_emulate_vm_instr(vcpu, rc);
>> + return rc;
>> }
>>
>
> Passing back the third byte is quick hacky. Instead of this change to
> kvm_emulate_instruction, I'd rather check the instruction bytes in
> gp_interception before calling kvm_emulate_instruction. That would be
> something like:
>
> - move "kvm_clear_exception_queue(vcpu);" inside the "if
> (!(emulation_type & EMULTYPE_NO_DECODE))". It doesn't apply when you
> are coming back from userspace.
>
> - extract the "if (!(emulation_type & EMULTYPE_NO_DECODE))" body to a
> new function x86_emulate_decoded_instruction. Call it from
> gp_interception, we know this is not a pagefault and therefore
> vcpu->arch.write_fault_to_shadow_pgtable must be false.
If the whole body inside if-statement is moved out, do you expect the
interface of x86_emulate_decoded_instruction to be something like:
int x86_emulate_decoded_instruction(struct kvm_vcpu *vcpu,
gpa_t cr2_or_gpa,
int emulation_type, void *insn,
int insn_len,
bool write_fault_to_spt)
And if so, what is the emulation type to use when calling this function
from svm.c? EMULTYPE_VMWARE_GP?
>
> - check ctxt->insn_bytes for an SVM instruction
>
> - if not an SVM instruction, call kvm_emulate_instruction(vcpu,
> EMULTYPE_VMWARE_GP|EMULTYPE_NO_DECODE).
>
> Thanks,
>
> Paolo
>
On 15/01/21 08:00, Wei Huang wrote:
> If the whole body inside if-statement is moved out, do you expect the
> interface of x86_emulate_decoded_instruction to be something like:
>
> int x86_emulate_decoded_instruction(struct kvm_vcpu *vcpu,
> gpa_t cr2_or_gpa,
> int emulation_type, void *insn,
> int insn_len,
> bool write_fault_to_spt)
An idea is to making the body of the new function just
init_emulate_ctxt(vcpu);
/*
* We will reenter on the same instruction since
* we do not set complete_userspace_io. This does not
* handle watchpoints yet, those would be handled in
* the emulate_ops.
*/
if (!(emulation_type & EMULTYPE_SKIP) &&
kvm_vcpu_check_breakpoint(vcpu, &r))
return r;
ctxt->interruptibility = 0;
ctxt->have_exception = false;
ctxt->exception.vector = -1;
ctxt->exception.error_code_valid = false;
ctxt->perm_ok = false;
ctxt->ud = emulation_type & EMULTYPE_TRAP_UD;
r = x86_decode_insn(ctxt, insn, insn_len);
trace_kvm_emulate_insn_start(vcpu);
++vcpu->stat.insn_emulation;
return r;
because for the new caller, on EMULATION_FAILED you can just re-enter
the guest.
> And if so, what is the emulation type to use when calling this function
> from svm.c? EMULTYPE_VMWARE_GP?
Just 0 I think.
Paolo