Re: [Linuxarm] Re: [PATCH v14 07/13] iommu/smmuv3: Implement cache_invalidate
Hi Eric, 在 2021/3/22 17:05, Auger Eric 写道: Hi Chenxiang, On 3/22/21 7:40 AM, chenxiang (M) wrote: Hi Eric, 在 2021/3/20 1:36, Auger Eric 写道: Hi Chenxiang, On 3/4/21 8:55 AM, chenxiang (M) wrote: Hi Eric, 在 2021/2/24 4:56, Eric Auger 写道: Implement domain-selective, pasid selective and page-selective IOTLB invalidations. Signed-off-by: Eric Auger --- v13 -> v14: - Add domain invalidation - do global inval when asid is not provided with addr granularity v7 -> v8: - ASID based invalidation using iommu_inv_pasid_info - check ARCHID/PASID flags in addr based invalidation - use __arm_smmu_tlb_inv_context and __arm_smmu_tlb_inv_range_nosync v6 -> v7 - check the uapi version v3 -> v4: - adapt to changes in the uapi - add support for leaf parameter - do not use arm_smmu_tlb_inv_range_nosync or arm_smmu_tlb_inv_context anymore v2 -> v3: - replace __arm_smmu_tlb_sync by arm_smmu_cmdq_issue_sync v1 -> v2: - properly pass the asid --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 74 + 1 file changed, 74 insertions(+) diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 4c19a1114de4..df3adc49111c 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2949,6 +2949,79 @@ static void arm_smmu_detach_pasid_table(struct iommu_domain *domain) mutex_unlock(&smmu_domain->init_mutex); } +static int +arm_smmu_cache_invalidate(struct iommu_domain *domain, struct device *dev, + struct iommu_cache_invalidate_info *inv_info) +{ +struct arm_smmu_cmdq_ent cmd = {.opcode = CMDQ_OP_TLBI_NSNH_ALL}; +struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); +struct arm_smmu_device *smmu = smmu_domain->smmu; + +if (smmu_domain->stage != ARM_SMMU_DOMAIN_NESTED) +return -EINVAL; + +if (!smmu) +return -EINVAL; + +if (inv_info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1) +return -EINVAL; + +if (inv_info->cache & IOMMU_CACHE_INV_TYPE_PASID || +inv_info->cache & IOMMU_CACHE_INV_TYPE_DEV_IOTLB) { +return -ENOENT; +} + +if (!(inv_info->cache & IOMMU_CACHE_INV_TYPE_IOTLB)) +return -EINVAL; + +/* IOTLB invalidation */ + +switch (inv_info->granularity) { +case IOMMU_INV_GRANU_PASID: +{ +struct iommu_inv_pasid_info *info = +&inv_info->granu.pasid_info; + +if (info->flags & IOMMU_INV_ADDR_FLAGS_PASID) +return -ENOENT; +if (!(info->flags & IOMMU_INV_PASID_FLAGS_ARCHID)) +return -EINVAL; + +__arm_smmu_tlb_inv_context(smmu_domain, info->archid); +return 0; +} +case IOMMU_INV_GRANU_ADDR: +{ +struct iommu_inv_addr_info *info = &inv_info->granu.addr_info; +size_t size = info->nb_granules * info->granule_size; +bool leaf = info->flags & IOMMU_INV_ADDR_FLAGS_LEAF; + +if (info->flags & IOMMU_INV_ADDR_FLAGS_PASID) +return -ENOENT; + +if (!(info->flags & IOMMU_INV_ADDR_FLAGS_ARCHID)) +break; + +arm_smmu_tlb_inv_range_domain(info->addr, size, + info->granule_size, leaf, + info->archid, smmu_domain); Is it possible to add a check before the function to make sure that info->granule_size can be recognized by SMMU? There is a scenario which will cause TLBI issue: RIL feature is enabled on guest but is disabled on host, and then on host it just invalidate 4K/2M/1G once a time, but from QEMU, info->nb_granules is always 1 and info->granule_size = size, if size is not equal to 4K or 2M or 1G (for example size = granule_size is 5M), it will only part of the size it wants to invalidate. Do you have any idea about this issue? At the QEMU VFIO notifier level, when I fill the struct iommu_cache_invalidate_info, I currently miss the granule info, hence this weird choice of setting setting info->nb_granules is always 1 and info->granule_size = size. I think in arm_smmu_cache_invalidate() I need to convert this info back to a the leaf page size, in case the host does not support RIL. Just as it is done in __arm_smmu_tlb_inv_range if RIL is supported. Does it makes sense to you? Yes, it makes sense to me. I am glad to test them if the patchset are ready. Thank you for spotting the issue. Eric I think maybe we can add a check here: if RIL is not enabled and also size is not the granule_size (4K/2M/1G) supported by SMMU hardware, can we just simply use the smallest granule_size supported by hardware all the time? + +arm_smmu_cmdq_issue_sync(smmu); +return 0; +} +case IOMMU_INV_GRANU_DOMAIN: +break; I check the qemu code (https://github.com/eauger/qemu/tree/v5.2.0-2stage-rfcv8), for opcode CMD_TLBI_NH_ALL or CMD_TLBI_NSNH_ALL from guest OS it calls smmu_inv_notifiers_all() to unamp all notifiers of all mr's, but it seems not set eve
Re: [Linuxarm] Re: [PATCH v14 07/13] iommu/smmuv3: Implement cache_invalidate
Hi Chenxiang, On 3/22/21 7:40 AM, chenxiang (M) wrote: > Hi Eric, > > > 在 2021/3/20 1:36, Auger Eric 写道: >> Hi Chenxiang, >> >> On 3/4/21 8:55 AM, chenxiang (M) wrote: >>> Hi Eric, >>> >>> >>> 在 2021/2/24 4:56, Eric Auger 写道: Implement domain-selective, pasid selective and page-selective IOTLB invalidations. Signed-off-by: Eric Auger --- v13 -> v14: - Add domain invalidation - do global inval when asid is not provided with addr granularity v7 -> v8: - ASID based invalidation using iommu_inv_pasid_info - check ARCHID/PASID flags in addr based invalidation - use __arm_smmu_tlb_inv_context and __arm_smmu_tlb_inv_range_nosync v6 -> v7 - check the uapi version v3 -> v4: - adapt to changes in the uapi - add support for leaf parameter - do not use arm_smmu_tlb_inv_range_nosync or arm_smmu_tlb_inv_context anymore v2 -> v3: - replace __arm_smmu_tlb_sync by arm_smmu_cmdq_issue_sync v1 -> v2: - properly pass the asid --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 74 + 1 file changed, 74 insertions(+) diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 4c19a1114de4..df3adc49111c 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2949,6 +2949,79 @@ static void arm_smmu_detach_pasid_table(struct iommu_domain *domain) mutex_unlock(&smmu_domain->init_mutex); } +static int +arm_smmu_cache_invalidate(struct iommu_domain *domain, struct device *dev, + struct iommu_cache_invalidate_info *inv_info) +{ + struct arm_smmu_cmdq_ent cmd = {.opcode = CMDQ_OP_TLBI_NSNH_ALL}; + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); + struct arm_smmu_device *smmu = smmu_domain->smmu; + + if (smmu_domain->stage != ARM_SMMU_DOMAIN_NESTED) + return -EINVAL; + + if (!smmu) + return -EINVAL; + + if (inv_info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1) + return -EINVAL; + + if (inv_info->cache & IOMMU_CACHE_INV_TYPE_PASID || + inv_info->cache & IOMMU_CACHE_INV_TYPE_DEV_IOTLB) { + return -ENOENT; + } + + if (!(inv_info->cache & IOMMU_CACHE_INV_TYPE_IOTLB)) + return -EINVAL; + + /* IOTLB invalidation */ + + switch (inv_info->granularity) { + case IOMMU_INV_GRANU_PASID: + { + struct iommu_inv_pasid_info *info = + &inv_info->granu.pasid_info; + + if (info->flags & IOMMU_INV_ADDR_FLAGS_PASID) + return -ENOENT; + if (!(info->flags & IOMMU_INV_PASID_FLAGS_ARCHID)) + return -EINVAL; + + __arm_smmu_tlb_inv_context(smmu_domain, info->archid); + return 0; + } + case IOMMU_INV_GRANU_ADDR: + { + struct iommu_inv_addr_info *info = &inv_info->granu.addr_info; + size_t size = info->nb_granules * info->granule_size; + bool leaf = info->flags & IOMMU_INV_ADDR_FLAGS_LEAF; + + if (info->flags & IOMMU_INV_ADDR_FLAGS_PASID) + return -ENOENT; + + if (!(info->flags & IOMMU_INV_ADDR_FLAGS_ARCHID)) + break; + + arm_smmu_tlb_inv_range_domain(info->addr, size, + info->granule_size, leaf, + info->archid, smmu_domain); >>> Is it possible to add a check before the function to make sure that >>> info->granule_size can be recognized by SMMU? >>> There is a scenario which will cause TLBI issue: RIL feature is enabled >>> on guest but is disabled on host, and then on >>> host it just invalidate 4K/2M/1G once a time, but from QEMU, >>> info->nb_granules is always 1 and info->granule_size = size, >>> if size is not equal to 4K or 2M or 1G (for example size = granule_size >>> is 5M), it will only part of the size it wants to invalidate. > > Do you have any idea about this issue? At the QEMU VFIO notifier level, when I fill the struct iommu_cache_invalidate_info, I currently miss the granule info, hence this weird choice of setting setting info->nb_granules is always 1 and info->granule_size = size. I think in arm_smmu_cache_invalidate() I need to convert this info back to a the leaf page size, in case the host does not support RIL. Just as it is done in __arm_smmu_tlb_inv_range if RIL is supported. Does it makes sense to you? Thank you for spotting the issue. Eric > >>> >>> I think maybe we can add a check here: if RIL is not enabled and also >>> size is not the granule_size (4K/2M/1G) supported by >>> SMMU hardware,
Re: [Linuxarm] Re: [PATCH v14 07/13] iommu/smmuv3: Implement cache_invalidate
Hi Eric, 在 2021/3/20 1:36, Auger Eric 写道: Hi Chenxiang, On 3/4/21 8:55 AM, chenxiang (M) wrote: Hi Eric, 在 2021/2/24 4:56, Eric Auger 写道: Implement domain-selective, pasid selective and page-selective IOTLB invalidations. Signed-off-by: Eric Auger --- v13 -> v14: - Add domain invalidation - do global inval when asid is not provided with addr granularity v7 -> v8: - ASID based invalidation using iommu_inv_pasid_info - check ARCHID/PASID flags in addr based invalidation - use __arm_smmu_tlb_inv_context and __arm_smmu_tlb_inv_range_nosync v6 -> v7 - check the uapi version v3 -> v4: - adapt to changes in the uapi - add support for leaf parameter - do not use arm_smmu_tlb_inv_range_nosync or arm_smmu_tlb_inv_context anymore v2 -> v3: - replace __arm_smmu_tlb_sync by arm_smmu_cmdq_issue_sync v1 -> v2: - properly pass the asid --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 74 + 1 file changed, 74 insertions(+) diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 4c19a1114de4..df3adc49111c 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2949,6 +2949,79 @@ static void arm_smmu_detach_pasid_table(struct iommu_domain *domain) mutex_unlock(&smmu_domain->init_mutex); } +static int +arm_smmu_cache_invalidate(struct iommu_domain *domain, struct device *dev, + struct iommu_cache_invalidate_info *inv_info) +{ + struct arm_smmu_cmdq_ent cmd = {.opcode = CMDQ_OP_TLBI_NSNH_ALL}; + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); + struct arm_smmu_device *smmu = smmu_domain->smmu; + + if (smmu_domain->stage != ARM_SMMU_DOMAIN_NESTED) + return -EINVAL; + + if (!smmu) + return -EINVAL; + + if (inv_info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1) + return -EINVAL; + + if (inv_info->cache & IOMMU_CACHE_INV_TYPE_PASID || + inv_info->cache & IOMMU_CACHE_INV_TYPE_DEV_IOTLB) { + return -ENOENT; + } + + if (!(inv_info->cache & IOMMU_CACHE_INV_TYPE_IOTLB)) + return -EINVAL; + + /* IOTLB invalidation */ + + switch (inv_info->granularity) { + case IOMMU_INV_GRANU_PASID: + { + struct iommu_inv_pasid_info *info = + &inv_info->granu.pasid_info; + + if (info->flags & IOMMU_INV_ADDR_FLAGS_PASID) + return -ENOENT; + if (!(info->flags & IOMMU_INV_PASID_FLAGS_ARCHID)) + return -EINVAL; + + __arm_smmu_tlb_inv_context(smmu_domain, info->archid); + return 0; + } + case IOMMU_INV_GRANU_ADDR: + { + struct iommu_inv_addr_info *info = &inv_info->granu.addr_info; + size_t size = info->nb_granules * info->granule_size; + bool leaf = info->flags & IOMMU_INV_ADDR_FLAGS_LEAF; + + if (info->flags & IOMMU_INV_ADDR_FLAGS_PASID) + return -ENOENT; + + if (!(info->flags & IOMMU_INV_ADDR_FLAGS_ARCHID)) + break; + + arm_smmu_tlb_inv_range_domain(info->addr, size, + info->granule_size, leaf, + info->archid, smmu_domain); Is it possible to add a check before the function to make sure that info->granule_size can be recognized by SMMU? There is a scenario which will cause TLBI issue: RIL feature is enabled on guest but is disabled on host, and then on host it just invalidate 4K/2M/1G once a time, but from QEMU, info->nb_granules is always 1 and info->granule_size = size, if size is not equal to 4K or 2M or 1G (for example size = granule_size is 5M), it will only part of the size it wants to invalidate. Do you have any idea about this issue? I think maybe we can add a check here: if RIL is not enabled and also size is not the granule_size (4K/2M/1G) supported by SMMU hardware, can we just simply use the smallest granule_size supported by hardware all the time? + + arm_smmu_cmdq_issue_sync(smmu); + return 0; + } + case IOMMU_INV_GRANU_DOMAIN: + break; I check the qemu code (https://github.com/eauger/qemu/tree/v5.2.0-2stage-rfcv8), for opcode CMD_TLBI_NH_ALL or CMD_TLBI_NSNH_ALL from guest OS it calls smmu_inv_notifiers_all() to unamp all notifiers of all mr's, but it seems not set event.entry.granularity which i think it should set IOMMU_INV_GRAN_ADDR. this is because IOMMU_INV_GRAN_ADDR = 0. But for clarity I should rather set it explicitly ;-) ok BTW, for opcode CMD_TLBI_NH_ALL or CMD_TLBI_NSNH_ALL, it needs to unmap size = 0x1 on 48bit system (it may spend much time), maybe it is better to set it as IOMMU_INV_GRANU_DOMAIN, then in host OS, send