Re: [Qemu-devel] [PATCH 2/7] target-mips: support Page Frame Number Extension field

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 4239 bytes --]

On 28/04/15 14:35, James Hogan wrote:
> 
> 
> On 28/04/15 13:41, Leon Alrae wrote:
>> Update tlb->PFN to contain PFN concatenated with PFNX. PFNX is 0 if large
>> physical address is not supported.
>>
>> Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
>> ---
>>  target-mips/op_helper.c | 32 ++++++++++++++++++++++++++------
>>  1 file changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
>> index c9a60bd..6bff927 100644
>> --- a/target-mips/op_helper.c
>> +++ b/target-mips/op_helper.c
>> @@ -1825,6 +1825,16 @@ static void r4k_mips_tlb_flush_extra (CPUMIPSState *env, int first)
>>      }
>>  }
>>  
>> +static inline uint64_t get_tlb_pfn_from_entrylo(uint64_t entrylo)
>> +{
>> +#if defined(TARGET_MIPS64)
>> +    return extract64(entrylo, 6, 54);
>> +#else
>> +    return extract64(entrylo, 6, 24) | /* PFN */
>> +           (extract64(entrylo, 32, 32) << 24); /* PFNX */
> 
> Where does the 32,32 come from? The PRA I have seems to imply that PFNX
> starts at bit 30 and goes up to bit 54.

Ah right, I guess this is from the output of MFHC0/MTHC0 instructions.
The specification of those instructions specify the mangling they do:

GPR[rt]_31:0 <- data_61..30
GPR[rt]_63..32 <- (data_61)^31 // sign-extend

Note how the field it extracts starts at bit 30. IMO the stored values
of EntryLo PFN should be continuous rather than split up like the
MFHC0/MTHC0 versions.

Cheers
James

> 
> That would of course also mean that the code for mfc0 EntryLo* needs
> tweaking so that PFNX doesn't cause XI/RI bits to be set at bits 30,31
> (haven't looked at other patches yet).
> 
>> +#endif
>> +}
>> +
>>  static void r4k_fill_tlb(CPUMIPSState *env, int idx)
>>  {
>>      r4k_tlb_t *tlb;
>> @@ -1848,13 +1858,13 @@ static void r4k_fill_tlb(CPUMIPSState *env, int idx)
>>      tlb->C0 = (env->CP0_EntryLo0 >> 3) & 0x7;
>>      tlb->XI0 = (env->CP0_EntryLo0 >> CP0EnLo_XI) & 1;
>>      tlb->RI0 = (env->CP0_EntryLo0 >> CP0EnLo_RI) & 1;
>> -    tlb->PFN[0] = (env->CP0_EntryLo0 >> 6) << 12;
>> +    tlb->PFN[0] = get_tlb_pfn_from_entrylo(env->CP0_EntryLo0) << 12;
>>      tlb->V1 = (env->CP0_EntryLo1 & 2) != 0;
>>      tlb->D1 = (env->CP0_EntryLo1 & 4) != 0;
>>      tlb->C1 = (env->CP0_EntryLo1 >> 3) & 0x7;
>>      tlb->XI1 = (env->CP0_EntryLo1 >> CP0EnLo_XI) & 1;
>>      tlb->RI1 = (env->CP0_EntryLo1 >> CP0EnLo_RI) & 1;
>> -    tlb->PFN[1] = (env->CP0_EntryLo1 >> 6) << 12;
>> +    tlb->PFN[1] = get_tlb_pfn_from_entrylo(env->CP0_EntryLo1) << 12;
>>  }
>>  
>>  void r4k_helper_tlbinv(CPUMIPSState *env)
>> @@ -1971,6 +1981,16 @@ void r4k_helper_tlbp(CPUMIPSState *env)
>>      }
>>  }
>>  
>> +static inline uint64_t get_entrylo_pfn_from_tlb(uint64_t tlb_pfn)
>> +{
>> +#if defined(TARGET_MIPS64)
>> +    return tlb_pfn << 6;
>> +#else
>> +    return (extract64(tlb_pfn, 0, 24) << 6) | /* PFN */
>> +           (extract64(tlb_pfn, 24, 32) << 32); /* PFNX */
> 
> same again. shouldn't it be 25 bits starting at bit 24, shifted to start
> at bit 30?
> 
> Cheers
> James
> 
>> +#endif
>> +}
>> +
>>  void r4k_helper_tlbr(CPUMIPSState *env)
>>  {
>>      r4k_tlb_t *tlb;
>> @@ -1997,12 +2017,12 @@ void r4k_helper_tlbr(CPUMIPSState *env)
>>          env->CP0_PageMask = tlb->PageMask;
>>          env->CP0_EntryLo0 = tlb->G | (tlb->V0 << 1) | (tlb->D0 << 2) |
>>                          ((uint64_t)tlb->RI0 << CP0EnLo_RI) |
>> -                        ((uint64_t)tlb->XI0 << CP0EnLo_XI) |
>> -                        (tlb->C0 << 3) | (tlb->PFN[0] >> 6);
>> +                        ((uint64_t)tlb->XI0 << CP0EnLo_XI) | (tlb->C0 << 3) |
>> +                        get_entrylo_pfn_from_tlb(tlb->PFN[0] >> 12);
>>          env->CP0_EntryLo1 = tlb->G | (tlb->V1 << 1) | (tlb->D1 << 2) |
>>                          ((uint64_t)tlb->RI1 << CP0EnLo_RI) |
>> -                        ((uint64_t)tlb->XI1 << CP0EnLo_XI) |
>> -                        (tlb->C1 << 3) | (tlb->PFN[1] >> 6);
>> +                        ((uint64_t)tlb->XI1 << CP0EnLo_XI) | (tlb->C1 << 3) |
>> +                        get_entrylo_pfn_from_tlb(tlb->PFN[1] >> 12);
>>      }
>>  }
>>  
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]