hw/arm/smmuv3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
From: Xiang Chen <chenxiang66@hisilicon.com>
In function memory_region_iommu_replay(), it decides to notify() or not
according to the perm of returned IOMMUTLBEntry. But for smmuv3, the
returned perm is always IOMMU_NONE even if the translation success.
Pass the real perm to returned IOMMUTLBEntry to avoid the issue.
Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
---
hw/arm/smmuv3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 674623aabe..707eb430c2 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -760,7 +760,7 @@ epilogue:
qemu_mutex_unlock(&s->mutex);
switch (status) {
case SMMU_TRANS_SUCCESS:
- entry.perm = flag;
+ entry.perm = cached_entry->entry.perm;
entry.translated_addr = cached_entry->entry.translated_addr +
(addr & cached_entry->entry.addr_mask);
entry.addr_mask = cached_entry->entry.addr_mask;
--
2.33.0
Hi Chenxiang, On 4/7/22 9:57 AM, chenxiang via wrote: > From: Xiang Chen <chenxiang66@hisilicon.com> > > In function memory_region_iommu_replay(), it decides to notify() or not > according to the perm of returned IOMMUTLBEntry. But for smmuv3, the > returned perm is always IOMMU_NONE even if the translation success. I think you should precise in the commit message that memory_region_iommu_replay() always calls the IOMMU MR translate() callback with flag=IOMMU_NONE and thus, currently, translate() returns an IOMMUTLBEntry with perm set to IOMMU_NONE if the translation succeeds, whereas it is expected to return the actual permission set in the table entry. > Pass the real perm to returned IOMMUTLBEntry to avoid the issue. > > Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com> > --- > hw/arm/smmuv3.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > index 674623aabe..707eb430c2 100644 > --- a/hw/arm/smmuv3.c > +++ b/hw/arm/smmuv3.c > @@ -760,7 +760,7 @@ epilogue: > qemu_mutex_unlock(&s->mutex); > switch (status) { > case SMMU_TRANS_SUCCESS: > - entry.perm = flag; > + entry.perm = cached_entry->entry.perm; With that clarification Reviewed-by: Eric Auger <eric.auger@redhat.com> the translate() doc in ./include/exec/memory.h states " If IOMMU_NONE is passed then the IOMMU must do the * full page table walk and report the permissions in the returned * IOMMUTLBEntry. (Note that this implies that an IOMMU may not * return different mappings for reads and writes.) " Thanks Eric > entry.translated_addr = cached_entry->entry.translated_addr + > (addr & cached_entry->entry.addr_mask); > entry.addr_mask = cached_entry->entry.addr_mask;
Hi Eric, 在 2022/4/15 0:02, Eric Auger 写道: > Hi Chenxiang, > > On 4/7/22 9:57 AM, chenxiang via wrote: >> From: Xiang Chen <chenxiang66@hisilicon.com> >> >> In function memory_region_iommu_replay(), it decides to notify() or not >> according to the perm of returned IOMMUTLBEntry. But for smmuv3, the >> returned perm is always IOMMU_NONE even if the translation success. > I think you should precise in the commit message that > memory_region_iommu_replay() always calls the IOMMU MR translate() > callback with flag=IOMMU_NONE and thus, currently, translate() returns > an IOMMUTLBEntry with perm set to IOMMU_NONE if the translation > succeeds, whereas it is expected to return the actual permission set in > the table entry. Thank you for your comments. I will change the commit message in next version. > > >> Pass the real perm to returned IOMMUTLBEntry to avoid the issue. >> >> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com> >> --- >> hw/arm/smmuv3.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c >> index 674623aabe..707eb430c2 100644 >> --- a/hw/arm/smmuv3.c >> +++ b/hw/arm/smmuv3.c >> @@ -760,7 +760,7 @@ epilogue: >> qemu_mutex_unlock(&s->mutex); >> switch (status) { >> case SMMU_TRANS_SUCCESS: >> - entry.perm = flag; >> + entry.perm = cached_entry->entry.perm; > With that clarification > Reviewed-by: Eric Auger <eric.auger@redhat.com> Ok, thanks > > the translate() doc in ./include/exec/memory.h states > " > If IOMMU_NONE is passed then the IOMMU must do the > * full page table walk and report the permissions in the returned > * IOMMUTLBEntry. (Note that this implies that an IOMMU may not > * return different mappings for reads and writes.) > " > > > Thanks > > Eric >> entry.translated_addr = cached_entry->entry.translated_addr + >> (addr & cached_entry->entry.addr_mask); >> entry.addr_mask = cached_entry->entry.addr_mask; > . >
© 2016 - 2024 Red Hat, Inc.