Re: [QUESTION] tcg: Is concurrent storing and code translation of the same code page considered as racing in MTTCG?

[Richard Henderson]

On 2/1/21 6:59 AM, Liren Wei wrote:
> On 2/1/21 7:01 AM, Richard Henderson wrote:
>> Yes, this is a bug, because we are trying to support e.g. x86 which does not
>> require an icache flush.
> 
> That is too bad :(
> 
> I know nothing about the modern hardware, it's really hard to imagine what
> is done in CPU to maintain the coherence when this kind of cross-modifying
> scenario happens.

Indeed.  Complicated cache shootdown stuff, no doubt.  It also helps that the
cpu is decoding one instruction at a time, whereas qemu is decoding many
instructions.

>> I think the page lock, the TLB_NOTDIRTY setting, and a possible sync on the
>> setting, needs to happen before the bytes are read during translation.
>> Otherwise we don't catch the case above, nor do we catch
>>
>>     CPU1                  CPU2
>>     ------------------    --------------------------
>>     TLB check -> fast
>>                           tb_gen_code() -> all of it
>>       write to ram
>>
>> Also because of x86 (and other architectures in which a single instruction 
>> can
>> span a page boundary), I think this lock+set+sync sequence needs to happen on
>> demand in something called from the function set defined in
>> include/exec/translator.h
>>
>> That also means that any target/cpu/ which has not been converted to use that
>> interface remains broken, and should be converted or deprecated.
> 
> I failed to figure out what do you mean by lock+set+sync,

I used "lock+set+sync" as shorthand for the sequence I described in the first
paragraph above.

> in particular:
>   - What is the use of the page lock here (Is this the lock of PageDesc?)

Yes, this would be the PageDesc lock.  It would prevent TLB_NOTDIRTY from being
removed by the slow-path write until any translation is complete.

>   - Is the "possible sync" means some kind of wait so that TLB_NOTDIRTY is
>     definitely able to catch further "write to ram"?

That's what the sync is for -- to make sure that no other cpu can have seen a
dirty page and not be finished with the write.

> Therefore maybe we can mark the RAM backing page in QEMU's page table as
> non-writable at an early stage in tb_gen_code() using the ability of the
> underlying OS...

It's very expensive to change page tables.  It's also complicated to capture
and re-start with signal handlers.  We do that sort of thing for
qemu-linux-user, where there *is* no slow path and we have no other option.

I think it would be much easier to reason with locks.


r~