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

[Auger Eric]

Hi Zenghui,

On 1/29/21 1:15 PM, Zenghui Yu wrote:
> 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
>  - ...
Yep I will further trace things. Anyway thank you for spotting it.
> 
>>>> ---
>>>>   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.
same for me ;-)

Eric
> 
> 
> Thanks,
> Zenghui
>