Subject: Re: [PATCH 1/3] Setup AMOR in HV mode

From: Balbir Singh <[email-masked]>
Date: Fri, 11 Nov 2016 17:15:28 +1100

On 09/11/16 02:13, Aneesh Kumar K.V wrote:
> Balbir Singh <[email-masked]> writes:
> 
>> AMOR should be setup in HV mode, we set it up once
>> and let the generic kernel handle IAMR. This patch is
>> used to enable storage keys in a following patch as
>> defined in ISA 3
>>
>> Reported-by: Aneesh Kumar K.V <[email-masked]>
>> Signed-off-by: Balbir Singh <[email-masked]>
>> ---
>>  arch/powerpc/mm/pgtable-radix.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
>> index ed7bddc..0fdd8ed 100644
>> --- a/arch/powerpc/mm/pgtable-radix.c
>> +++ b/arch/powerpc/mm/pgtable-radix.c
>> @@ -320,6 +320,25 @@ static void update_hid_for_radix(void)
>>  		cpu_relax();
>>  }
>>  
>> +/*
>> + * In HV mode, we init AMOR so that the hypervisor
>> + * and guest can setup IMAR, enable key 0 and set
>> + * it to 1
>> + * AMOR = 1100....00 (Mask for key 0 is 11)
>> + */
>> +static void __init radix_init_amor(void)
>> +{
>> +	unsigned long amor_mask = 0xc000000000000000;
>> +	unsigned long amor = mfspr(SPRN_AMOR);
>> +
>> +	if (cpu_has_feature(CPU_FTR_POWER9_DD1))
>> +		return;
> 
> Why is DD1 excluded ?
> 
>> +
>> +	amor |= amor_mask;
>> +
> 
> Can't we just init it with the constant 0xc000000000000000;
> I can understand kvm code wanting to do the '|' above, but does the host
> init code need to look at the previous value there ?
> 
> 
>> +	mtspr(SPRN_AMOR, amor);
>> +}
>> +
>>  void __init radix__early_init_mmu(void)
>>  {
>>  	unsigned long lpcr;
>> @@ -376,6 +395,7 @@ void __init radix__early_init_mmu(void)
>>  		lpcr = mfspr(SPRN_LPCR);
>>  		mtspr(SPRN_LPCR, lpcr | LPCR_UPRT | LPCR_HR);
>>  		radix_init_partition_table();
>> +		radix_init_amor();
>>  	}
>>  
>>  	radix_init_pgtable();
> 
> What about secondaries ? Don't we need to init AMOR there ?

My bad, I should have checked and caught this

Balbir Singh