qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Xin Tong
Subject: Re: [Qemu-devel] [PATCH v3] implementing victim TLB for QEMU system emulated TLB
Date: Sun, 2 Feb 2014 12:27:24 -0600

On Sun, Feb 2, 2014 at 10:19 AM, Peter Maydell <address@hidden> wrote:
> 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.
>
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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]