Re: [PATCH] hw/arm/smmuv3: Fix addr_mask for range-based invalidation

From: Zenghui Yu <yuzenghui@huawei.com>
To: Auger Eric <eric.auger@redhat.com>, <qemu-devel@nongnu.org>,
	<qemu-arm@nongnu.org>, <peter.maydell@linaro.org>
Cc: wanghaibin.wang@huawei.com
Subject: Re: [PATCH] hw/arm/smmuv3: Fix addr_mask for range-based invalidation
Date: Fri, 29 Jan 2021 20:15:46 +0800	[thread overview]
Message-ID: <aca67ce0-5e26-a0cf-1c31-4b08694afda4@huawei.com> (raw)
In-Reply-To: <45f3334d-f0b4-9a40-1bd1-78bd04735eaf@redhat.com>

Hi Eric,

On 2021/1/29 5:30, Auger Eric wrote:
> Hi Zenghui,
> 
> On 1/28/21 9:25 AM, Auger Eric wrote:
>> Hi Zenghui,
>>
>> On 12/25/20 10:50 AM, Zenghui Yu wrote:
>>> When performing range-based IOTLB invalidation, we should decode the TG
>>> field into the corresponding translation granule size so that we can pass
>>> the correct invalidation range to backend. Set @granule to (tg * 2 + 10) to
>>> properly emulate the architecture.
>>>
>>> Fixes: d52915616c05 ("hw/arm/smmuv3: Get prepared for range invalidation")
>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>>
>> Good catch! I tested with older guest kernels though. I wonder how I did
>> not face the bug?
> Please ignore this wrong comment as this corresponds to recent kernels
> instead. Still puzzled anyway ;-)

I noticed this when looking through your nested SMMU series and I didn't
have much clue about the impact on the real setups.

I guess we may receive some unexpected fault events with this bug. But I
think we may miss it for some reasons:

  - the stale TLB entries happen to be evicted due to heavy traffic
  - some form of over-invalidation is performed by your implementation
  - ...

>>> ---
>>>   hw/arm/smmuv3.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>>> index bbca0e9f20..65231c7d52 100644
>>> --- a/hw/arm/smmuv3.c
>>> +++ b/hw/arm/smmuv3.c
>>> @@ -801,7 +801,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
>>>   {
>>>       SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
>>>       IOMMUTLBEvent event;
>>> -    uint8_t granule = tg;
>>> +    uint8_t granule;
>>>   
>>>       if (!tg) {
>>>           SMMUEventInfo event = {.inval_ste_allowed = true};
>>> @@ -821,6 +821,8 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
>>>               return;
>>>           }
>>>           granule = tt->granule_sz;
>>> +    } else {
>>> +        guanule = tg * 2 + 10;
>> maybe just init granule to this value above while fixing the typo.

My intention is to initialize @granule to this value explicitly for the
range-based invalidation case. But I'm okay with either way.

Thanks,
Zenghui