Re: [Qemu-devel] [RFC 02/10] use a different translation block list for each cpu.

[Frederic Konrad <fred.konrad@greensocs.com>]

On 29/01/2015 16:24, Peter Maydell wrote:
> On 16 January 2015 at 17:19,  <fred.konrad@greensocs.com> wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> We need a different TranslationBlock list for each core in case of multithread
>> TCG.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>   translate-all.c | 40 ++++++++++++++++++++++------------------
>>   1 file changed, 22 insertions(+), 18 deletions(-)
>>
>> diff --git a/translate-all.c b/translate-all.c
>> index 8fa4378..0e11c70 100644
>> --- a/translate-all.c
>> +++ b/translate-all.c
>> @@ -72,10 +72,11 @@
>>   #endif
>>
>>   #define SMC_BITMAP_USE_THRESHOLD 10
>> +#define MAX_CPUS 256
>>
>>   typedef struct PageDesc {
>>       /* list of TBs intersecting this ram page */
>> -    TranslationBlock *first_tb;
>> +    TranslationBlock *first_tb[MAX_CPUS];
> Do we really need to know this for every CPU, or just for
> the one that's using this PageDesc? I am assuming we're going to make
> the l1_map be per-CPU.

Do we have any clue of which cpu is using this PageDesc?
We did this like that because it is quite simple.
>
>>       /* in order to optimize self modifying code, we count the number
>>          of lookups we do to a given page to use a bitmap */
>>       unsigned int code_write_count;
>> @@ -750,7 +751,7 @@ static inline void invalidate_page_bitmap(PageDesc *p)
>>   /* Set to NULL all the 'first_tb' fields in all PageDescs. */
>>   static void page_flush_tb_1(int level, void **lp)
>>   {
>> -    int i;
>> +    int i, j;
>>
>>       if (*lp == NULL) {
>>           return;
>> @@ -759,7 +760,9 @@ static void page_flush_tb_1(int level, void **lp)
>>           PageDesc *pd = *lp;
>>
>>           for (i = 0; i < V_L2_SIZE; ++i) {
>> -            pd[i].first_tb = NULL;
>> +            for (j = 0; j < MAX_CPUS; j++) {
>> +                pd[i].first_tb[j] = NULL;
>> +            }
>>               invalidate_page_bitmap(pd + i);
>>           }
>>       } else {
>> @@ -937,12 +940,12 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
>>       /* remove the TB from the page list */
>>       if (tb->page_addr[0] != page_addr) {
>>           p = page_find(tb->page_addr[0] >> TARGET_PAGE_BITS);
>> -        tb_page_remove(&p->first_tb, tb);
>> +        tb_page_remove(&p->first_tb[current_cpu->cpu_index], tb);
> Anything using current_cpu in this code is hugely suspect.
> For instance cpu_restore_state() takes a CPUState pointer and
> calls this function -- either it should be acting on just that
> CPU (which might not be the current one) or on all CPUs. In
> any case implicitly working on current_cpu here is wrong.
>
> Probably we need to look at the public-facing functions here
> and decide which should have "operate on all CPUs" semantics
> and which should have "operate on the CPU passed as a parameter"
> and which "operate on the implicit current CPU".

Ok so the idea would be to have eg a cpu mask parameter to know which cpu to
invalidate/restore etc etc?
Or just pointer and invalidate all if NULL?

Thanks,
Fred
>
> -- PMM