| Message ID | 20201225095015.609-1-yuzenghui@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/smmuv3: Fix addr_mask for range-based invalidation | expand |
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;
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; >
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; >>
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
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 >
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;
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(-)