On Sun, Feb 2, 2014 at 10:19 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 2 February 2014 15:15, Xin Tong <trent.t...@gmail.com> wrote:
>> Hi Peter
>>
>> Thank you for your reviews , i have 2 questions.
>>
>> On Sat, Feb 1, 2014 at 4:14 PM, Peter Maydell <peter.mayd...@linaro.org>
>> wrote:
>>> On 28 January 2014 17:31, Xin Tong <trent.t...@gmail.com> wrote:
>>>> +/* macro to check the victim tlb */
>>>> +#define HELPER_CHECK_VICTIM_TLB(ACCESS_TYPE) \
>>>> +do { \
>>>> + for (vtlb_idx = CPU_VTLB_SIZE; vtlb_idx >= 0; --vtlb_idx) { \
>>>> + if (ACCESS_TYPE == (addr & TARGET_PAGE_MASK)) { \
>>>> + /* found entry in victim tlb */ \
>>>> + swap_tlb(&env->tlb_table[mmu_idx][index], \
>>>> + &env->tlb_v_table[mmu_idx][vtlb_idx], \
>>>> + &env->iotlb[mmu_idx][index], \
>>>> + &env->iotlb_v[mmu_idx][vtlb_idx]); \
>>>
>>> This is the only place swap_tlb gets used, so probably better to
>>> ditch that as a separate function and just inline it here. (Then
>>> everywhere that uses softmmu_template.h gets the inlined
>>> version, rather than cputlb.c getting to inline and the rest not.)
>>>
>>>> + break; \
>>>> + } \
>>>> + } \
>>>> +} while (0);
>>>
>>> I think we could make this macro use a bit less ugly.
>>> (1) just call it VICTIM_TLB_HIT; 'helper' has a particular
>>> meaning in QEMU (function called from generated code), and
>>> besides it makes the calling code read a bit less naturally.
>>> (2) rather than having it take as an argument
>>> "env->tlb_v_table[mmu_idx][vtlb_idx].ADDR_READ", just
>>> take the "ADDR_READ" part. This makes the callers simpler
>>> and avoids giving the impression that the macro is going to
>>> simply evaluate its argument once (ie that it is function-like).
>>
>> i probably should use tlb_addr defined on the very top of the
>> helper_le_ld_name macro ?
>
> Not sure what you mean here. tlb_addr is a variable.
>
There is a
>>> (3) Make it a gcc statement-expression, so it can return a
>>> value. Then you can (a) make the scope of vtlb_idx be only
>>> inside teh macro, and avoid forcing the caller to define it and
>>> (b) have the usage pattern be
>>> if (!VICTIM_TLB_HIT(ADDR_READ)) {
>>> tlb_fill(env, addr, READ_ACCESS_TYPE, mmu_idx, retaddr);
>>> }
>>
>> would GCC statement expression be supported by other compilers ? i do
>> not want to make the code less portable. I do not see any use of
>> statement expression in the code ive read from QEMU code base.
>
> No, we use it already. You can see a bunch of uses if you do
> git grep '({'
> (as well as some false positive matches, of course).
>
> Statement expressions are supported by both gcc and clang,
> which is the set of compilers we care about.
Ok got it. would moving vtlb_idx inside the macro break the C89 rule
of "No Variable declaration in middle of block"
>
> thanks
> -- PMM
