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

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

[Zenghui Yu]

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>
---
 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;
     }
 
     event.type = IOMMU_NOTIFIER_UNMAP;
-- 
2.19.1

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

[Zenghui Yu]

On 2020/12/25 17:50, Zenghui Yu wrote:
> @@ -821,6 +821,8 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
>               return;
>           }
>           granule = tt->granule_sz;
> +    } else {
> +        guanule = tg * 2 + 10;

I'm embarrassed about that.

s/guanule/granule/

>       }
>   
>       event.type = IOMMU_NOTIFIER_UNMAP;

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

[Auger Eric]

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?


> ---
>  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.

Thanks

Eric
>      }
>  
>      event.type = IOMMU_NOTIFIER_UNMAP;
> 

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

[Auger Eric]

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 ;-)

Eric
> 
> 
>> ---
>>  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.
> 
> Thanks
> 
> Eric
>>      }
>>  
>>      event.type = IOMMU_NOTIFIER_UNMAP;
>>

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

[Zenghui Yu]

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

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
>