[RFC,1/5] cputlb: cleanup get_page_addr_code to use VICTIM_TLB_HIT

From: KONRAD Frederic <fred.konrad@greensocs.com>

This replaces env1 and page_index variables by env and index
so we can use VICTIM_TLB_HIT macro later.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 cputlb.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

On Fri, Feb 03, 2017 at 06:06:33PM +0100, fred.konrad@greensocs.com wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
> 
> This replaces env1 and page_index variables by env and index
> so we can use VICTIM_TLB_HIT macro later.
>

Hi Fred,

A question, wouldn't it be more readable to add env and index arguments to VICTIM_TLB_HIT?
VICTIM_TLB_HIT could perhaps even be made a static inline?

Cheers,
Edgar


 
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  cputlb.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index 6c39927..665caea 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -457,21 +457,21 @@ static void report_bad_exec(CPUState *cpu, target_ulong addr)
>   * is actually a ram_addr_t (in system mode; the user mode emulation
>   * version of this function returns a guest virtual address).
>   */
> -tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
> +tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
>  {
> -    int mmu_idx, page_index, pd;
> +    int mmu_idx, index, pd;
>      void *p;
>      MemoryRegion *mr;
> -    CPUState *cpu = ENV_GET_CPU(env1);
> +    CPUState *cpu = ENV_GET_CPU(env);
>      CPUIOTLBEntry *iotlbentry;
>  
> -    page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> -    mmu_idx = cpu_mmu_index(env1, true);
> -    if (unlikely(env1->tlb_table[mmu_idx][page_index].addr_code !=
> +    index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> +    mmu_idx = cpu_mmu_index(env, true);
> +    if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
>                   (addr & TARGET_PAGE_MASK))) {
> -        cpu_ldub_code(env1, addr);
> +        cpu_ldub_code(env, addr);
>      }
> -    iotlbentry = &env1->iotlb[mmu_idx][page_index];
> +    iotlbentry = &env->iotlb[mmu_idx][index];
>      pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
>      mr = iotlb_to_region(cpu, pd, iotlbentry->attrs);
>      if (memory_region_is_unassigned(mr)) {
> @@ -484,7 +484,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
>              exit(1);
>          }
>      }
> -    p = (void *)((uintptr_t)addr + env1->tlb_table[mmu_idx][page_index].addend);
> +    p = (void *)((uintptr_t)addr + env->tlb_table[mmu_idx][index].addend);
>      return qemu_ram_addr_from_host_nofail(p);
>  }
>  
> -- 
> 1.8.3.1
>

On 02/04/2017 12:30 PM, Edgar E. Iglesias wrote:
> On Fri, Feb 03, 2017 at 06:06:33PM +0100, fred.konrad@greensocs.com wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> This replaces env1 and page_index variables by env and index
>> so we can use VICTIM_TLB_HIT macro later.
>>
> 
> Hi Fred,
> 
> A question, wouldn't it be more readable to add env and index arguments to VICTIM_TLB_HIT?
> VICTIM_TLB_HIT could perhaps even be made a static inline?
> 
> Cheers,
> Edgar

Hi Edgar,

Well it seems the VICTIM_TLB_HIT macro is just here to hide those
variables actually.

in cputlb.c:
static bool victim_tlb_hit(CPUArchState *env, size_t mmu_idx,
                           size_t index, size_t elt_ofs,
                           target_ulong page)

and then:
/* Macro to call the above, with local variables from the use context.  */
#define VICTIM_TLB_HIT(TY, ADDR) \
  victim_tlb_hit(env, mmu_idx, index, offsetof(CPUTLBEntry, TY), \
                 (ADDR) & TARGET_PAGE_MASK)

My point of view is clearly that it makes more difficult to read.
So if everybody agrees I can drop the macro and call directly
victim_tlb_hit.

Fred


> 
> 
>  
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>  cputlb.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/cputlb.c b/cputlb.c
>> index 6c39927..665caea 100644
>> --- a/cputlb.c
>> +++ b/cputlb.c
>> @@ -457,21 +457,21 @@ static void report_bad_exec(CPUState *cpu, target_ulong addr)
>>   * is actually a ram_addr_t (in system mode; the user mode emulation
>>   * version of this function returns a guest virtual address).
>>   */
>> -tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
>> +tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
>>  {
>> -    int mmu_idx, page_index, pd;
>> +    int mmu_idx, index, pd;
>>      void *p;
>>      MemoryRegion *mr;
>> -    CPUState *cpu = ENV_GET_CPU(env1);
>> +    CPUState *cpu = ENV_GET_CPU(env);
>>      CPUIOTLBEntry *iotlbentry;
>>  
>> -    page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>> -    mmu_idx = cpu_mmu_index(env1, true);
>> -    if (unlikely(env1->tlb_table[mmu_idx][page_index].addr_code !=
>> +    index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>> +    mmu_idx = cpu_mmu_index(env, true);
>> +    if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
>>                   (addr & TARGET_PAGE_MASK))) {
>> -        cpu_ldub_code(env1, addr);
>> +        cpu_ldub_code(env, addr);
>>      }
>> -    iotlbentry = &env1->iotlb[mmu_idx][page_index];
>> +    iotlbentry = &env->iotlb[mmu_idx][index];
>>      pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
>>      mr = iotlb_to_region(cpu, pd, iotlbentry->attrs);
>>      if (memory_region_is_unassigned(mr)) {
>> @@ -484,7 +484,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
>>              exit(1);
>>          }
>>      }
>> -    p = (void *)((uintptr_t)addr + env1->tlb_table[mmu_idx][page_index].addend);
>> +    p = (void *)((uintptr_t)addr + env->tlb_table[mmu_idx][index].addend);
>>      return qemu_ram_addr_from_host_nofail(p);
>>  }
>>  
>> -- 
>> 1.8.3.1
>>