Re: [Qemu-devel] [PATCH v3] implementing victim TLB for QEMU system emulated TLB

[Peter Maydell]

On 2 February 2014 15:15, Xin Tong <address@hidden> wrote:
> Hi Peter
>
> Thank you for your reviews , i have 2 questions.
>
> On Sat, Feb 1, 2014 at 4:14 PM, Peter Maydell <address@hidden> wrote:
>> On 28 January 2014 17:31, Xin Tong <address@hidden> 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