RE: [PATCH v5 2/4] hw/arm/smmuv3: Update implementation of CFGI commands based on device SID
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 > >>> > >>> 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 > >>> --- > >>> 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 = SMM
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 >>> >>> 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 ;-) > > 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 >>> --- >>> 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
RE: [PATCH v5 2/4] hw/arm/smmuv3: Update implementation of CFGI commands based on device SID
> -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 > > > > 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. The CFGI commands are only called when the SMMU device is removed. So we think there is no big performance impact. > > Thanks > > Eric > > > > > "smmu_iommu_mr" function can't get MR according to SID for non > PCI/PCIe devices. > > > > Signed-off-by: chunming > > --- > > 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, >
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 > > 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? Thanks Eric > "smmu_iommu_mr" function can't get MR according to SID for non PCI/PCIe > devices. > > Signed-off-by: chunming > --- > 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