[RFC PATCH kernel 0/2] PreventHostIBS feature for SEV-ES and SNP guests

[RFC PATCH kernel 0/2] PreventHostIBS feature for SEV-ES and SNP guests

[Manali Shukla]

Adds support for PreventHostIBS feature for SEV-ES and SNP guests.
Currently, the hypervisor is able to inspect instruction based samples
from the guest and gather execution information.  With enablement of
PreventHostIBS feature, SEV-ES and SNP guests may choose to disallow
use of instruction based sampling by the hypervisor in order to limit
the information gathered about their execution.  (More information in
Section 15.36.17 APM Volume 2)

While implementing this feature, unknown NMIs were being seen. On
further investigation, a race was found effecting the IBS FETCH/OP
MSR.

ENABLE bit and VALID bit for IBS_FETCH_CTL are contained in the same
MSR and same is the case with IBS_OP_CTL.

Consider the following scenario:
- The IBS MSR which has ENABLE bit set and VALID bit clear is read.
- During the process of clearing the ENABLE bit and writing the IBS
  MSR to disable IBS, an IBS event can occur that sets the VALID bit.
- The write operation on IBS MSR can clear the newly set VALID bit.
- Since this situation is occurring in the CLGI/STGI window
  (PreventHostIBS window), the actual NMI is not taken.
- Once VMRUN is issued, it will exit with VMEXIT_NMI and as soon as
  STGI is executed, the pending NMI will trigger.
- The IBS NMI handler checks for the VALID bit to determine if the NMI
  is generated because of IBS.
- Since VALID bit is now clear, it doesn't recognize that an IBS event
  is occurred which in turn generates the dazed and confused unknown
  NMI messages.

Per-cpu ibs_flags which indicates whether PreventHostIBS window is
active/inactive are added to avoid the above mentioned race.

An active PreventHostIBS window is set before calling VMRUN and
cleared after STGI. PreventHostIBS window check is added to
perf_ibs_handle_irq(), to avoid unknown NMIs and treat them as handled
when window is active.

There are 2 patches in this series.
1) Add amd_prevent_hostibs_window() function to set per-cpu ibs_flags
based on an active/inactive PreventHostIBS window.
2) Enable PreventHostIBS for SEV-ES and SNP guests.

Testing done:
- Executed program symbols in guest are not captured in host when
  PreventHostIBS feature is enabled.
- Generated 1000+ NMIs using cpuid command, no unknown NMIs are seen
  after enablement of PreventHostIBS feature.

Qemu commandline to enable PreventHostIBS on guest.

qemu-system-x86_64 -enable-kvm -cpu EPYC-v4,+nohostibs \ ..

Manali Shukla (2):
  perf/x86/amd: Add amd_prevent_hostibs_window() to set per-cpu
    ibs_flags
  KVM: SEV: PreventHostIBS enablement for SEV-ES and SNP guest

 arch/x86/events/amd/ibs.c          | 64 ++++++++++++++++++++++++++++++
 arch/x86/include/asm/cpufeatures.h |  1 +
 arch/x86/include/asm/perf_event.h  | 20 ++++++++++
 arch/x86/kvm/svm/sev.c             | 10 +++++
 arch/x86/kvm/svm/svm.c             | 39 +++++++++++++++++-
 arch/x86/kvm/svm/svm.h             |  1 +
 6 files changed, 133 insertions(+), 2 deletions(-)

-- 
2.34.1

[RFC PATCH kernel 1/2] perf/x86/amd: Add amd_prevent_hostibs_window() to set per-cpu ibs_flags

[Manali Shukla]

Add a function to set per-cpu ibs_flags based on an active or inactive
PreventHostIBS window.

MSR_AMD64_IBSFETCHCTL[IbsFetchEn] and MSR_AMD64_IBSOPCTL[IbsOpEn] bits
need to be cleared for PreventHostIBS feature to be enabled before VMRUN
is executed.

ENABLE bit and VALID bit for MSR_AMD64_IBSFETCHCTL are contained in the
same MSR and same is the case with MSR_AMD64_IBSOPCTL.

Consider the following scenario:
- The IBS MSR which has ENABLE bit set and VALID bit clear is read.
- During the process of clearing the ENABLE bit and writing the IBS MSR
  to disable IBS, an IBS event can occur that sets the VALID bit.
- The write operation on IBS MSR can clear the newly set VALID bit.
- Since this situation is occurring in the CLGI/STGI window
  (PreventHostIBS window), the actual NMI is not taken.
- Once VMRUN is issued, it will exit with VMEXIT_NMI. As soon as STGI is
  executed, the pending NMI will trigger.
- The IBS NMI handler checks for the VALID bit to determine if the NMI
  is generated because of IBS.
- Since VALID bit is now clear, it doesn't recognize that an IBS event
  is occurred. Due to this reason, the dazed and confused unknown NMI
  messages are generated.

amd_prevent_hostibs_window() is added to avoid these messages when
PreventHostIBS window is active and PreventHostIBS feature is enabled
for the guest.

Signed-off-by: Manali Shukla <manali.shukla@amd.com>
---
 arch/x86/events/amd/ibs.c         | 64 +++++++++++++++++++++++++++++++
 arch/x86/include/asm/perf_event.h | 20 ++++++++++
 2 files changed, 84 insertions(+)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index da3f5ebac4e1..e96a4c9ff4ba 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -30,7 +30,9 @@ static u32 ibs_caps;
 
 #define IBS_FETCH_CONFIG_MASK	(IBS_FETCH_RAND_EN | IBS_FETCH_MAX_CNT)
 #define IBS_OP_CONFIG_MASK	IBS_OP_MAX_CNT
+#define PREVENT_HOSTIBS_WINDOW  BIT(0)
 
