Re: [PATCH] KVM: PPC: e6500: support powers of 2K TLB1 sizes

[Laurentiu Tudor <b10716@freescale.com>]

On 09/24/2015 11:23 PM, Scott Wood wrote:
> On Thu, 2015-09-24 at 15:57 +0300, Laurentiu Tudor wrote:
>> Book-E MMUv2 present in e6500 cores supports
>> powers of 2K page sizes while older MMUv1 cores
>> support only powers of 4K page sizes, or in other
>> words the LSB of TSIZE on MMUv1 is always 0.
>> Thus, on MMUv2 we must not strip the LSB.
> 
> We can get better TLB utilization by not stripping it, but why "must not" 
> which makes it sound like a bugfix rather than an optimization?

Not sure i get it. If a guests wants a 2K^^(2n+1) translation
size it will instead get a 2K^^2n size, no? Isn't this an issue?

>> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>>     [Laurentiu.Tudor@freescale.com: addressed review
>>          feedback, split in distinct patch]
>> Signed-off-by: Laurentiu Tudor <Laurentiu.Tudor@freescale.com>
>> ---
>>  arch/powerpc/kvm/e500_mmu_host.c | 28 +++++++++++++++++++++-------
>>  1 file changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/e500_mmu_host.c 
>> b/arch/powerpc/kvm/e500_mmu_host.c
>> index 4d33e19..12d5c67 100644
>> --- a/arch/powerpc/kvm/e500_mmu_host.c
>> +++ b/arch/powerpc/kvm/e500_mmu_host.c
>> @@ -371,6 +371,7 @@ static inline int kvmppc_e500_shadow_map(struct 
>> kvmppc_vcpu_e500 *vcpu_e500,
>>  
>>                       unsigned long start, end;
>>                       unsigned long slot_start, slot_end;
>> +                     int tsize_inc;
>>  
>>                       pfnmap = 1;
>>  
>> @@ -392,10 +393,20 @@ static inline int kvmppc_e500_shadow_map(struct 
>> kvmppc_vcpu_e500 *vcpu_e500,
>>                               MAS1_TSIZE_SHIFT;
>>  
>>                       /*
>> -                      * e500 doesn't implement the lowest tsize bit,
>> -                      * or 1K pages.
>> +                      * MMUv1 doesn't implement the lowest tsize bit,
>> +                      * or translations smaller than 4K.
>>                        */
>> -                     tsize = max(BOOK3E_PAGESZ_4K, tsize & ~1);
>> +                     if (!has_feature(&vcpu_e500->vcpu, VCPU_FTR_MMU_V2))
>> +                             tsize &= ~1;
>> +                     tsize = max(BOOK3E_PAGESZ_4K, tsize);
>> +
>> +                     /*
>> +                      * Calculate TSIZE increment. MMUv2 supports
>> +                      * power of 2K translations while MMUv1 is limited
>> +                      * to power of 4K sizes.
>> +                      */
>> +                     tsize_inc = has_feature(&vcpu_e500->vcpu,
>> +                                             VCPU_FTR_MMU_V2) ? 1 : 2;
> 
> If you calculate tsize_inc first, then the previous if-statement can become 
> "tsize &= ~(tsize_inc - 1);".

Thanks for the tip. Will do this in the next version.

>>  
>>                       /*
>>                        * Now find the largest tsize (up to what the guest
>> @@ -404,7 +415,8 @@ static inline int kvmppc_e500_shadow_map(struct 
>> kvmppc_vcpu_e500 *vcpu_e500,
>>                        * aligned.
>>                        */
>>  
>> -                     for (; tsize > BOOK3E_PAGESZ_4K; tsize -= 2) {
>> +                     for (; tsize > BOOK3E_PAGESZ_4K;
>> +                          tsize -= tsize_inc) {
>>                               unsigned long gfn_start, gfn_end;
>>                               tsize_pages = 1 << (tsize - 2);
>>  
>> @@ -437,10 +449,12 @@ static inline int kvmppc_e500_shadow_map(struct 
>> kvmppc_vcpu_e500 *vcpu_e500,
>>                       tsize = min(__ilog2(psize) - 10, tsize);
>>  
>>                       /*
>> -                      * e500 doesn't implement the lowest tsize bit,
>> -                      * or 1K pages.
>> +                      * MMUv1 doesn't implement the lowest tsize bit,
>> +                      * or translations smaller than 4K.
>>                        */
> 
> This comment makes it sound like MMUv2 might support translations smaller 
> than 4K.

Right. I'll rephrase in the next spin.

Thanks for the review!

---
Best Regards, Laurentiu