On 8/14/19 8:33 AM, Michael Ellerman wrote:
> Hi Claudio,
>
> Claudio Carvalho <cclaudio@xxxxxxxxxxxxx> writes:
>> From: Michael Anderson <andmike@xxxxxxxxxxxxx>
>>
>> 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
>
