Re: [Qemu-devel] [Qemu-ppc] qemu-system-ppc video artifacts since "tcg: drop global lock during TCG code execution"

From: "Alex Bennée" <alex.bennee@linaro.org>
To: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: BALATON Zoltan <balaton@eik.bme.hu>,
	jan.kiszka@siemens.com, qemu-devel <qemu-devel@nongnu.org>,
	cota@braap.org, "qemu-ppc@nongnu.org" <qemu-ppc@nongnu.org>,
	bobby.prani@gmail.com, rth@twiddle.net,
	fred.konrad@greensocs.com
Subject: Re: [Qemu-devel] [Qemu-ppc] qemu-system-ppc video artifacts since "tcg: drop global lock during TCG code execution"
Date: Wed, 15 Mar 2017 14:19:20 +0000	[thread overview]
Message-ID: <87d1dipqqf.fsf@linaro.org> (raw)
In-Reply-To: <f1bba13a-0be4-b97e-c2fd-181e81e738cf@ilande.co.uk>

Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:

> On 15/03/17 11:14, Alex Bennée wrote:
>
>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>
>>> On Tue, 14 Mar 2017, Alex Bennée wrote:
>>>> So from a single-threaded -smp guest case there should be no difference
>>>> in behaviour.
>> <snip>
>>>> However this shouldn't affect
>>>> anything in the single-threaded world.
>>>
>>> I think we have a single CPU and thread for these ppc machines here so
>>> I'm not sure how this could be relevant.
>>>
>>>> However delaying tlb_flushes() could certainly expose/hide stuff that is
>>>> accessing the dirty mechanism. tlb_flush itself now takes the tb_lock() to
>>>> avoid racing with the TB invalidation logic. The act of the flush will
>>>> certainly wipe all existing SoftMMU entries and force a re-load on each
>>>> memory access.
>>>>
>>>> So is the dirty status of memory being read from outside a vCPU
>>>> execution context?
>>>
>>> Like from the display controller models that use
>>> memory_region_get_dirty() to check if the frambuffer needs to be
>>> updated? But all display adaptors seem to do this and the problem was
>>> only seem on ppc so it may be related to something ppc specific.
>>
>> So this accesses the memory_region API which is under RCU control.
>> AFAIUI this should mean the dirty status may be read late (e.g. next
>> update) but should never be incorrect (e.g. miss a dirtying operation).
>
> AFAICT check_tlb_flush() gets passed a global parameter which if set
> true invalidates the TLB across all CPU TLBs rather than just the local
> CPU TLB - but then in this case we're only running with a single CPU so
> I can't see how this is relevant.

Not quite. tlb_flush used to take a global flag but it ignored it. It
has been removed in recent updates to the cputlb API ;-)

> Have you been able to reproduce the artifacts locally at all? I'm
> wondering if once the icount fixup patches are in, it might be easier to
> debug if enabling icount causes the artifacts to appear in a
> deterministic manner.

I have, and I can make them go away as well by forcing a full update.
See my other longer email for a description of what I think is
happening. Now I just need a neat and upstreamable fix ;-)

--
Alex Bennée