+static DEFINE_PER_CPU(unsigned int, ibs_flags);
 
 /*
  * IBS states:
@@ -1035,6 +1037,18 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
 		if (test_and_clear_bit(IBS_STOPPED, pcpu->state))
 			return 1;
 
+		/*
+		 * Catch NMIs generated in an active PreventHostIBS window:
+		 * incoming NMIs from an active PreventHostIBS window might have
+		 * the VALID bit cleared when it is supposed to be set due to
+		 * a race. The reason for the race is ENABLE and VALID bits for
+		 * MSR_AMD64_IBSFETCHCTL and MSR_AMD64_IBSOPCTL being in their
+		 * same respective MSRs.  Ignore all such NMIs and treat them as
+		 * handled.
+		 */
+		if (__this_cpu_read(ibs_flags) & PREVENT_HOSTIBS_WINDOW)
+			return 1;
+
 		return 0;
 	}
 
@@ -1540,3 +1554,53 @@ static __init int amd_ibs_init(void)
 
 /* Since we need the pci subsystem to init ibs we can't do this earlier: */
 device_initcall(amd_ibs_init);
+
+void amd_prevent_hostibs_window(bool active)
+{
+	if (active)
+		__this_cpu_write(ibs_flags,
+				 __this_cpu_read(ibs_flags) |
+				 PREVENT_HOSTIBS_WINDOW);
+	else
+		__this_cpu_write(ibs_flags,
+				 __this_cpu_read(ibs_flags) &
+				 ~PREVENT_HOSTIBS_WINDOW);
+}
+EXPORT_SYMBOL_GPL(amd_prevent_hostibs_window);
+
+bool amd_disable_ibs_fetch(u64 *ibs_fetch_ctl)
+{
+	*ibs_fetch_ctl = __rdmsr(MSR_AMD64_IBSFETCHCTL);
+	if (!(*ibs_fetch_ctl & IBS_FETCH_ENABLE))
+		return false;
+
+	native_wrmsrl(MSR_AMD64_IBSFETCHCTL,
+		      *ibs_fetch_ctl & ~IBS_FETCH_ENABLE);
+
+	return true;
+}
+EXPORT_SYMBOL(amd_disable_ibs_fetch);
+
+bool amd_disable_ibs_op(u64 *ibs_op_ctl)
+{
+	*ibs_op_ctl = __rdmsr(MSR_AMD64_IBSOPCTL);
+	if (!(*ibs_op_ctl & IBS_OP_ENABLE))
+		return false;
+
+	native_wrmsrl(MSR_AMD64_IBSOPCTL, *ibs_op_ctl & ~IBS_OP_ENABLE);
+
+	return true;
+}
+EXPORT_SYMBOL(amd_disable_ibs_op);
+
+void amd_restore_ibs_fetch(u64 ibs_fetch_ctl)
+{
+	native_wrmsrl(MSR_AMD64_IBSFETCHCTL, ibs_fetch_ctl);
+}
+EXPORT_SYMBOL(amd_restore_ibs_fetch);
+
+void amd_restore_ibs_op(u64 ibs_op_ctl)
+{
+	native_wrmsrl(MSR_AMD64_IBSOPCTL, ibs_op_ctl);
+}
+EXPORT_SYMBOL(amd_restore_ibs_op);
diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
index 5d0f6891ae61..1005505e23b1 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -561,6 +561,26 @@ static inline void intel_pt_handle_vmx(int on)
 }
 #endif
 
+#if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_CPU_SUP_AMD)
+extern void amd_prevent_hostibs_window(bool active);
+extern bool amd_disable_ibs_fetch(u64 *ibs_fetch_ctl);
+extern bool amd_disable_ibs_op(u64 *ibs_op_ctl);
+extern void amd_restore_ibs_fetch(u64 ibs_fetch_ctl);
+extern void amd_restore_ibs_op(u64 ibs_op_ctl);
+#else
+static inline void amd_prevent_hostibs_window(bool active) {}
+static inline bool amd_disable_ibs_fetch(u64 *ibs_fetch_ctl)
+{
+	return false;
+}
+static inline bool amd_disable_ibs_op(u64 *ibs_op_ctl)
+{
+	return false;
+}
+static inline void amd_restore_ibs_fetch(u64 ibs_fetch_ctl) {}
+static inline void amd_restore_ibs_op(u64 ibs_op_ctl) {}
+#endif
+
 #if defined(CONFIG_PERF_EVENTS) && defined(CONFIG_CPU_SUP_AMD)
  extern void amd_pmu_enable_virt(void);
  extern void amd_pmu_disable_virt(void);
-- 
2.34.1

[RFC PATCH kernel 2/2] KVM: SEV: PreventHostIBS enablement for SEV-ES and SNP guest

[Manali Shukla]

Currently, the hypervisor is able to inspect instruction based samples
from a guest and gather execution information. SEV-ES and SNP guests
can disallow the use of instruction based sampling by hypervisor by
enabling the PreventHostIBS feature for the guest.  (More information
in Section 15.36.17 APM Volume 2)

The MSR_AMD64_IBSFETCHCTL[IbsFetchEn] and MSR_AMD64_IBSOPCTL[IbsOpEn]
bits need to be disabled before VMRUN is called when PreventHostIBS
feature is enabled. If either of these bits are not 0, VMRUN will fail
with VMEXIT_INVALID error code.

Because of an IBS race condition when disabling IBS, KVM needs to
indicate when it is in a PreventHostIBS window. Activate the window
based on whether IBS is currently active or inactive.

Signed-off-by: Manali Shukla <manali.shukla@amd.com>
---
 arch/x86/include/asm/cpufeatures.h |  1 +
 arch/x86/kvm/svm/sev.c             | 10 ++++++++
 arch/x86/kvm/svm/svm.c             | 39 ++++++++++++++++++++++++++++--
 arch/x86/kvm/svm/svm.h             |  1 +
 4 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 61012476d66e..1812e74f846a 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -425,6 +425,7 @@
 #define X86_FEATURE_SEV_ES		(19*32+ 3) /* AMD Secure Encrypted Virtualization - Encrypted State */
 #define X86_FEATURE_V_TSC_AUX		(19*32+ 9) /* "" Virtual TSC_AUX */
 #define X86_FEATURE_SME_COHERENT	(19*32+10) /* "" AMD hardware-enforced cache coherency */
