Re: [PATCH] util/log: flush TB cache when log level changes

["Alex Bennée" <alex.bennee@linaro.org>]

Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> writes:

> On 25.01.2021 14:09, Alex Bennée wrote:
>> 
>> Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> writes:
>> 
>>> On 22.01.2021 14:42, Alex Bennée wrote:
>>>>
>>>> Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> writes:
<snip>
>>>>>    #endif
>>>>> +    tb_flush_all();
>>>>> +
>>>>
>>>> I would call tb_flush(current_cpu) or first_cpu here. But two things:
>>>>
>>>>    - I'm not sure you have a CPU at all times qemu_set_log is called
>>>>    - It seems overly aggressive to throw away all translations every time
>>>>      the log level is changed. I would define a mask in log.h and have
>>>>      something like:
>>>
>>> Do you propose removing the parameter from tb_flush or omitting the loop
>>> from tb_flush_all?
>> 
>> No tb_flush should keep the CPU interface. In normal usage from the
>> emulation we always have a CPU to call. However for qemu_set_log you
>> will need to find a CPU to call or bail out if you can't. Maybe
>
> It the following true? We can't get rid of CPU in tb_flush, because 
> do_tb_flush must be executed in vCPU thread.
> Can one CPU break others execution in case of SMP? Can we move flush to 
> BH somehow?

The flush has to execute in the CPU context because it uses the
exclusive mechanism to ensure all other CPUs are currently quiescent
before doing the flush. Currently the exclusive mechanism is only aware
of CPU threads and extending it would be essentially re-introducing a
BQL.

>
>> something like:
>> 
>>    CPUStatus *cpu = current_cpu || first_cpu;
>>    if (cpu) {
>>        tb_flush(cpu);
>>    }
>> 
>
> Then we'll have to expose all this CPU stuff to utils and add stubs for 
> them.
>
>> my only worry is if qemu_set_log is called from outside a CPU context
>> (current_cpu will always be NULL) while first_cpu is in a exclusive
>> region. We could extend cpu_in_exclusive_context to be:
>> 
>>    cpu == current_cpu && cpu->in_exclusive_context
>> 
>> but that seems a little icky to me. Paolo, any thoughts?
>> 
>>>
>>>>     if (log_flags & LOG_TRANSLATION) {
>>>>         tb_flush();
>>>>     }
>>>>
>>>>>        /*
>>>>>         * In all cases we only log if qemu_loglevel is set.
>>>>>         * Also:
>>>>
>>>>
>> 
>> 


-- 
Alex Bennée