On 10.01.2011, at 15:15, Aurelien Jarno wrote: > On Mon, Jan 10, 2011 at 03:07:52PM +0100, Alexander Graf wrote: >> >> On 10.01.2011, at 15:00, Aurelien Jarno wrote: >> >>> On Mon, Jan 10, 2011 at 01:13:25PM +0100, Alexander Graf wrote: >>>> >>>> On 06.01.2011, at 23:12, Aurelien Jarno wrote: >>>> >>>>> Hi, >>>>> >>>>> I have just sent a tcg/arm patch concerning code retranslation. You >>>>> might want to look at the description (copied below), as from a first >>>>> glance ppc, s390 and sparc TCG targets might be affected. If you see >>>>> guest kernel panics, some segmentation fault of qemu or in the guest, >>>>> strange behaviors, that happen randomly and that looks difficult to >>>>> debug it might be the issue. >>>>> >>>>> Aurelien >>>>> >>>>> >>>>> | QEMU uses code retranslation to restore the CPU state when an exception >>>>> | happens. For it to work the retranslation must not modify the generated >>>>> | code. This is what is currently implemented in ARM TCG. >>>>> | >>>>> | However on CPU that don't have icache/dcache/memory synchronised like >>>>> | ARM, this requirement is stronger and code retranslation must not modify >>>>> | the generated code "atomically", as the cache line might be flushed >>>>> | at any moment (interrupt, exception, task switching), even if not >>>>> | triggered by QEMU. The probability for this to happen is very low, and >>>>> | depends on cache size and associativiy, machine load, interrupts, so the >>>>> | symptoms are might happen randomly. >>>>> | >>>>> | This requirement is currently not followed in tcg/arm, for the >>>>> | load/store code, which basically has the following structure: >>>>> | 1) tlb access code is written >>>>> | 2) conditional fast path code is written >>>>> | 3) branch is written with a temporary target >>>>> | 4) slow path code is written >>>>> | 5) branch target is updated >>>>> | The cache lines corresponding to the retranslated code is not flushed >>>>> | after code retranslation as the generated code is supposed to be the >>>>> | same. However if the cache line corresponding to the branch instruction >>>>> | is flushed between step 3 and 5, and is not flushed again before the >>>>> | code is executed again, the branch target is wrong. In the guest, the >>>>> | symptoms are MMU page fault at a random addresses, which leads to >>>>> | kernel page fault or segmentation faults. >>>> >>>> I don't see the problem. If you have separate icache from dcache, the code >>>> in question doesn't get executed during the rewrite, so all should be >>>> fine. If you have both combined, the data write should automatically >>>> modify the cache line, so all is great too. >>> >>> As far as I understand, this is what happens to the branch target >>> instruction, or at least one possibility: >>> >>> operation | icache | dcache | mem/L2 | >>> ------------------------------------------+--------+--------+--------+ >>> tlb access code is written | absent | absent | ok | >>> conditional fast path code is written | absent | absent | ok | >>> branch is written with a temporary target | absent | wrong | ok | >>> cache line is flushed to memory | absent | absent | wrong | >>> slow path code is written | absent | absent | wrong | >>> branch target is updated | absent | ok | wrong | >>> TB is re-executed | wrong | ok | wrong | >>> >>> Note that the issue is real, the patch really fixes the issue on ARM and >>> MIPS. It's not impossible that I don't fully understand it given I can't >>> analyse the cache values at a given time. However, when QEMU crashes >>> because of that, we have seen that the executed code doesn't match what >>> GDB says, and changing the temporary branch value also changes the way >>> QEMU or the guest crash. >>> >>> The problem doesn't happens very often though (for sure less than 1 >>> every few millions). The other way to fix it is to issue a cache flush >>> after a code retranslation, however, it is something very costly on some >>> architectures. >> >> I'd guess it only happens when code is overwritten. Only then icache can >> still be stale in that code region. Can't we just invalidate the icache on >> every tb flush? That should also fix the issue I'd guess. > > That's a solution that works (tested). However making sure that the > retranslation doesn't change the code looks better.
Yeah, I agree. Temporary retranslation really shouldn't modify existing code. We really do need an icache flush on tb flushes still though as that's a separate issue? Or are these already in? Alex
