Hi, > -----Original Message----- > From: Eric Auger [mailto:eric.au...@redhat.com] > Sent: Wednesday, September 01, 2021 8:05 PM > To: Li, Chunming; chunming; peter.mayd...@linaro.org > Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; Wen, Jianxian; Liu, > Renwei > Subject: Re: [PATCH v5 2/4] hw/arm/smmuv3: Update implementation of > CFGI commands based on device SID > > Hi, > > On 9/1/21 8:51 AM, Li, Chunming wrote: > > > >> -----Original Message----- > >> From: Eric Auger [mailto:eric.au...@redhat.com] > >> Sent: Tuesday, August 31, 2021 10:02 PM > >> To: chunming; peter.mayd...@linaro.org > >> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; Wen, Jianxian; Liu, > >> Renwei; Li, Chunming > >> Subject: Re: [PATCH v5 2/4] hw/arm/smmuv3: Update implementation of > >> CFGI commands based on device SID > >> > >> Hi Chunming > >> > >> On 8/25/21 10:08 AM, chunming wrote: > >>> From: chunming <chunming...@verisilicon.com> > >>> > >>> Replace "smmuv3_flush_config" with "g_hash_table_foreach_remove". > >> this replacement may have a potential negative impact on the > >> performance > >> for PCIe support, which is the main use case: a unique > >> g_hash_table_remove() is replaced by an iteration over all the > config > >> hash keys. > >> > >> I wonder if you couldn't just adapt smmu_iommu_mr() and it case this > >> latter returns NULL for the current PCIe search, look up in the > >> platform > >> device list: > >> > >> peri_sdev_list? > > I think there are 2 scenes: > > 1. PCIe devices share same SID with peripheral devices. > > 2. Multi peripheral devices share same SID. > > If we search PCIe 1st then search peri_sdev_list, there are 2 > problems: > > 1. The code is complex. > > 2. We may need to search peri_sdev_list multi times. It may > has performance impact. > why multiple times? 1st you look for a PCIe RID and then you look for > platform devices? Still I am dubious about the duplicate streamid case. > what I wanted to emphasize is at the moment I do not have a clear view > about your use case and I don't want to degrade the perf of the main > use > case to support a yet to be defined one ;-)
Yes, you are right. I agree with you after re-checking the code. I will update it in next version. > > > > The CFGI commands are only called when the SMMU device is removed. > > So we think there is no big performance impact. > > Nevertheless I think this is not a major issue indeed. > > Thanks > > Eric > > > >> Thanks > >> > >> Eric > >> > >> > >> > >>> "smmu_iommu_mr" function can't get MR according to SID for non > >> PCI/PCIe devices. > >>> Signed-off-by: chunming <chunming...@verisilicon.com> > >>> --- > >>> hw/arm/smmuv3.c | 35 ++++++++++---------------------- > -- > >> - > >>> include/hw/arm/smmu-common.h | 5 ++++- > >>> 2 files changed, 14 insertions(+), 26 deletions(-) > >>> > >>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > >>> index 11d7fe8423..9f3f13fb8e 100644 > >>> --- a/hw/arm/smmuv3.c > >>> +++ b/hw/arm/smmuv3.c > >>> @@ -613,14 +613,6 @@ static SMMUTransCfg > >> *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event) > >>> return cfg; > >>> } > >>> > >>> -static void smmuv3_flush_config(SMMUDevice *sdev) > >>> -{ > >>> - SMMUv3State *s = sdev->smmu; > >>> - SMMUState *bc = &s->smmu_state; > >>> - > >>> - trace_smmuv3_config_cache_inv(smmu_get_sid(sdev)); > >>> - g_hash_table_remove(bc->configs, sdev); > >>> -} > >>> > >>> static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, > hwaddr > >> addr, > >>> IOMMUAccessFlags flag, int > >> iommu_idx) > >>> @@ -964,22 +956,18 @@ static int smmuv3_cmdq_consume(SMMUv3State > *s) > >>> case SMMU_CMD_CFGI_STE: > >>> { > >>> uint32_t sid = CMD_SID(&cmd); > >>> - IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, sid); > >>> - SMMUDevice *sdev; > >>> + SMMUSIDRange sid_range; > >>> > >>> if (CMD_SSEC(&cmd)) { > >>> cmd_error = SMMU_CERROR_ILL; > >>> break; > >>> } > >>> > >>> - if (!mr) { > >>> - break; > >>> - } > >>> - > >>> + sid_range.start = sid; > >>> + sid_range.end = sid; > >>> trace_smmuv3_cmdq_cfgi_ste(sid); > >>> - sdev = container_of(mr, SMMUDevice, iommu); > >>> - smmuv3_flush_config(sdev); > >>> - > >>> + g_hash_table_foreach_remove(bs->configs, > >> smmuv3_invalidate_ste, > >>> + &sid_range); > >>> break; > >>> } > >>> case SMMU_CMD_CFGI_STE_RANGE: /* same as SMMU_CMD_CFGI_ALL > >> */ > >>> @@ -1006,21 +994,18 @@ static int smmuv3_cmdq_consume(SMMUv3State > *s) > >>> case SMMU_CMD_CFGI_CD_ALL: > >>> { > >>> uint32_t sid = CMD_SID(&cmd); > >>> - IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, sid); > >>> - SMMUDevice *sdev; > >>> + SMMUSIDRange sid_range; > >>> > >>> if (CMD_SSEC(&cmd)) { > >>> cmd_error = SMMU_CERROR_ILL; > >>> break; > >>> } > >>> > >>> - if (!mr) { > >>> - break; > >>> - } > >>> - > >>> + sid_range.start = sid; > >>> + sid_range.end = sid; > >>> trace_smmuv3_cmdq_cfgi_cd(sid); > >>> - sdev = container_of(mr, SMMUDevice, iommu); > >>> - smmuv3_flush_config(sdev); > >>> + g_hash_table_foreach_remove(bs->configs, > >> smmuv3_invalidate_ste, > >>> + &sid_range); > >>> break; > >>> } > >>> case SMMU_CMD_TLBI_NH_ASID: > >>> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu- > >> common.h > >>> index 95cd12a4b5..d016455d80 100644 > >>> --- a/include/hw/arm/smmu-common.h > >>> +++ b/include/hw/arm/smmu-common.h > >>> @@ -159,7 +159,10 @@ int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t > iova, > >> IOMMUAccessFlags perm, > >>> */ > >>> SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova); > >>> > >>> -/* Return the iommu mr associated to @sid, or NULL if none */ > >>> +/** > >>> + * Return the iommu mr associated to @sid, or NULL if none > >>> + * Only for PCI device, check smmu_find_peri_sdev for non PCI/PCIe > >> device > >>> + */ > >>> IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid); > >>> > >>> #define SMMU_IOTLB_MAX_SIZE 256