From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Ellerman Date: Thu, 19 Dec 2019 10:59:57 +0000 Subject: Re: [PATCH 1/2] powerpc/pseries/svm: Don't access some SPRs Message-Id: <87immcmvgy.fsf@mpe.ellerman.id.au> List-Id: References: <20191218043048.3400-1-sukadev@linux.ibm.com> <875zidoqok.fsf@mpe.ellerman.id.au> <20191218235753.GA12285@us.ibm.com> In-Reply-To: <20191218235753.GA12285@us.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Sukadev Bhattiprolu Cc: andmike@linux.ibm.com, linuxram@us.ibm.com, kvm-ppc@vger.kernel.org, linuxppc-dev@ozlabs.org, Sukadev Bhattiprolu , bauerman@linux.ibm.com Sukadev Bhattiprolu writes: > Michael Ellerman [mpe@ellerman.id.au] wrote: >> >> eg. here. >> >> This is the fast path of context switch. >> >> That expands to: >> >> if (!(mfmsr() & MSR_S)) >> asm volatile("mfspr %0, SPRN_BESCR" : "=r" (rval)); >> if (!(mfmsr() & MSR_S)) >> asm volatile("mfspr %0, SPRN_EBBHR" : "=r" (rval)); >> if (!(mfmsr() & MSR_S)) >> asm volatile("mfspr %0, SPRN_EBBRR" : "=r" (rval)); >> > > Yes, should have optimized this at least :-) >> >> If the Ultravisor is going to disable EBB and BHRB then we need new >> CPU_FTR bits for those, and the code that accesses those registers >> needs to be put behind cpu_has_feature(EBB) etc. > > Will try the cpu_has_feature(). Would it be ok to use a single feature > bit, like UV or make it per-register group as that could need more > feature bits? We already have a number of places using is_secure_guest(): arch/powerpc/include/asm/mem_encrypt.h: return is_secure_guest(); arch/powerpc/include/asm/mem_encrypt.h: return is_secure_guest(); arch/powerpc/include/asm/svm.h:#define get_dtl_cache_ctor() (is_secure_guest() ? dtl_cache_ctor : NULL) arch/powerpc/kernel/machine_kexec_64.c: if (is_secure_guest() && !(image->preserve_context || arch/powerpc/kernel/paca.c: if (is_secure_guest()) arch/powerpc/kernel/sysfs.c: return sprintf(buf, "%u\n", is_secure_guest()); arch/powerpc/platforms/pseries/iommu.c: if (!is_secure_guest()) arch/powerpc/platforms/pseries/smp.c: if (cpu_has_feature(CPU_FTR_DBELL) && !is_secure_guest()) arch/powerpc/platforms/pseries/svm.c: if (!is_secure_guest()) Which could all (or mostly) be converted to use a cpu_has_feature(CPU_FTR_SVM). So yeah I guess it makes sense to do that, create a CPU_FTR_SVM and set it early in boot based on MSR_S. You could argue it's a firmware feature, so should be FW_FEATURE_SVM, but we don't use jump_labels for firmware features so they're not as nice for hot-path code like register switching. Also the distinction between CPU and firmware features is a bit arbitrary. cheers