+#define X86_FEATURE_PREVENT_HOST_IBS	(19*32+15) /* "" AMD prevent host ibs */
 
 /*
  * BUG word(s)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 86d6897f4806..b348b8931721 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -569,6 +569,12 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
 	if (svm->vcpu.guest_debug || (svm->vmcb->save.dr7 & ~DR7_FIXED_1))
 		return -EINVAL;
 
+	if (sev_es_guest(svm->vcpu.kvm) &&
+	    guest_cpuid_has(&svm->vcpu, X86_FEATURE_PREVENT_HOST_IBS)) {
+		save->sev_features |= BIT(6);
+		svm->prevent_hostibs_enabled = true;
+	}
+
 	/*
 	 * SEV-ES will use a VMSA that is pointed to by the VMCB, not
 	 * the traditional VMSA that is part of the VMCB. Copy the
@@ -2158,6 +2164,10 @@ void __init sev_set_cpu_caps(void)
 		kvm_cpu_cap_clear(X86_FEATURE_SEV);
 	if (!sev_es_enabled)
 		kvm_cpu_cap_clear(X86_FEATURE_SEV_ES);
+
+	/* Enable PreventhostIBS feature for SEV-ES and higher guests */
+	if (sev_es_enabled)
+		kvm_cpu_cap_set(X86_FEATURE_PREVENT_HOST_IBS);
 }
 
 void __init sev_hardware_setup(void)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 9a194aa1a75a..47c1e0fff23e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3914,10 +3914,39 @@ static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu, bool spec_ctrl_in
 
 	guest_state_enter_irqoff();
 
-	if (sev_es_guest(vcpu->kvm))
+	if (sev_es_guest(vcpu->kvm)) {
+		bool ibs_fetch_active, ibs_op_active;
+		u64 ibs_fetch_ctl, ibs_op_ctl;
+
+		if (svm->prevent_hostibs_enabled) {
+			/*
+			 * With PreventHostIBS enabled, IBS profiling cannot
+			 * be active when VMRUN is executed. Disable IBS before
+			 * executing VMRUN and, because of a race condition,
+			 * enable the PreventHostIBS window if IBS profiling was
+			 * active.
+			 */
+			ibs_fetch_active =
+				amd_disable_ibs_fetch(&ibs_fetch_ctl);
+			ibs_op_active =
+				amd_disable_ibs_op(&ibs_op_ctl);
+
+			amd_prevent_hostibs_window(ibs_fetch_active ||
+						   ibs_op_active);
+		}
+
 		__svm_sev_es_vcpu_run(svm, spec_ctrl_intercepted);
-	else
+
+		if (svm->prevent_hostibs_enabled) {
+			if (ibs_fetch_active)
+				amd_restore_ibs_fetch(ibs_fetch_ctl);
+
+			if (ibs_op_active)
+				amd_restore_ibs_op(ibs_op_ctl);
+		}
+	} else {
 		__svm_vcpu_run(svm, spec_ctrl_intercepted);
+	}
 
 	guest_state_exit_irqoff();
 }
@@ -4008,6 +4037,12 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
 
 	/* Any pending NMI will happen here */
 
