diff mbox series

[RFC,29/33] KVM: VMX: Save instruction length on EPT violation

Message ID 20231108111806.92604-30-nsaenz@amazon.com
State New
Headers
Series KVM: x86: hyperv: Introduce VSM support |

Commit Message

Nicolas Saenz Julienne Nov. 8, 2023, 11:18 a.m. UTC 
  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(+)

Comments

Alexander Graf Nov. 8, 2023, 12:40 p.m. UTC | #1 
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
Sean Christopherson Nov. 8, 2023, 4:15 p.m. UTC | #2 
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.
Alexander Graf Nov. 8, 2023, 5:11 p.m. UTC | #3 
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
Sean Christopherson Nov. 8, 2023, 5:20 p.m. UTC | #4 
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*.
Alexander Graf Nov. 8, 2023, 5:27 p.m. UTC | #5 
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
Jim Mattson Nov. 8, 2023, 6:19 p.m. UTC | #6 
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 mbox series

Patch

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