Re: [PATCH v5 2/4] hw/arm/smmuv3: Update implementation of CFGI commands based on device SID

[Eric Auger]

Hi,

On 9/1/21 8:51 AM, Li, Chunming wrote:
>
>> -----Original Message-----
>> From: Eric Auger [mailto:eric.auger@redhat.com]
>> Sent: Tuesday, August 31, 2021 10:02 PM
>> To: chunming; peter.maydell@linaro.org
>> Cc: qemu-arm@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.li@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 ;-)
>         
> 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.li@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