+	/*
+	 * Disable the PreventHostIBS window since any pending IBS NMIs will
+	 * have been handled.
+	 */
+	amd_prevent_hostibs_window(false);
+
 	if (unlikely(svm->vmcb->control.exit_code == SVM_EXIT_NMI))
 		kvm_after_interrupt(vcpu);
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 4826e6cc611b..71f32fcfd219 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -254,6 +254,7 @@ struct vcpu_svm {
 	bool pause_filter_enabled         : 1;
 	bool pause_threshold_enabled      : 1;
 	bool vgif_enabled                 : 1;
+	bool prevent_hostibs_enabled      : 1;
 
 	u32 ldr_reg;
 	u32 dfr_reg;
-- 
2.34.1

Re: [RFC PATCH kernel 1/2] perf/x86/amd: Add amd_prevent_hostibs_window() to set per-cpu ibs_flags

[Peter Zijlstra]

On Mon, Feb 06, 2023 at 06:05:44AM +0000, Manali Shukla wrote:
> Add a function to set per-cpu ibs_flags based on an active or inactive
> PreventHostIBS window.
> 
> MSR_AMD64_IBSFETCHCTL[IbsFetchEn] and MSR_AMD64_IBSOPCTL[IbsOpEn] bits
> need to be cleared for PreventHostIBS feature to be enabled before VMRUN
> is executed.
> 
> ENABLE bit and VALID bit for MSR_AMD64_IBSFETCHCTL are contained in the
> same MSR and same is the case with MSR_AMD64_IBSOPCTL.
> 
> Consider the following scenario:
> - The IBS MSR which has ENABLE bit set and VALID bit clear is read.
> - During the process of clearing the ENABLE bit and writing the IBS MSR
>   to disable IBS, an IBS event can occur that sets the VALID bit.
> - The write operation on IBS MSR can clear the newly set VALID bit.
> - Since this situation is occurring in the CLGI/STGI window
>   (PreventHostIBS window), the actual NMI is not taken.
> - Once VMRUN is issued, it will exit with VMEXIT_NMI. As soon as STGI is
>   executed, the pending NMI will trigger.
> - The IBS NMI handler checks for the VALID bit to determine if the NMI
>   is generated because of IBS.
> - Since VALID bit is now clear, it doesn't recognize that an IBS event
>   is occurred. Due to this reason, the dazed and confused unknown NMI
>   messages are generated.
> 
> amd_prevent_hostibs_window() is added to avoid these messages when
> PreventHostIBS window is active and PreventHostIBS feature is enabled
> for the guest.
> 
> Signed-off-by: Manali Shukla <manali.shukla@amd.com>

URGH... so am I reading this right that this is a sodding terrible
software implementation of perf_event_attr::exclude_guest ?

Re: [RFC PATCH kernel 1/2] perf/x86/amd: Add amd_prevent_hostibs_window() to set per-cpu ibs_flags

[Shukla, Manali]

On 2/13/2023 6:40 PM, Peter Zijlstra wrote:
> On Mon, Feb 06, 2023 at 06:05:44AM +0000, Manali Shukla wrote:
>> Add a function to set per-cpu ibs_flags based on an active or inactive
>> PreventHostIBS window.
>>
>> MSR_AMD64_IBSFETCHCTL[IbsFetchEn] and MSR_AMD64_IBSOPCTL[IbsOpEn] bits
>> need to be cleared for PreventHostIBS feature to be enabled before VMRUN
>> is executed.
>>
>> ENABLE bit and VALID bit for MSR_AMD64_IBSFETCHCTL are contained in the
>> same MSR and same is the case with MSR_AMD64_IBSOPCTL.
>>
>> Consider the following scenario:
>> - The IBS MSR which has ENABLE bit set and VALID bit clear is read.
>> - During the process of clearing the ENABLE bit and writing the IBS MSR
>>   to disable IBS, an IBS event can occur that sets the VALID bit.
>> - The write operation on IBS MSR can clear the newly set VALID bit.
>> - Since this situation is occurring in the CLGI/STGI window
>>   (PreventHostIBS window), the actual NMI is not taken.
>> - Once VMRUN is issued, it will exit with VMEXIT_NMI. As soon as STGI is
>>   executed, the pending NMI will trigger.
>> - The IBS NMI handler checks for the VALID bit to determine if the NMI
>>   is generated because of IBS.
>> - Since VALID bit is now clear, it doesn't recognize that an IBS event
>>   is occurred. Due to this reason, the dazed and confused unknown NMI
>>   messages are generated.
>>
>> amd_prevent_hostibs_window() is added to avoid these messages when
>> PreventHostIBS window is active and PreventHostIBS feature is enabled
>> for the guest.
>>
>> Signed-off-by: Manali Shukla <manali.shukla@amd.com>
> 
> URGH... so am I reading this right that this is a sodding terrible
> software implementation of perf_event_attr::exclude_guest ?

Not exactly. 
Unlike exclude_guest where profiler decides whether it wants to trace
guest data or not, PreventHostIBS gives control to the Guest. Secured 
guests(SEV-ES/SEV-SNP) can disallow the use of IBS by the hypervisor,
in order to limit the information which can be gathered by host from
its execution.

-Manali

Re: [RFC PATCH kernel 1/2] perf/x86/amd: Add amd_prevent_hostibs_window() to set per-cpu ibs_flags

[Ravi Bangoria]

On 06-Feb-23 11:35 AM, Manali Shukla wrote:
> Add a function to set per-cpu ibs_flags based on an active or inactive
> PreventHostIBS window.
> 
> MSR_AMD64_IBSFETCHCTL[IbsFetchEn] and MSR_AMD64_IBSOPCTL[IbsOpEn] bits
> need to be cleared for PreventHostIBS feature to be enabled before VMRUN
> is executed.
> 
> ENABLE bit and VALID bit for MSR_AMD64_IBSFETCHCTL are contained in the
> same MSR and same is the case with MSR_AMD64_IBSOPCTL.
> 
> Consider the following scenario:
> - The IBS MSR which has ENABLE bit set and VALID bit clear is read.
> - During the process of clearing the ENABLE bit and writing the IBS MSR
>   to disable IBS, an IBS event can occur that sets the VALID bit.
> - The write operation on IBS MSR can clear the newly set VALID bit.
> - Since this situation is occurring in the CLGI/STGI window
>   (PreventHostIBS window), the actual NMI is not taken.
> - Once VMRUN is issued, it will exit with VMEXIT_NMI. As soon as STGI is
>   executed, the pending NMI will trigger.
> - The IBS NMI handler checks for the VALID bit to determine if the NMI
>   is generated because of IBS.
> - Since VALID bit is now clear, it doesn't recognize that an IBS event
>   is occurred. Due to this reason, the dazed and confused unknown NMI
>   messages are generated.
> 
> amd_prevent_hostibs_window() is added to avoid these messages when
> PreventHostIBS window is active and PreventHostIBS feature is enabled
> for the guest.
> 
> Signed-off-by: Manali Shukla <manali.shukla@amd.com>

LGTM.

Reviewed-by: Ravi Bangoria <ravi.bangoria@amd.com>

Re: [RFC PATCH kernel 0/2] PreventHostIBS feature for SEV-ES and SNP guests

[Manali Shukla]

On 2/6/2023 11:35 AM, Manali Shukla wrote:
> Adds support for PreventHostIBS feature for SEV-ES and SNP guests.
> Currently, the hypervisor is able to inspect instruction based samples
> from the guest and gather execution information.  With enablement of
> PreventHostIBS feature, SEV-ES and SNP guests may choose to disallow
> use of instruction based sampling by the hypervisor in order to limit
> the information gathered about their execution.  (More information in
> Section 15.36.17 APM Volume 2)
> 
> While implementing this feature, unknown NMIs were being seen. On
> further investigation, a race was found effecting the IBS FETCH/OP
> MSR.
> 
> ENABLE bit and VALID bit for IBS_FETCH_CTL are contained in the same
> MSR and same is the case with IBS_OP_CTL.
> 
> Consider the following scenario:
> - The IBS MSR which has ENABLE bit set and VALID bit clear is read.
> - During the process of clearing the ENABLE bit and writing the IBS
>   MSR to disable IBS, an IBS event can occur that sets the VALID bit.
> - The write operation on IBS MSR can clear the newly set VALID bit.
> - Since this situation is occurring in the CLGI/STGI window
>   (PreventHostIBS window), the actual NMI is not taken.
> - Once VMRUN is issued, it will exit with VMEXIT_NMI and as soon as
>   STGI is executed, the pending NMI will trigger.
> - The IBS NMI handler checks for the VALID bit to determine if the NMI
>   is generated because of IBS.
> - Since VALID bit is now clear, it doesn't recognize that an IBS event
>   is occurred which in turn generates the dazed and confused unknown
>   NMI messages.
> 
> Per-cpu ibs_flags which indicates whether PreventHostIBS window is
> active/inactive are added to avoid the above mentioned race.
> 
> An active PreventHostIBS window is set before calling VMRUN and
> cleared after STGI. PreventHostIBS window check is added to
> perf_ibs_handle_irq(), to avoid unknown NMIs and treat them as handled
> when window is active.
> 
> There are 2 patches in this series.
> 1) Add amd_prevent_hostibs_window() function to set per-cpu ibs_flags
> based on an active/inactive PreventHostIBS window.
> 2) Enable PreventHostIBS for SEV-ES and SNP guests.
> 
> Testing done:
> - Executed program symbols in guest are not captured in host when
>   PreventHostIBS feature is enabled.
> - Generated 1000+ NMIs using cpuid command, no unknown NMIs are seen
>   after enablement of PreventHostIBS feature.
> 
> Qemu commandline to enable PreventHostIBS on guest.
> 
> qemu-system-x86_64 -enable-kvm -cpu EPYC-v4,+nohostibs \ ..
> 
> Manali Shukla (2):
>   perf/x86/amd: Add amd_prevent_hostibs_window() to set per-cpu
>     ibs_flags
>   KVM: SEV: PreventHostIBS enablement for SEV-ES and SNP guest
> 
>  arch/x86/events/amd/ibs.c          | 64 ++++++++++++++++++++++++++++++
>  arch/x86/include/asm/cpufeatures.h |  1 +
>  arch/x86/include/asm/perf_event.h  | 20 ++++++++++
>  arch/x86/kvm/svm/sev.c             | 10 +++++
>  arch/x86/kvm/svm/svm.c             | 39 +++++++++++++++++-
>  arch/x86/kvm/svm/svm.h             |  1 +
>  6 files changed, 133 insertions(+), 2 deletions(-)
> 

