Re: [PATCH 3/7] iommu/arm-smmu: Add tlb_sync implementation hook

Thierry Reding
2019-08-30T11:16:04Z

[-- Attachment #1.1: Type: text/plain, Size: 3970 bytes --]

On Thu, Aug 29, 2019 at 03:47:03PM -0700, Krishna Reddy wrote:
> tlb_sync hook allows nvidia smmu handle tlb sync
> across multiple SMMUs as necessary.
>
> Signed-off-by: Krishna Reddy <vdumpa@nvidia.com>
> ---
> drivers/iommu/arm-smmu-nvidia.c | 32 ++++++++++++++++++++++++++++++++
> drivers/iommu/arm-smmu.c | 8 +++++---
> drivers/iommu/arm-smmu.h | 4 ++++
> 3 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-nvidia.c b/drivers/iommu/arm-smmu-nvidia.c
> index d93ceda..a429b2c 100644
> --- a/drivers/iommu/arm-smmu-nvidia.c
> +++ b/drivers/iommu/arm-smmu-nvidia.c
> @@ -56,11 +56,43 @@ static void nsmmu_write_reg64(struct arm_smmu_device *smmu,
> writeq_relaxed(val, nsmmu_page(smmu, i, page) + offset);
> }
>
> +static void nsmmu_tlb_sync_wait(struct arm_smmu_device *smmu, int page,
> + int sync, int status, int inst)
> +{
> + u32 reg;
> + unsigned int spin_cnt, delay;
> +
> + for (delay = 1; delay < TLB_LOOP_TIMEOUT; delay *= 2) {
> + for (spin_cnt = TLB_SPIN_COUNT; spin_cnt > 0; spin_cnt--) {
> + reg = readl_relaxed(
> + nsmmu_page(smmu, inst, page) + status);
> + if (!(reg & sTLBGSTATUS_GSACTIVE))
> + return;
> + cpu_relax();
> + }
> + udelay(delay);
> + }
> + dev_err_ratelimited(smmu->dev,
> + "TLB sync timed out -- SMMU may be deadlocked\n");
> +}
> +
> +static void nsmmu_tlb_sync(struct arm_smmu_device *smmu, int page,
> + int sync, int status)
> +{
> + int i;
> +
> + arm_smmu_writel(smmu, page, sync, 0);
> +
> + for (i = 0; i < to_nsmmu(smmu)->num_inst; i++)
> + nsmmu_tlb_sync_wait(smmu, page, sync, status, i);
> +}
> +
> static const struct arm_smmu_impl nsmmu_impl = {
> .read_reg = nsmmu_read_reg,
> .write_reg = nsmmu_write_reg,
> .read_reg64 = nsmmu_read_reg64,
> .write_reg64 = nsmmu_write_reg64,
> + .tlb_sync = nsmmu_tlb_sync,
> };
>
> struct arm_smmu_device *nvidia_smmu_impl_init(struct arm_smmu_device *smmu)
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 46e1641..f5454e71 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -52,9 +52,6 @@
> */
> #define QCOM_DUMMY_VAL -1
>
> -#define TLB_LOOP_TIMEOUT 1000000 /* 1s! */
> -#define TLB_SPIN_COUNT 10
> -
> #define MSI_IOVA_BASE 0x8000000
> #define MSI_IOVA_LENGTH 0x100000
>
> @@ -244,6 +241,11 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu, int page,
> unsigned int spin_cnt, delay;
> u32 reg;
>
> + if (smmu->impl->tlb_sync) {
> + smmu->impl->tlb_sync(smmu, page, sync, status);
> + return;
> + }
> +

Wouldn't it work if you replaced all calls of __arm_smmu_tlb_sync() by
smmu->impl->tlb_sync() and assign __arm_smmu_tlb_sync() as default for
devices that don't need to override it? That makes this patch slightly
larger, but it saves us one level of indirection.

> +
> arm_smmu_writel(smmu, page, sync, QCOM_DUMMY_VAL);
> for (delay = 1; delay < TLB_LOOP_TIMEOUT; delay *= 2) {
> for (spin_cnt = TLB_SPIN_COUNT; spin_cnt > 0; spin_cnt--) {
> diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> index 9645bf1..d3217f1 100644
> --- a/drivers/iommu/arm-smmu.h
> +++ b/drivers/iommu/arm-smmu.h
> @@ -207,6 +207,8 @@ enum arm_smmu_cbar_type {
> /* Maximum number of context banks per SMMU */
> #define ARM_SMMU_MAX_CBS 128
>
> +#define TLB_LOOP_TIMEOUT 1000000 /* 1s! */
> +#define TLB_SPIN_COUNT 10
>
> /* Shared driver definitions */
> enum arm_smmu_arch_version {
> @@ -336,6 +338,8 @@ struct arm_smmu_impl {
> int (*cfg_probe)(struct arm_smmu_device *smmu);
> int (*reset)(struct arm_smmu_device *smmu);
> int (*init_context)(struct arm_smmu_domain *smmu_domain);
> + void (*tlb_sync)(struct arm_smmu_device *smmu, int page, int sync,
> + int status);

Can't page, sync and status all be unsigned?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]