Re: [PATCH 1/2] powerpc/pseries/svm: Don't access some SPRs

[Michael Ellerman <mpe@ellerman.id.au>]

Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> 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