Re: [Qemu-devel] [PATCH 26/26] tcg: Use tlb_fill probe from tlb_vaddr_to_host

From: Richard Henderson <richard.henderson@linaro.org>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH 26/26] tcg: Use tlb_fill probe from tlb_vaddr_to_host
Date: Thu, 9 May 2019 15:24:51 -0700	[thread overview]
Message-ID: <d790a33a-bbe8-70a7-f9e2-65387b8def45@linaro.org> (raw)
In-Reply-To: <CAFEAcA-Agc6KWMqAA3V7tA0aNYF8NhDuC6Opw2Qx5qVey-YTfQ@mail.gmail.com>

On 5/9/19 1:56 AM, Peter Maydell wrote:
> On Thu, 9 May 2019 at 06:24, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 4/29/19 10:41 AM, Peter Maydell wrote:
>>> On Wed, 3 Apr 2019 at 05:05, Richard Henderson
>>> <richard.henderson@linaro.org> wrote:
>>>>
>>>> Most of the existing users would continue around a loop which
>>>> would fault the tlb entry in via a normal load/store.  But for
>>>> SVE we have a true non-faulting case which requires the new
>>>> probing form of tlb_fill.
>>>
>>> So am I right in thinking that this fixes a bug where we
>>> previously would mark a load as faulted if the memory happened
>>> not to be in the TLB, whereas now we will correctly pull in the
>>> TLB entry and do the load ?
>>
>> Yes.
>>
>>> (Since guest code ought to be handling the "non-first-load
>>> faulted" case by looping round or otherwise arranging to
>>> retry, nothing in practice would have noticed this bug, right?)
>>
>> Yes.
>>
>> The only case with changed behaviour is (expected to be) SVE no-fault, where
>> the loop you mention would have produced different incorrect results.
> 
> OK. If we're fixing a guest-visible bug it would be nice to
> describe that in the commit message.

The commit message now reads, in part,

But for AArch64 SVE we have an existing emulation bug wherein we
would mark the first element of a no-fault vector load as faulted
(within the FFR, not via exception) just because we did not have
its address in the TLB.  Now we can properly only mark it as faulted
if there really is no valid, readable translation, while still not
raising an exception.  (Note that beyond the first element of the
vector, the hardware may report a fault for any reason whatsoever;
with at least one element loaded, forward progress is guaranteed.)

r~