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.
>> (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.
thanks
-- PMM
