Re: [Qemu-devel] [RFC v6 11/14] softmmu: Simplify helper_*_st_name, wrap MMIO code

[alvise rigo]

On Mon, Jan 11, 2016 at 10:54 AM, Alex Bennée <address@hidden> wrote:
>
> Alvise Rigo <address@hidden> writes:
>
>> Attempting to simplify the helper_*_st_name, wrap the MMIO code into an
>> inline function.
>>
>> Suggested-by: Jani Kokkonen <address@hidden>
>> Suggested-by: Claudio Fontana <address@hidden>
>> Signed-off-by: Alvise Rigo <address@hidden>
>> ---
>>  softmmu_template.h | 64 
>> +++++++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 44 insertions(+), 20 deletions(-)
>>
>> diff --git a/softmmu_template.h b/softmmu_template.h
>> index 92f92b1..2ebf527 100644
>> --- a/softmmu_template.h
>> +++ b/softmmu_template.h
>> @@ -396,6 +396,26 @@ static inline void glue(helper_le_st_name, 
>> _do_unl_access)(CPUArchState *env,
>>      }
>>  }
>>
>> +static inline void glue(helper_le_st_name, _do_mmio_access)(CPUArchState 
>> *env,
>> +                                                            DATA_TYPE val,
>> +                                                            target_ulong 
>> addr,
>> +                                                            TCGMemOpIdx oi,
>> +                                                            unsigned 
>> mmu_idx,
>> +                                                            int index,
>> +                                                            uintptr_t 
>> retaddr)
>> +{
>> +    CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index];
>> +
>> +    if ((addr & (DATA_SIZE - 1)) != 0) {
>> +        glue(helper_le_st_name, _do_unl_access)(env, val, addr, mmu_idx,
>> +                                                oi, retaddr);
>> +    }
>> +    /* ??? Note that the io helpers always read data in the target
>> +       byte ordering.  We should push the LE/BE request down into io.  */
>> +    val = TGT_LE(val);
>> +    glue(io_write, SUFFIX)(env, iotlbentry, val, addr, retaddr);
>> +}
>> +
>
> Some comment as previous patches. I think we can have a single function
> that is shared between both helpers.

Of course. If the objdump you got from this version and the version
with single helper is basically the same, then there's no reason to
make two distinct variants.

Thank you,
alvise

>
>>  void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
>>                         TCGMemOpIdx oi, uintptr_t retaddr)
>>  {
>> @@ -458,16 +478,8 @@ void helper_le_st_name(CPUArchState *env, target_ulong 
>> addr, DATA_TYPE val,
>>
>>              return;
>>          } else {
>> -            if ((addr & (DATA_SIZE - 1)) != 0) {
>> -                glue(helper_le_st_name, _do_unl_access)(env, val, addr, 
>> mmu_idx,
>> -                                                        oi, retaddr);
>> -            }
>> -            iotlbentry = &env->iotlb[mmu_idx][index];
>> -
>> -            /* ??? Note that the io helpers always read data in the target
>> -               byte ordering.  We should push the LE/BE request down into 
>> io.  */
>> -            val = TGT_LE(val);
>> -            glue(io_write, SUFFIX)(env, iotlbentry, val, addr, retaddr);
>> +            glue(helper_le_st_name, _do_mmio_access)(env, val, addr, oi,
>> +                                                     mmu_idx, index, 
>> retaddr);
>>              return;
>>          }
>>      }
>> @@ -523,6 +535,26 @@ static inline void glue(helper_be_st_name, 
>> _do_unl_access)(CPUArchState *env,
>>      }
>>  }
>>
>> +static inline void glue(helper_be_st_name, _do_mmio_access)(CPUArchState 
>> *env,
>> +                                                            DATA_TYPE val,
>> +                                                            target_ulong 
>> addr,
>> +                                                            TCGMemOpIdx oi,
>> +                                                            unsigned 
>> mmu_idx,
>> +                                                            int index,
>> +                                                            uintptr_t 
>> retaddr)
>> +{
>> +    CPUIOTLBEntry *iotlbentry = &env->iotlb[mmu_idx][index];
>> +
>> +    if ((addr & (DATA_SIZE - 1)) != 0) {
>> +        glue(helper_be_st_name, _do_unl_access)(env, val, addr, mmu_idx,
>> +                                                oi, retaddr);
>> +    }
>> +    /* ??? Note that the io helpers always read data in the target
>> +       byte ordering.  We should push the LE/BE request down into io.  */
>> +    val = TGT_BE(val);
>> +    glue(io_write, SUFFIX)(env, iotlbentry, val, addr, retaddr);
>> +}
>> +
>>  void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
>>                         TCGMemOpIdx oi, uintptr_t retaddr)
>>  {
>> @@ -585,16 +617,8 @@ void helper_be_st_name(CPUArchState *env, target_ulong 
>> addr, DATA_TYPE val,
>>
>>              return;
>>          } else {
>> -            if ((addr & (DATA_SIZE - 1)) != 0) {
>> -                glue(helper_be_st_name, _do_unl_access)(env, val, addr, 
>> mmu_idx,
>> -                                                        oi, retaddr);
>> -            }
>> -            iotlbentry = &env->iotlb[mmu_idx][index];
>> -
>> -            /* ??? Note that the io helpers always read data in the target
>> -               byte ordering.  We should push the LE/BE request down into 
>> io.  */
>> -            val = TGT_BE(val);
>> -            glue(io_write, SUFFIX)(env, iotlbentry, val, addr, retaddr);
>> +            glue(helper_be_st_name, _do_mmio_access)(env, val, addr, oi,
>> +                                                     mmu_idx, index, 
>> retaddr);
>>              return;
>>          }
>>      }
>
>
> --
> Alex Bennée