| Message ID | 20231108111806.92604-30-nsaenz@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: hyperv: Introduce VSM support | expand |
On 08.11.23 12:18, Nicolas Saenz Julienne wrote: > Save the length of the instruction that triggered an EPT violation in > struct kvm_vcpu_arch. This will be used to populate Hyper-V VSM memory > intercept messages. > > Signed-off-by: Nicolas Saenz Julienne <nsaenz@amazon.com> In v1, please do this for SVM as well :) Alex Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On Wed, Nov 08, 2023, Alexander Graf wrote: > > On 08.11.23 12:18, Nicolas Saenz Julienne wrote: > > Save the length of the instruction that triggered an EPT violation in > > struct kvm_vcpu_arch. This will be used to populate Hyper-V VSM memory > > intercept messages. > > > > Signed-off-by: Nicolas Saenz Julienne <nsaenz@amazon.com> > > > In v1, please do this for SVM as well :) Why? KVM caches values on VMX because VMREAD is measurable slower than memory accesses, especially when running nested. SVM has no such problems. I wouldn't be surprised if adding a "cache" is actually less performant due to increased pressure and misses on the hardware cache.
On 08.11.23 17:15, Sean Christopherson wrote: > > On Wed, Nov 08, 2023, Alexander Graf wrote: >> On 08.11.23 12:18, Nicolas Saenz Julienne wrote: >>> Save the length of the instruction that triggered an EPT violation in >>> struct kvm_vcpu_arch. This will be used to populate Hyper-V VSM memory >>> intercept messages. >>> >>> Signed-off-by: Nicolas Saenz Julienne <nsaenz@amazon.com> >> >> In v1, please do this for SVM as well :) > Why? KVM caches values on VMX because VMREAD is measurable slower than memory > accesses, especially when running nested. SVM has no such problems. I wouldn't > be surprised if adding a "cache" is actually less performant due to increased > pressure and misses on the hardware cache. My understanding was that this patch wasn't about caching it, it was about storing it somewhere generically so we can use it for the fault injection code path in the following patch. And if we don't set this variable for SVM, it just means Credential Guard fault injection would be broken there. Alex Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On Wed, Nov 08, 2023, Nicolas Saenz Julienne wrote: > Save the length of the instruction that triggered an EPT violation in > struct kvm_vcpu_arch. This will be used to populate Hyper-V VSM memory > intercept messages. This is silly and unnecessarily obfuscates *why* (as my response regarding SVM shows), i.e. that this is "needed" becuase the value is consumed by a *different* vCPU, not because of performance concerns. It's also broken, AFAICT nothing prevents the intercepted vCPU from hitting a different EPT violation before the target vCPU consumes exit_instruction_len. Holy cow. All of deliver_gpa_intercept() is wildly unsafe. Aside from race conditions, which in and of themselves are a non-starter, nothing guarantees that the intercepted vCPU actually cached all of the information that is held in its VMCS. The sane way to do this is to snapshot *all* information on the intercepted vCPU, and then hand that off as a payload to the target vCPU. That is, assuming the cross-vCPU stuff is actually necessary. At a glance, I don't see anything that explains *why*.
On 08.11.23 18:20, Sean Christopherson wrote: > On Wed, Nov 08, 2023, Nicolas Saenz Julienne wrote: >> Save the length of the instruction that triggered an EPT violation in >> struct kvm_vcpu_arch. This will be used to populate Hyper-V VSM memory >> intercept messages. > This is silly and unnecessarily obfuscates *why* (as my response regarding SVM > shows), i.e. that this is "needed" becuase the value is consumed by a *different* > vCPU, not because of performance concerns. > > It's also broken, AFAICT nothing prevents the intercepted vCPU from hitting a > different EPT violation before the target vCPU consumes exit_instruction_len. > > Holy cow. All of deliver_gpa_intercept() is wildly unsafe. Aside from race > conditions, which in and of themselves are a non-starter, nothing guarantees that > the intercepted vCPU actually cached all of the information that is held in its VMCS. > > The sane way to do this is to snapshot *all* information on the intercepted vCPU, > and then hand that off as a payload to the target vCPU. That is, assuming the > cross-vCPU stuff is actually necessary. At a glance, I don't see anything that > explains *why*. Yup, I believe you repeated the comment I had on the function - and Nicolas already agreed :). This should go through user space which automatically means you need to bubble up all necessary trap data to user space on the faulting vCPU and then inject the full set of data into the receiving one. My point with the comment on this patch was "Don't break AMD (or ancient VMX without instruction length decoding [Does that exist? I know SVM has old CPUs that don't do it]) please". Alex Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On Wed, Nov 8, 2023 at 9:27 AM Alexander Graf <graf@amazon.com> wrote: > My point with the comment on this patch was "Don't break AMD (or ancient > VMX without instruction length decoding [Does that exist? I know SVM has > old CPUs that don't do it]) please". VM-exit instruction length is not defined for all VM-exit reasons (EPT misconfiguration is one that is notably absent), but the field has been there since Prescott.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 1f5a85d461ce..1a854776d91e 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -967,6 +967,8 @@ struct kvm_vcpu_arch { /* set at EPT violation at this point */ unsigned long exit_qualification; + u32 exit_instruction_len; + /* pv related host specific info */ struct { bool pv_unhalted; diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 6e502ba93141..9c83ee3a293d 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -5773,6 +5773,7 @@ static int handle_ept_violation(struct kvm_vcpu *vcpu) PFERR_GUEST_FINAL_MASK : PFERR_GUEST_PAGE_MASK; vcpu->arch.exit_qualification = exit_qualification; + vcpu->arch.exit_instruction_len = vmcs_read32(VM_EXIT_INSTRUCTION_LEN); /* * Check that the GPA doesn't exceed physical memory limits, as that is
Save the length of the instruction that triggered an EPT violation in struct kvm_vcpu_arch. This will be used to populate Hyper-V VSM memory intercept messages. Signed-off-by: Nicolas Saenz Julienne <nsaenz@amazon.com> --- arch/x86/include/asm/kvm_host.h | 2 ++ arch/x86/kvm/vmx/vmx.c | 1 + 2 files changed, 3 insertions(+)