Re: [PATCH v4 4/8] KVM: PPC: Ultravisor: Use UV_WRITE_PATE ucall to register a PATE

[Claudio Carvalho <cclaudio@linux.ibm.com>]

On 7/11/19 9:57 AM, Michael Ellerman wrote:
>
>>  
>>  static pmd_t *get_pmd_from_cache(struct mm_struct *mm)
>> diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
>> index 8904aa1243d8..da6a6b76a040 100644
>> --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
>> +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
>> @@ -656,8 +656,10 @@ void radix__early_init_mmu_secondary(void)
>>  		lpcr = mfspr(SPRN_LPCR);
>>  		mtspr(SPRN_LPCR, lpcr | LPCR_UPRT | LPCR_HR);
>>  
>> -		mtspr(SPRN_PTCR,
>> -		      __pa(partition_tb) | (PATB_SIZE_SHIFT - 12));
>> +		if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
>> +			mtspr(SPRN_PTCR, __pa(partition_tb) |
>> +			      (PATB_SIZE_SHIFT - 12));
>> +
>>  		radix_init_amor();
>>  	}
>>  
>> @@ -673,7 +675,8 @@ void radix__mmu_cleanup_all(void)
>>  	if (!firmware_has_feature(FW_FEATURE_LPAR)) {
>>  		lpcr = mfspr(SPRN_LPCR);
>>  		mtspr(SPRN_LPCR, lpcr & ~LPCR_UPRT);
>> -		mtspr(SPRN_PTCR, 0);
>> +		if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
>> +			mtspr(SPRN_PTCR, 0);
>>  		powernv_set_nmmu_ptcr(0);
>>  		radix__flush_tlb_all();
>>  	}
> There's four of these case where we skip touching the PTCR, which is
> right on the borderline of warranting an accessor. I guess we can do it
> as a cleanup later.

I agree.

Since the kernel doesn't need to access a big number of ultravisor
privileged registers, maybe we can define mtspr_<reg> and mfspr_<reg>
inline functions that in ultravisor.h that skip touching the register if an
ultravisor is present and and the register is ultravisor privileged. Thus,
we don't need to replicate comments and that also would make it easier for
developers to know what are the ultravisor privileged registers.

Something like this:

--- a/arch/powerpc/include/asm/ultravisor.h
+++ b/arch/powerpc/include/asm/ultravisor.h
@@ -10,10 +10,21 @@
 
 #include <asm/ultravisor-api.h>
 #include <asm/asm-prototypes.h>
+#include <asm/reg.h>
 
 int early_init_dt_scan_ultravisor(unsigned long node, const char *uname,
                                  int depth, void *data);
 
+static inline void mtspr_ptcr(unsigned long val)
+{
+       /*
+        * If the ultravisor firmware is present, it maintains the partition
+        * table. PTCR becomes ultravisor privileged only for writing.
+        */
+       if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
+               mtspr(SPRN_PTCR, val);
+}
+
 static inline int uv_register_pate(u64 lpid, u64 dw0, u64 dw1)
 {
        return ucall_norets(UV_WRITE_PATE, lpid, dw0, dw1);
diff --git a/arch/powerpc/mm/book3s64/pgtable.c
b/arch/powerpc/mm/book3s64/pgtable.c
index e1bbc48e730f..25156f9dfde8 100644
--- a/arch/powerpc/mm/book3s64/pgtable.c
+++ b/arch/powerpc/mm/book3s64/pgtable.c
@@ -220,7 +220,7 @@ void __init mmu_partition_table_init(void)
         * 64 K size.
         */
        ptcr = __pa(partition_tb) | (PATB_SIZE_SHIFT - 12);
-       mtspr(SPRN_PTCR, ptcr);
+       mtspr_ptcr(ptcr);
        powernv_set_nmmu_ptcr(ptcr);
 }

What do you think?
An alternative could be to change the mtspr() and mfspr() macros as we
proposed in the v1, but access to non-ultravisor privileged registers would
be performance impacted because we always would need to check if the
register is one of the few ultravisor registers that the kernel needs to
access.

Thanks,
Claudio


> cheers
>