A gentle reminder for the review.

-Manali

Re: [RFC PATCH kernel 2/2] KVM: SEV: PreventHostIBS enablement for SEV-ES and SNP guest

[Nikunj A. Dadhania]

On 06/02/23 11:35, Manali Shukla wrote:
> Currently, the hypervisor is able to inspect instruction based samples
> from a guest and gather execution information. SEV-ES and SNP guests
> can disallow the use of instruction based sampling by hypervisor by
> enabling the PreventHostIBS feature for the guest.  (More information
> in Section 15.36.17 APM Volume 2)
> 
> The MSR_AMD64_IBSFETCHCTL[IbsFetchEn] and MSR_AMD64_IBSOPCTL[IbsOpEn]
> bits need to be disabled before VMRUN is called when PreventHostIBS
> feature is enabled. If either of these bits are not 0, VMRUN will fail
> with VMEXIT_INVALID error code.
> 
> Because of an IBS race condition when disabling IBS, KVM needs to
> indicate when it is in a PreventHostIBS window. Activate the window
> based on whether IBS is currently active or inactive.
> 
> Signed-off-by: Manali Shukla <manali.shukla@amd.com>

Looks good.

Reviewed-by: Nikunj A Dadhania <nikunj@amd.com>

Re: [RFC PATCH kernel 0/2] PreventHostIBS feature for SEV-ES and SNP guests

[Manali Shukla]

On 3/15/2023 10:33 AM, Manali Shukla wrote:
> On 2/6/2023 11:35 AM, Manali Shukla wrote:
>> Adds support for PreventHostIBS feature for SEV-ES and SNP guests.
>> Currently, the hypervisor is able to inspect instruction based samples
>> from the guest and gather execution information.  With enablement of
>> PreventHostIBS feature, SEV-ES and SNP guests may choose to disallow
>> use of instruction based sampling by the hypervisor in order to limit
>> the information gathered about their execution.  (More information in
>> Section 15.36.17 APM Volume 2)
>>
>> While implementing this feature, unknown NMIs were being seen. On
>> further investigation, a race was found effecting the IBS FETCH/OP
>> MSR.
>>
>> ENABLE bit and VALID bit for IBS_FETCH_CTL are contained in the same
>> MSR and same is the case with IBS_OP_CTL.
>>
>> Consider the following scenario:
>> - The IBS MSR which has ENABLE bit set and VALID bit clear is read.
>> - During the process of clearing the ENABLE bit and writing the IBS
>>   MSR to disable IBS, an IBS event can occur that sets the VALID bit.
>> - The write operation on IBS MSR can clear the newly set VALID bit.
>> - Since this situation is occurring in the CLGI/STGI window
>>   (PreventHostIBS window), the actual NMI is not taken.
>> - Once VMRUN is issued, it will exit with VMEXIT_NMI and as soon as
>>   STGI is executed, the pending NMI will trigger.
>> - The IBS NMI handler checks for the VALID bit to determine if the NMI
>>   is generated because of IBS.
>> - Since VALID bit is now clear, it doesn't recognize that an IBS event
>>   is occurred which in turn generates the dazed and confused unknown
>>   NMI messages.
>>
>> Per-cpu ibs_flags which indicates whether PreventHostIBS window is
>> active/inactive are added to avoid the above mentioned race.
>>
>> An active PreventHostIBS window is set before calling VMRUN and
>> cleared after STGI. PreventHostIBS window check is added to
>> perf_ibs_handle_irq(), to avoid unknown NMIs and treat them as handled
>> when window is active.
>>
>> There are 2 patches in this series.
>> 1) Add amd_prevent_hostibs_window() function to set per-cpu ibs_flags
>> based on an active/inactive PreventHostIBS window.
>> 2) Enable PreventHostIBS for SEV-ES and SNP guests.
>>
>> Testing done:
>> - Executed program symbols in guest are not captured in host when
>>   PreventHostIBS feature is enabled.
>> - Generated 1000+ NMIs using cpuid command, no unknown NMIs are seen
>>   after enablement of PreventHostIBS feature.
>>
>> Qemu commandline to enable PreventHostIBS on guest.
>>
>> qemu-system-x86_64 -enable-kvm -cpu EPYC-v4,+nohostibs \ ..
>>
>> Manali Shukla (2):
>>   perf/x86/amd: Add amd_prevent_hostibs_window() to set per-cpu
>>     ibs_flags
>>   KVM: SEV: PreventHostIBS enablement for SEV-ES and SNP guest
>>
>>  arch/x86/events/amd/ibs.c          | 64 ++++++++++++++++++++++++++++++
>>  arch/x86/include/asm/cpufeatures.h |  1 +
>>  arch/x86/include/asm/perf_event.h  | 20 ++++++++++
>>  arch/x86/kvm/svm/sev.c             | 10 +++++
>>  arch/x86/kvm/svm/svm.c             | 39 +++++++++++++++++-
>>  arch/x86/kvm/svm/svm.h             |  1 +
>>  6 files changed, 133 insertions(+), 2 deletions(-)
>>
> 
> A gentle reminder for the review.
> 
> -Manali

