Re: [PATCH v5 4/7] powerpc/mm: Use UV_WRITE_PATE ucall to register a PATE

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

On 8/14/19 8:33 AM, Michael Ellerman wrote:
> Hi Claudio,
>
> Claudio Carvalho <cclaudio@linux.ibm.com> writes:
>> From: Michael Anderson <andmike@linux.ibm.com>
>>
>> In ultravisor enabled systems, the ultravisor creates and maintains the
>> partition table in secure memory where the hypervisor cannot access, and
>                                    ^
>                                    which?
>
>> therefore, the hypervisor have to do the UV_WRITE_PATE ucall whenever it
>                             ^          ^
>                             has        a
>> wants to set a partition table entry (PATE).
>>
>> This patch adds the UV_WRITE_PATE ucall and uses it to set a PATE if
>> ultravisor is enabled. Additionally, this also also keeps a copy of the
>> partition table because the nestMMU does not have access to secure
>> memory. Such copy has entries for nonsecure and hypervisor partition.
> I'm having trouble parsing the last sentence there.
>
> Or at least it doesn't seem to match the code, or I don't understand
> either the code or the comment. More below.
>
>> diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
>> index 85bc81abd286..033731f5dbaa 100644
>> --- a/arch/powerpc/mm/book3s64/pgtable.c
>> +++ b/arch/powerpc/mm/book3s64/pgtable.c
>> @@ -213,34 +223,50 @@ void __init mmu_partition_table_init(void)
>>  	powernv_set_nmmu_ptcr(ptcr);
>>  }
>>  
>> -void mmu_partition_table_set_entry(unsigned int lpid, unsigned long dw0,
>> -				   unsigned long dw1)
>> +/*
>> + * Global flush of TLBs and partition table caches for this lpid. The type of
>> + * flush (hash or radix) depends on what the previous use of this partition ID
>> + * was, not the new use.
>> + */
>> +static void flush_partition(unsigned int lpid, unsigned long old_patb0)
> A nicer API would be for the 2nd param to be a "bool radix", and have
> the caller worry about the fact that it comes from (patb0 & PATB_HR).

Yes, I agree. I applied that to next patchset version.


>
>>  {
>> -	unsigned long old = be64_to_cpu(partition_tb[lpid].patb0);
>> -
>> -	partition_tb[lpid].patb0 = cpu_to_be64(dw0);
>> -	partition_tb[lpid].patb1 = cpu_to_be64(dw1);
>> -
>> -	/*
>> -	 * Global flush of TLBs and partition table caches for this lpid.
>> -	 * The type of flush (hash or radix) depends on what the previous
>> -	 * use of this partition ID was, not the new use.
>> -	 */
>>  	asm volatile("ptesync" : : : "memory");
>> -	if (old & PATB_HR) {
>> -		asm volatile(PPC_TLBIE_5(%0,%1,2,0,1) : :
>> +	if (old_patb0 & PATB_HR) {
>> +		asm volatile(PPC_TLBIE_5(%0, %1, 2, 0, 1) : :
>>  			     "r" (TLBIEL_INVAL_SET_LPID), "r" (lpid));
>> -		asm volatile(PPC_TLBIE_5(%0,%1,2,1,1) : :
>> +		asm volatile(PPC_TLBIE_5(%0, %1, 2, 1, 1) : :
> That looks like an unrelated whitespace change.
>
>>  			     "r" (TLBIEL_INVAL_SET_LPID), "r" (lpid));
>>  		trace_tlbie(lpid, 0, TLBIEL_INVAL_SET_LPID, lpid, 2, 0, 1);
>>  	} else {
>> -		asm volatile(PPC_TLBIE_5(%0,%1,2,0,0) : :
>> +		asm volatile(PPC_TLBIE_5(%0, %1, 2, 0, 0) : :
> Ditto.

True. I dropped the two changes above in the next patchset version.

Thanks,
Claudio


>
>>  			     "r" (TLBIEL_INVAL_SET_LPID), "r" (lpid));
>>  		trace_tlbie(lpid, 0, TLBIEL_INVAL_SET_LPID, lpid, 2, 0, 0);
>>  	}
>>  	/* do we need fixup here ?*/
>>  	asm volatile("eieio; tlbsync; ptesync" : : : "memory");
>>  }
>> +
>> +void mmu_partition_table_set_entry(unsigned int lpid, unsigned long dw0,
>> +				  unsigned long dw1)
>> +{
>> +	unsigned long old = be64_to_cpu(partition_tb[lpid].patb0);
>> +
>> +	partition_tb[lpid].patb0 = cpu_to_be64(dw0);
>> +	partition_tb[lpid].patb1 = cpu_to_be64(dw1);
> ie. here we always update the copy of the partition table, regardless of
> whether we're running under an ultravisor or not. So the copy is a
> complete copy isn't it?
>
>> +	/*
>> +	 * In ultravisor enabled systems, the ultravisor maintains the partition
>> +	 * table in secure memory where we don't have access, therefore, we have
>> +	 * to do a ucall to set an entry.
>> +	 */
>> +	if (firmware_has_feature(FW_FEATURE_ULTRAVISOR)) {
>> +		uv_register_pate(lpid, dw0, dw1);
>> +		pr_info("PATE registered by ultravisor: dw0 = 0x%lx, dw1 = 0x%lx\n",
>> +			dw0, dw1);
>> +	} else {
>> +		flush_partition(lpid, old);
>> +	}
> What is different is whether we flush or not.
>
> And don't we still need to do the flush for the nestMMU? I assume we're
> saying the ultravisor will broadcast a flush for us, which will also
> handle the nestMMU case?
>
> cheers
>