Re: [PATCH v3 2/2] powerpc/pseries/svm: Disable BHRB/EBB/PMU access

[Paul Mackerras]

On Thu, Jan 09, 2020 at 09:19:57PM -0800, Sukadev Bhattiprolu wrote:
> Ultravisor disables some CPU features like BHRB, EBB and PMU in
> secure virtual machines (SVMs). Skip accessing those registers
> in SVMs to avoid getting a Program Interrupt.

It would be useful to have more explanation of the rationale for the
ultravisor disabling access to those features, and indicate whether
this is a temporary restriction or a permanent one.  If SVMs are never
going to be able to use the PMU then that is a bad thing in my
opinion.  In other words, the commit message should tell us whether
the restriction is just because the ultravisor doesn't yet have code
for managing and context-switching the PMU, or if there is there some
reason why using the PMU in a SVM will always be prohibited for some
security-related reason.

Also, the only way that a SVM would be getting into the KVM code that
you are patching is if it is trying to do nested virtualization.
However, the SVM should already know that it is not able to do nested
virtualization because the ultravisor should be intercepting and
failing the H_SET_PARTITION_TABLE hypercall.  So I think there is no
good reason for patching the KVM code like you are doing unless the
PMU restriction is permanent and we are intending someday to enable
SVMs to have nested guests.

Paul.

[PATCH v3 2/2] powerpc/pseries/svm: Disable BHRB/EBB/PMU access

[Sukadev Bhattiprolu]

Ultravisor disables some CPU features like BHRB, EBB and PMU in
secure virtual machines (SVMs). Skip accessing those registers
in SVMs to avoid getting a Program Interrupt.

Signed-off-by: Sukadev Bhattiprolu 
Acked-by: Madhavan Srinivasan 
---
Changelog[v2]
- [Michael Ellerman] Optimize the code using FW_FEATURE_SVM
- Merged EBB/BHRB and PMU patches into one and reorganized code.
- Fix some build errors reported by 
---
 arch/powerpc/kernel/cpu_setup_power.S   | 21 
 arch/powerpc/kernel/process.c   | 23 ++---
 arch/powerpc/kvm/book3s_hv.c| 33 -
 arch/powerpc/kvm/book3s_hv_rmhandlers.S | 32 +++-
 arch/powerpc/kvm/book3s_hv_tm_builtin.c | 21 ++--
 arch/powerpc/perf/core-book3s.c |  6 +
 arch/powerpc/xmon/xmon.c| 30 +-
 7 files changed, 114 insertions(+), 52 deletions(-)

diff --git a/arch/powerpc/kernel/cpu_setup_power.S 
b/arch/powerpc/kernel/cpu_setup_power.S
index a460298c7ddb..9e895d8db468 100644
--- a/arch/powerpc/kernel/cpu_setup_power.S
+++ b/arch/powerpc/kernel/cpu_setup_power.S
@@ -206,14 +206,35 @@ __init_PMU_HV_ISA207:
blr
 
 __init_PMU:
+#ifdef CONFIG_PPC_SVM
+   /*
+* SVM's are restricted from accessing PMU, so skip.
+*/
+   mfmsr   r5
+   rldicl  r5, r5, 64-MSR_S_LG, 62
+   cmpwi   r5,1
+   beq skip1
+#endif
li  r5,0
mtspr   SPRN_MMCRA,r5
mtspr   SPRN_MMCR0,r5
mtspr   SPRN_MMCR1,r5
mtspr   SPRN_MMCR2,r5
+skip1:
blr
 
 __init_PMU_ISA207:
+
+#ifdef CONFIG_PPC_SVM
+   /*
+* SVM's are restricted from accessing PMU, so skip.
+   */
+   mfmsr   r5
+   rldicl  r5, r5, 64-MSR_S_LG, 62
+   cmpwi   r5,1
+   beq skip2
+#endif
li  r5,0
mtspr   SPRN_MMCRS,r5
+skip2:
blr
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 639ceae7da9d..83c7c4119305 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -64,6 +64,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -1059,9 +1060,11 @@ static inline void save_sprs(struct thread_struct *t)
t->dscr = mfspr(SPRN_DSCR);
 
if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
-   t->bescr = mfspr(SPRN_BESCR);
-   t->ebbhr = mfspr(SPRN_EBBHR);
-   t->ebbrr = mfspr(SPRN_EBBRR);
+   if (!is_secure_guest()) {
+   t->bescr = mfspr(SPRN_BESCR);
+   t->ebbhr = mfspr(SPRN_EBBHR);
+   t->ebbrr = mfspr(SPRN_EBBRR);
+   }
 
t->fscr = mfspr(SPRN_FSCR);
 
@@ -1097,12 +1100,14 @@ static inline void restore_sprs(struct thread_struct 
*old_thread,
}
 
if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
-   if (old_thread->bescr != new_thread->bescr)
-   mtspr(SPRN_BESCR, new_thread->bescr);
-   if (old_thread->ebbhr != new_thread->ebbhr)
-   mtspr(SPRN_EBBHR, new_thread->ebbhr);
-   if (old_thread->ebbrr != new_thread->ebbrr)
-   mtspr(SPRN_EBBRR, new_thread->ebbrr);
+   if (!is_secure_guest()) {
+   if (old_thread->bescr != new_thread->bescr)
+   mtspr(SPRN_BESCR, new_thread->bescr);
+   if (old_thread->ebbhr != new_thread->ebbhr)
+   mtspr(SPRN_EBBHR, new_thread->ebbhr);
+   if (old_thread->ebbrr != new_thread->ebbrr)
+   mtspr(SPRN_EBBRR, new_thread->ebbrr);
+   }
 
if (old_thread->fscr != new_thread->fscr)
mtspr(SPRN_FSCR, new_thread->fscr);
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 709cf1fd4cf4..29a2640108d1 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -42,6 +42,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -3568,9 +3569,11 @@ int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 
time_limit,
mtspr(SPRN_PSPB, vcpu->arch.pspb);
mtspr(SPRN_FSCR, vcpu->arch.fscr);
mtspr(SPRN_TAR, vcpu->arch.tar);
-   mtspr(SPRN_EBBHR, vcpu->arch.ebbhr);
-   mtspr(SPRN_EBBRR, vcpu->arch.ebbrr);
-   mtspr(SPRN_BESCR, vcpu->arch.bescr);
+   if (!is_secure_guest()) {
+   mtspr(SPRN_EBBHR, vcpu->arch.ebbhr);
+   mtspr(SPRN_EBBRR, vcpu->arch.ebbrr);
+   mtspr(SPRN_BESCR, vcpu->arch.bescr);
+   }
mtspr(SPRN_WORT, vcpu->arch.wort);
mtspr(SPRN_TIDR, vcpu->arch.tid);
mtspr(SPRN_DAR, vcpu->arch.shregs.dar);
@@ -3641,9 +3644,11 @@ int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 
time_limit,
vcpu->arch.pspb = mfspr(SPRN_PSPB);