A gentle reminder for the review.

-Manali

Re: [RFC PATCH kernel 2/2] KVM: SEV: PreventHostIBS enablement for SEV-ES and SNP guest

[Sean Christopherson]

On Mon, Feb 06, 2023, Manali Shukla wrote:
> Currently, the hypervisor is able to inspect instruction based samples
> from a guest and gather execution information. SEV-ES and SNP guests
> can disallow the use of instruction based sampling by hypervisor by
> enabling the PreventHostIBS feature for the guest.  (More information
> in Section 15.36.17 APM Volume 2)
> 
> The MSR_AMD64_IBSFETCHCTL[IbsFetchEn] and MSR_AMD64_IBSOPCTL[IbsOpEn]
> bits need to be disabled before VMRUN is called when PreventHostIBS
> feature is enabled. If either of these bits are not 0, VMRUN will fail
> with VMEXIT_INVALID error code.
> 
> Because of an IBS race condition when disabling IBS, KVM needs to
> indicate when it is in a PreventHostIBS window. Activate the window
> based on whether IBS is currently active or inactive.
> 
> Signed-off-by: Manali Shukla <manali.shukla@amd.com>
> ---
>  arch/x86/include/asm/cpufeatures.h |  1 +
>  arch/x86/kvm/svm/sev.c             | 10 ++++++++
>  arch/x86/kvm/svm/svm.c             | 39 ++++++++++++++++++++++++++++--
>  arch/x86/kvm/svm/svm.h             |  1 +
>  4 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 61012476d66e..1812e74f846a 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -425,6 +425,7 @@
>  #define X86_FEATURE_SEV_ES		(19*32+ 3) /* AMD Secure Encrypted Virtualization - Encrypted State */
>  #define X86_FEATURE_V_TSC_AUX		(19*32+ 9) /* "" Virtual TSC_AUX */
>  #define X86_FEATURE_SME_COHERENT	(19*32+10) /* "" AMD hardware-enforced cache coherency */
> +#define X86_FEATURE_PREVENT_HOST_IBS	(19*32+15) /* "" AMD prevent host ibs */
>  
>  /*
>   * BUG word(s)
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 86d6897f4806..b348b8931721 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -569,6 +569,12 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>  	if (svm->vcpu.guest_debug || (svm->vmcb->save.dr7 & ~DR7_FIXED_1))
>  		return -EINVAL;
>  
> +	if (sev_es_guest(svm->vcpu.kvm) &&
> +	    guest_cpuid_has(&svm->vcpu, X86_FEATURE_PREVENT_HOST_IBS)) {
> +		save->sev_features |= BIT(6);
> +		svm->prevent_hostibs_enabled = true;
> +	}
> +
>  	/*
>  	 * SEV-ES will use a VMSA that is pointed to by the VMCB, not
>  	 * the traditional VMSA that is part of the VMCB. Copy the
> @@ -2158,6 +2164,10 @@ void __init sev_set_cpu_caps(void)
>  		kvm_cpu_cap_clear(X86_FEATURE_SEV);
>  	if (!sev_es_enabled)
>  		kvm_cpu_cap_clear(X86_FEATURE_SEV_ES);
> +
> +	/* Enable PreventhostIBS feature for SEV-ES and higher guests */
> +	if (sev_es_enabled)
> +		kvm_cpu_cap_set(X86_FEATURE_PREVENT_HOST_IBS);

Uh, you can't just force a cap, there needs to be actual hardware support.  Just
copy what was done for X86_FEATURE_SEV_ES.


>  }
>  
>  void __init sev_hardware_setup(void)
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 9a194aa1a75a..47c1e0fff23e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3914,10 +3914,39 @@ static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu, bool spec_ctrl_in
>  
>  	guest_state_enter_irqoff();
>  
> -	if (sev_es_guest(vcpu->kvm))
> +	if (sev_es_guest(vcpu->kvm)) {
> +		bool ibs_fetch_active, ibs_op_active;
> +		u64 ibs_fetch_ctl, ibs_op_ctl;
> +
> +		if (svm->prevent_hostibs_enabled) {
> +			/*
> +			 * With PreventHostIBS enabled, IBS profiling cannot
> +			 * be active when VMRUN is executed. Disable IBS before
> +			 * executing VMRUN and, because of a race condition,
> +			 * enable the PreventHostIBS window if IBS profiling was
> +			 * active.

And the race can't be fixed because...?

> +			 */
> +			ibs_fetch_active =
> +				amd_disable_ibs_fetch(&ibs_fetch_ctl);
> +			ibs_op_active =
> +				amd_disable_ibs_op(&ibs_op_ctl);
> +
> +			amd_prevent_hostibs_window(ibs_fetch_active ||
> +						   ibs_op_active);
> +		}
> +
>  		__svm_sev_es_vcpu_run(svm, spec_ctrl_intercepted);
> -	else
> +
> +		if (svm->prevent_hostibs_enabled) {
> +			if (ibs_fetch_active)
> +				amd_restore_ibs_fetch(ibs_fetch_ctl);
> +
> +			if (ibs_op_active)
> +				amd_restore_ibs_op(ibs_op_ctl);

IIUC, this adds up to 2 RDMSRs and 4 WRMSRs to the VMRUN path.  Blech.  There's
gotta be a better way to implement this.  Like PeterZ said, this is basically
exclude_guest.

Re: [RFC PATCH kernel 2/2] KVM: SEV: PreventHostIBS enablement for SEV-ES and SNP guest

[Manali Shukla]

On 3/25/2023 1:25 AM, Sean Christopherson wrote:
> On Mon, Feb 06, 2023, Manali Shukla wrote:
>> Currently, the hypervisor is able to inspect instruction based samples
>> from a guest and gather execution information. SEV-ES and SNP guests
>> can disallow the use of instruction based sampling by hypervisor by
>> enabling the PreventHostIBS feature for the guest.  (More information
>> in Section 15.36.17 APM Volume 2)
>>
>> The MSR_AMD64_IBSFETCHCTL[IbsFetchEn] and MSR_AMD64_IBSOPCTL[IbsOpEn]
>> bits need to be disabled before VMRUN is called when PreventHostIBS
>> feature is enabled. If either of these bits are not 0, VMRUN will fail
>> with VMEXIT_INVALID error code.
>>
>> Because of an IBS race condition when disabling IBS, KVM needs to
>> indicate when it is in a PreventHostIBS window. Activate the window
>> based on whether IBS is currently active or inactive.
>>
>> Signed-off-by: Manali Shukla <manali.shukla@amd.com>
>> ---
>>  arch/x86/include/asm/cpufeatures.h |  1 +
>>  arch/x86/kvm/svm/sev.c             | 10 ++++++++
>>  arch/x86/kvm/svm/svm.c             | 39 ++++++++++++++++++++++++++++--
>>  arch/x86/kvm/svm/svm.h             |  1 +
>>  4 files changed, 49 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>> index 61012476d66e..1812e74f846a 100644
>> --- a/arch/x86/include/asm/cpufeatures.h
>> +++ b/arch/x86/include/asm/cpufeatures.h
>> @@ -425,6 +425,7 @@
>>  #define X86_FEATURE_SEV_ES		(19*32+ 3) /* AMD Secure Encrypted Virtualization - Encrypted State */
>>  #define X86_FEATURE_V_TSC_AUX		(19*32+ 9) /* "" Virtual TSC_AUX */
>>  #define X86_FEATURE_SME_COHERENT	(19*32+10) /* "" AMD hardware-enforced cache coherency */
>> +#define X86_FEATURE_PREVENT_HOST_IBS	(19*32+15) /* "" AMD prevent host ibs */
>>  
>>  /*
>>   * BUG word(s)
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 86d6897f4806..b348b8931721 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -569,6 +569,12 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>>  	if (svm->vcpu.guest_debug || (svm->vmcb->save.dr7 & ~DR7_FIXED_1))
>>  		return -EINVAL;
>>  
>> +	if (sev_es_guest(svm->vcpu.kvm) &&
>> +	    guest_cpuid_has(&svm->vcpu, X86_FEATURE_PREVENT_HOST_IBS)) {
>> +		save->sev_features |= BIT(6);
>> +		svm->prevent_hostibs_enabled = true;
>> +	}
>> +
>>  	/*
>>  	 * SEV-ES will use a VMSA that is pointed to by the VMCB, not
>>  	 * the traditional VMSA that is part of the VMCB. Copy the
>> @@ -2158,6 +2164,10 @@ void __init sev_set_cpu_caps(void)
>>  		kvm_cpu_cap_clear(X86_FEATURE_SEV);
>>  	if (!sev_es_enabled)
>>  		kvm_cpu_cap_clear(X86_FEATURE_SEV_ES);
>> +
>> +	/* Enable PreventhostIBS feature for SEV-ES and higher guests */
>> +	if (sev_es_enabled)
>> +		kvm_cpu_cap_set(X86_FEATURE_PREVENT_HOST_IBS);
> 
> Uh, you can't just force a cap, there needs to be actual hardware support.  Just
> copy what was done for X86_FEATURE_SEV_ES.

Okay. I will do the suggested changes.
> 
> 
>>  }
>>  
>>  void __init sev_hardware_setup(void)
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 9a194aa1a75a..47c1e0fff23e 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -3914,10 +3914,39 @@ static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu, bool spec_ctrl_in
>>  
>>  	guest_state_enter_irqoff();
>>  
>> -	if (sev_es_guest(vcpu->kvm))
>> +	if (sev_es_guest(vcpu->kvm)) {
>> +		bool ibs_fetch_active, ibs_op_active;
>> +		u64 ibs_fetch_ctl, ibs_op_ctl;
>> +
>> +		if (svm->prevent_hostibs_enabled) {
>> +			/*
>> +			 * With PreventHostIBS enabled, IBS profiling cannot
>> +			 * be active when VMRUN is executed. Disable IBS before
>> +			 * executing VMRUN and, because of a race condition,
>> +			 * enable the PreventHostIBS window if IBS profiling was
>> +			 * active.
> 
> And the race can't be fixed because...?

Race can not be fixed because VALID and ENABLE bit for IBS_FETCH_CTL and IBS_OP_CTL
are contained in their same resepective MSRs. Due to this reason following scenario can
be generated:
Read IBS_FETCH_CTL (IbsFetchEn bit is 1 and IBSFetchVal bit is 0)
Write IBS_FETCH_CTL (IbsFetchEn is 0 now)
Imagine in between Read and Write, IBSFetchVal changes to 1. Write to IBS_FETCH_CTL will
clear the IBSFetchVal bit. When STGI is executed after VMEXIT, the NMI is taken and check for
valid mask will fail and generate Dazed and Confused NMI messages.
Please refer to cover letter for more details.

> 
>> +			 */
>> +			ibs_fetch_active =
>> +				amd_disable_ibs_fetch(&ibs_fetch_ctl);
>> +			ibs_op_active =
>> +				amd_disable_ibs_op(&ibs_op_ctl);
>> +
>> +			amd_prevent_hostibs_window(ibs_fetch_active ||
>> +						   ibs_op_active);
>> +		}
>> +
>>  		__svm_sev_es_vcpu_run(svm, spec_ctrl_intercepted);
>> -	else
>> +
>> +		if (svm->prevent_hostibs_enabled) {
>> +			if (ibs_fetch_active)
>> +				amd_restore_ibs_fetch(ibs_fetch_ctl);
>> +
>> +			if (ibs_op_active)
>> +				amd_restore_ibs_op(ibs_op_ctl);
> 
> IIUC, this adds up to 2 RDMSRs and 4 WRMSRs to the VMRUN path.  Blech.  There's
> gotta be a better way to implement this.  

I will try to find a better way to implement this.

> Like PeterZ said, this is basically
> exclude_guest.

As I mentioned before, exclude_guest lets the profiler decide whether it wants to trace the guest
data or not, whereas PreventHostIBS lets the owner of the guest decide whether host can trace guest's
data or not.


Thank you for reviewing the patches.

- Manali

Re: [RFC PATCH kernel 2/2] KVM: SEV: PreventHostIBS enablement for SEV-ES and SNP guest

[Sean Christopherson]

On Wed, Mar 29, 2023, Manali Shukla wrote:
> On 3/25/2023 1:25 AM, Sean Christopherson wrote:
> > On Mon, Feb 06, 2023, Manali Shukla wrote:
> >> -	if (sev_es_guest(vcpu->kvm))
> >> +	if (sev_es_guest(vcpu->kvm)) {
> >> +		bool ibs_fetch_active, ibs_op_active;
> >> +		u64 ibs_fetch_ctl, ibs_op_ctl;
> >> +
> >> +		if (svm->prevent_hostibs_enabled) {
> >> +			/*
> >> +			 * With PreventHostIBS enabled, IBS profiling cannot
> >> +			 * be active when VMRUN is executed. Disable IBS before
> >> +			 * executing VMRUN and, because of a race condition,
> >> +			 * enable the PreventHostIBS window if IBS profiling was
> >> +			 * active.
> > 
> > And the race can't be fixed because...?
> 
> Race can not be fixed because VALID and ENABLE bit for IBS_FETCH_CTL and IBS_OP_CTL
> are contained in their same resepective MSRs. Due to this reason following scenario can
> be generated:
> Read IBS_FETCH_CTL (IbsFetchEn bit is 1 and IBSFetchVal bit is 0)
> Write IBS_FETCH_CTL (IbsFetchEn is 0 now)
> Imagine in between Read and Write, IBSFetchVal changes to 1. Write to IBS_FETCH_CTL will
> clear the IBSFetchVal bit. When STGI is executed after VMEXIT, the NMI is taken and check for
> valid mask will fail and generate Dazed and Confused NMI messages.
> Please refer to cover letter for more details.

I understand the race, I'm asking why this series doesn't fix the race.  Effectively
suppressing potentially unexpected NMIs because PreventHostIBS was enable is ugly.

> >> +			 */
> >> +			ibs_fetch_active =
> >> +				amd_disable_ibs_fetch(&ibs_fetch_ctl);
> >> +			ibs_op_active =
> >> +				amd_disable_ibs_op(&ibs_op_ctl);
> >> +
> >> +			amd_prevent_hostibs_window(ibs_fetch_active ||
> >> +						   ibs_op_active);
> >> +		}
> >> +
> >>  		__svm_sev_es_vcpu_run(svm, spec_ctrl_intercepted);
> >> -	else
> >> +
> >> +		if (svm->prevent_hostibs_enabled) {
> >> +			if (ibs_fetch_active)
> >> +				amd_restore_ibs_fetch(ibs_fetch_ctl);
> >> +
> >> +			if (ibs_op_active)
> >> +				amd_restore_ibs_op(ibs_op_ctl);
> > 
> > IIUC, this adds up to 2 RDMSRs and 4 WRMSRs to the VMRUN path.  Blech.  There's
> > gotta be a better way to implement this.  
> 
> I will try to find a better way to implement this.
> 
> > Like PeterZ said, this is basically
> > exclude_guest.
> 
> As I mentioned before, exclude_guest lets the profiler decide whether it wants to trace the guest
> data or not, whereas PreventHostIBS lets the owner of the guest decide whether host can trace guest's
> data or not.

PreventHostIBS is purely an enforcement, it does not actually do anything to
disable tracing of the guest.  What PeterZ and I are complaining about is that
instead of integrating this feature with exclude_guest, e.g. finding a way to
make guest tracing mutually exclusive with KVM_RUN so that PreventHostIBS can be
contexted switched according, this series instead backdoors into perf to forcefully
disable tracing.

In other words, please try to create a sane contract between userspace, perf, and
KVM, e.g. disallow tracing a guest with PreventHostIBS at some level instead of
silently toggling tracing around VMRUN.