Re: [PATCH v5 5/5] iommu/vt-d: Check UAPI data processed by IOMMU core

[Lu Baolu]

On 7/17/20 2:45 AM, Jacob Pan wrote:

IOMMU generic layer already does sanity checks UAPI data for version
match and argsz range under generic information.
Remove the redundant version check from VT-d driver and check for vendor
specific data size.

Signed-off-by: Jacob Pan 


Reviewed-by: Lu Baolu 

Best regards,
baolu


---
  drivers/iommu/intel/iommu.c | 3 +--
  drivers/iommu/intel/svm.c   | 7 +--
  2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f3a6ca88cf95..5e80484f0537 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -5383,8 +5383,7 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain, 
struct device *dev,
int ret = 0;
u64 size = 0;
  
-	if (!inv_info || !dmar_domain ||

-   inv_info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
+   if (!inv_info || !dmar_domain)
return -EINVAL;
  
  	if (!dev || !dev_is_pci(dev))

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 713b3a218483..55ea11e9c0f5 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -240,8 +240,11 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, 
struct device *dev,
if (WARN_ON(!iommu) || !data)
return -EINVAL;
  
-	if (data->version != IOMMU_GPASID_BIND_VERSION_1 ||

-   data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
+   if (data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
+   return -EINVAL;
+
+   /* IOMMU core ensures argsz is more than the start of the union */
+   if (data->argsz < offsetofend(struct iommu_gpasid_bind_data, 
vendor.vtd))
return -EINVAL;
  
  	if (!dev_is_pci(dev))



___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 5/5] iommu/arm-smmu: Setup identity domain for boot mappings

[Bjorn Andersson]

With many Qualcomm platforms not having functional S2CR BYPASS a
temporary IOMMU domain, without translation, needs to be allocated in
order to allow these memory transactions.

Unfortunately the boot loader uses the first few context banks, so
rather than overwriting a active bank the last context bank is used and
streams are diverted here during initialization.

This also performs the readback of SMR registers for the Qualcomm
platform, to trigger the mechanism.

This is based on prior work by Thierry Reding and Laurentiu Tudor.

Tested-by: John Stultz 
Tested-by: Vinod Koul 
Signed-off-by: Bjorn Andersson 
---

Changes since v1:
- Rebased to avoid conflict
- Picked up tested-by

 drivers/iommu/arm-smmu-qcom.c | 11 +
 drivers/iommu/arm-smmu.c  | 79 +--
 drivers/iommu/arm-smmu.h  |  3 ++
 3 files changed, 89 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
index 10eb024981d1..147af11049eb 100644
--- a/drivers/iommu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm-smmu-qcom.c
@@ -26,6 +26,7 @@ static const struct of_device_id qcom_smmu_client_of_match[] 
__maybe_unused = {
 static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
 {
unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 
1);
+   u32 smr;
u32 reg;
int i;
 
@@ -56,6 +57,16 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
}
}
 
+   for (i = 0; i < smmu->num_mapping_groups; i++) {
+   smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
+
+   if (FIELD_GET(ARM_SMMU_SMR_VALID, smr)) {
+   smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
+   smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
+   smmu->smrs[i].valid = true;
+   }
+   }
+
return 0;
 }
 
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 08a650fe02e3..69bd8ee03516 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -652,7 +652,8 @@ static void arm_smmu_write_context_bank(struct 
arm_smmu_device *smmu, int idx)
 }
 
 static int arm_smmu_init_domain_context(struct iommu_domain *domain,
-   struct arm_smmu_device *smmu)
+   struct arm_smmu_device *smmu,
+   bool boot_domain)
 {
int irq, start, ret = 0;
unsigned long ias, oas;
@@ -770,6 +771,15 @@ static int arm_smmu_init_domain_context(struct 
iommu_domain *domain,
ret = -EINVAL;
goto out_unlock;
}
+
+   /*
+* Use the last context bank for identity mappings during boot, to
+* avoid overwriting in-use bank configuration while we're setting up
+* the new mappings.
+*/
+   if (boot_domain)
+   start = smmu->num_context_banks - 1;
+
ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
  smmu->num_context_banks);
if (ret < 0)
@@ -1149,7 +1159,10 @@ static int arm_smmu_attach_dev(struct iommu_domain 
*domain, struct device *dev)
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct arm_smmu_master_cfg *cfg;
struct arm_smmu_device *smmu;
+   bool free_identity_domain = false;
+   int idx;
int ret;
+   int i;
 
if (!fwspec || fwspec->ops != _smmu_ops) {
dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
@@ -1174,7 +1187,7 @@ static int arm_smmu_attach_dev(struct iommu_domain 
*domain, struct device *dev)
return ret;
 
/* Ensure that the domain is finalised */
-   ret = arm_smmu_init_domain_context(domain, smmu);
+   ret = arm_smmu_init_domain_context(domain, smmu, false);
if (ret < 0)
goto rpm_put;
 
@@ -1190,9 +1203,33 @@ static int arm_smmu_attach_dev(struct iommu_domain 
*domain, struct device *dev)
goto rpm_put;
}
 
+   /* Decrement use counter for any references to the identity domain */
+   mutex_lock(>stream_map_mutex);
+   if (smmu->identity) {
+   struct arm_smmu_domain *identity = 
to_smmu_domain(smmu->identity);
+
+   for_each_cfg_sme(cfg, fwspec, i, idx) {
+   if (smmu->s2crs[idx].cbndx == identity->cfg.cbndx) {
+   smmu->num_identity_masters--;
+   if (smmu->num_identity_masters == 0)
+   free_identity_domain = true;
+   }
+   }
+   }
+   mutex_unlock(>stream_map_mutex);
+
/* Looks ok, so add the device to the domain */
ret = arm_smmu_domain_add_master(smmu_domain, cfg, fwspec);
 
+   /*
+* The last stream map to reference the identity domain has been

[PATCH v2 2/5] iommu/arm-smmu: Emulate bypass by using context banks

[Bjorn Andersson]

Some firmware found on various Qualcomm platforms traps writes to S2CR
of type BYPASS and writes FAULT into the register. This prevents us from
marking the streams for the display controller as BYPASS to allow
continued scanout of the screen through the initialization of the ARM
SMMU.

This adds a Qualcomm specific cfg_probe function, which probes the
behavior of the S2CR registers and if found faulty enables the related
quirk. Based on this quirk context banks are allocated for IDENTITY
domains as well, but with ARM_SMMU_SCTLR_M omitted.

The result is valid stream mappings, without translation.

Tested-by: John Stultz 
Tested-by: Vinod Koul 
Signed-off-by: Bjorn Andersson 
---

Changes since v1:
- Picked up tested-by

 drivers/iommu/arm-smmu-qcom.c | 21 +
 drivers/iommu/arm-smmu.c  | 14 --
 drivers/iommu/arm-smmu.h  |  3 +++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
index be4318044f96..d95a5ee8c83c 100644
--- a/drivers/iommu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm-smmu-qcom.c
@@ -23,6 +23,26 @@ static const struct of_device_id qcom_smmu_client_of_match[] 
__maybe_unused = {
{ }
 };
 
+static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
+{
+   unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 
1);
+   u32 reg;
+
+   /*
+* With some firmware writes to S2CR of type FAULT are ignored, and
+* writing BYPASS will end up as FAULT in the register. Perform a write
+* to S2CR to detect if this is the case with the current firmware.
+*/
+   arm_smmu_gr0_write(smmu, last_s2cr, FIELD_PREP(ARM_SMMU_S2CR_TYPE, 
S2CR_TYPE_BYPASS) |
+   FIELD_PREP(ARM_SMMU_S2CR_CBNDX, 
0xff) |
+   FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, 
S2CR_PRIVCFG_DEFAULT));
+   reg = arm_smmu_gr0_read(smmu, last_s2cr);
+   if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS)
+   smmu->qcom_bypass_quirk = true;
+
+   return 0;
+}
+
 static int qcom_smmu_def_domain_type(struct device *dev)
 {
const struct of_device_id *match =
@@ -61,6 +81,7 @@ static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
 }
 
 static const struct arm_smmu_impl qcom_smmu_impl = {
+   .cfg_probe = qcom_smmu_cfg_probe,
.def_domain_type = qcom_smmu_def_domain_type,
.reset = qcom_smmu500_reset,
 };
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index fb85e716ae9a..5d5fe6741ed4 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -654,7 +654,9 @@ static void arm_smmu_write_context_bank(struct 
arm_smmu_device *smmu, int idx)
 
/* SCTLR */
reg = ARM_SMMU_SCTLR_CFIE | ARM_SMMU_SCTLR_CFRE | ARM_SMMU_SCTLR_AFE |
- ARM_SMMU_SCTLR_TRE | ARM_SMMU_SCTLR_M;
+ ARM_SMMU_SCTLR_TRE;
+   if (cfg->m)
+   reg |= ARM_SMMU_SCTLR_M;
if (stage1)
reg |= ARM_SMMU_SCTLR_S1_ASIDPNE;
if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
@@ -678,7 +680,11 @@ static int arm_smmu_init_domain_context(struct 
iommu_domain *domain,
if (smmu_domain->smmu)
goto out_unlock;
 
-   if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+   /*
+* Nothing to do for IDENTITY domains,unless disabled context banks are
+* used to emulate bypass mappings on Qualcomm platforms.
+*/
+   if (domain->type == IOMMU_DOMAIN_IDENTITY && !smmu->qcom_bypass_quirk) {
smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
smmu_domain->smmu = smmu;
goto out_unlock;
@@ -826,6 +832,10 @@ static int arm_smmu_init_domain_context(struct 
iommu_domain *domain,
domain->geometry.aperture_end = (1UL << ias) - 1;
domain->geometry.force_aperture = true;
 
+   /* Enable translation for non-identity context banks */
+   if (domain->type != IOMMU_DOMAIN_IDENTITY)
+   cfg->m = true;
+
/* Initialise the context bank with our page table cfg */
arm_smmu_init_context_bank(smmu_domain, _cfg);
arm_smmu_write_context_bank(smmu, cfg->cbndx);
diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index d172c024be61..a71d193073e4 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -305,6 +305,8 @@ struct arm_smmu_device {
 
/* IOMMU core code handle */
struct iommu_device iommu;
+
+   boolqcom_bypass_quirk;
 };
 
 enum arm_smmu_context_fmt {
@@ -323,6 +325,7 @@ struct arm_smmu_cfg {
};
enum arm_smmu_cbar_type cbar;
enum arm_smmu_context_fmt   fmt;
+   boolm;
 };
 #define ARM_SMMU_INVALID_IRPTNDX   0xff
 
-- 
2.26.2

___
iommu mailing list

[PATCH v2 1/5] iommu/arm-smmu: Make all valid stream mappings BYPASS

[Bjorn Andersson]

Turn all stream mappings marked as valid into BYPASS. This allows the
platform specific implementation to configure stream mappings to match
the boot loader's configuration for e.g. display to continue to function
through the reset of the SMMU.

Tested-by: John Stultz 
Tested-by: Vinod Koul 
Suggested-by: Robin Murphy 
Signed-off-by: Bjorn Andersson 
---

Changes since v1:
- Mark arm_smmu_setup_identity() static
- Picked up tested-by tags

 drivers/iommu/arm-smmu.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 243bc4cb2705..fb85e716ae9a 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1924,6 +1924,22 @@ static int arm_smmu_device_cfg_probe(struct 
arm_smmu_device *smmu)
return 0;
 }
 
+static int arm_smmu_setup_identity(struct arm_smmu_device *smmu)
+{
+   int i;
+
+   for (i = 0; i < smmu->num_mapping_groups; i++) {
+   if (smmu->smrs[i].valid) {
+   smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
+   smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
+   smmu->s2crs[i].cbndx = 0xff;
+   smmu->s2crs[i].count++;
+   }
+   }
+
+   return 0;
+}
+
 struct arm_smmu_match_data {
enum arm_smmu_arch_version version;
enum arm_smmu_implementation model;
@@ -2181,6 +2197,10 @@ static int arm_smmu_device_probe(struct platform_device 
*pdev)
if (err)
return err;
 
+   err = arm_smmu_setup_identity(smmu);
+   if (err)
+   return err;
+
if (smmu->version == ARM_SMMU_V2) {
if (smmu->num_context_banks > smmu->num_context_irqs) {
dev_err(dev,
-- 
2.26.2

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 4/5] iommu/arm-smmu-qcom: Consistently initialize stream mappings

[Bjorn Andersson]

Firmware that traps writes to S2CR to translate BYPASS into FAULT also
ignores writes of type FAULT. As such booting with "disable_bypass" set
will result in all S2CR registers left as configured by the bootloader.

This has been seen to result in indeterministic results, as these
mappings might linger and reference context banks that Linux is
reconfiguring.

Use the fact that BYPASS writes result in FAULT type to force all stream
mappings to FAULT.

Tested-by: John Stultz 
Tested-by: Vinod Koul 
Signed-off-by: Bjorn Andersson 
---

Changes since v1:
- Fixed subject spelling mistake
- Picked up tested-by

 drivers/iommu/arm-smmu-qcom.c | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
index d95a5ee8c83c..10eb024981d1 100644
--- a/drivers/iommu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm-smmu-qcom.c
@@ -27,6 +27,7 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
 {
unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 
1);
u32 reg;
+   int i;
 
/*
 * With some firmware writes to S2CR of type FAULT are ignored, and
@@ -37,9 +38,24 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
FIELD_PREP(ARM_SMMU_S2CR_CBNDX, 
0xff) |
FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, 
S2CR_PRIVCFG_DEFAULT));
reg = arm_smmu_gr0_read(smmu, last_s2cr);
-   if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS)
+   if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS) {
smmu->qcom_bypass_quirk = true;
 
+   /*
+* With firmware ignoring writes of type FAULT, booting the
+* Linux kernel with disable_bypass disabled (i.e. "enable
+* bypass") the initialization during probe will leave mappings
+* in an inconsistent state. Avoid this by configuring all
+* S2CRs to BYPASS.
+*/
+   for (i = 0; i < smmu->num_mapping_groups; i++) {
+   smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
+   smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
+   smmu->s2crs[i].cbndx = 0xff;
+   smmu->s2crs[i].count = 0;
+   }
+   }
+
return 0;
 }
 
-- 
2.26.2

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 3/5] iommu/arm-smmu: Move SMR and S2CR definitions to header file

[Bjorn Andersson]

Expose the SMR and S2CR structs in the header file, to allow platform
specific implementations to populate/initialize the smrs and s2cr
arrays.

Tested-by: John Stultz 
Tested-by: Vinod Koul 
Signed-off-by: Bjorn Andersson 
---

Changes since v1:
- Picked up tested-by

 drivers/iommu/arm-smmu.c | 14 --
 drivers/iommu/arm-smmu.h | 15 +++
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 5d5fe6741ed4..08a650fe02e3 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -68,24 +68,10 @@ module_param(disable_bypass, bool, S_IRUGO);
 MODULE_PARM_DESC(disable_bypass,
"Disable bypass streams such that incoming transactions from devices 
that are not attached to an iommu domain will report an abort back to the 
device and will not be allowed to pass through the SMMU.");
 
-struct arm_smmu_s2cr {
-   struct iommu_group  *group;
-   int count;
-   enum arm_smmu_s2cr_type type;
-   enum arm_smmu_s2cr_privcfg  privcfg;
-   u8  cbndx;
-};
-
 #define s2cr_init_val (struct arm_smmu_s2cr){  \
.type = disable_bypass ? S2CR_TYPE_FAULT : S2CR_TYPE_BYPASS,\
 }
 
-struct arm_smmu_smr {
-   u16 mask;
-   u16 id;
-   boolvalid;
-};
-
 struct arm_smmu_cb {
u64 ttbr[2];
u32 tcr[2];
diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index a71d193073e4..bcd160d01c53 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -251,6 +251,21 @@ enum arm_smmu_implementation {
QCOM_SMMUV2,
 };
 
+struct arm_smmu_s2cr {
+   struct iommu_group  *group;
+   int count;
+   enum arm_smmu_s2cr_type type;
+   enum arm_smmu_s2cr_privcfg  privcfg;
+   u8  cbndx;
+};
+
+struct arm_smmu_smr {
+   u16 mask;
+   u16 id;
+   boolvalid;
+   boolpinned;
+};
+
 struct arm_smmu_device {
struct device   *dev;
 
-- 
2.26.2

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 0/5] iommu/arm-smmu: Support maintaining bootloader mappings

[Bjorn Andersson]

Based on previous attempts and discussions this is the latest attempt at
inheriting stream mappings set up by the bootloader, for e.g. boot splash or
efifb.

The first patch is an implementation of Robin's suggestion that we should just
mark the relevant stream mappings as BYPASS. Relying on something else to set
up the stream mappings wanted - e.g. by reading it back in platform specific
implementation code.

The series then tackles the problem seen in most versions of Qualcomm firmware,
that the hypervisor intercepts BYPASS writes and turn them into FAULTs. It does
this by allocating context banks for identity domains as well, with translation
disabled.

Lastly it amends the stream mapping initialization code to allocate a specific
identity domain that is used for any mappings inherited from the bootloader, if
above Qualcomm quirk is required.


The series has been tested and shown to allow booting SDM845, SDM850, SM8150,
SM8250 with boot splash screen setup by the bootloader. Specifically it also
allows the Lenovo Yoga C630 to boot with SMMU and efifb enabled.

Bjorn Andersson (5):
  iommu/arm-smmu: Make all valid stream mappings BYPASS
  iommu/arm-smmu: Emulate bypass by using context banks
  iommu/arm-smmu: Move SMR and S2CR definitions to header file
  iommu/arm-smmu-qcom: Consistently initialize stream mappings
  iommu/arm-smmu: Setup identity domain for boot mappings

 drivers/iommu/arm-smmu-qcom.c |  48 +
 drivers/iommu/arm-smmu.c  | 123 +-
 drivers/iommu/arm-smmu.h  |  21 ++
 3 files changed, 174 insertions(+), 18 deletions(-)

-- 
2.26.2

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] iommu/arm-smmu-v3: remove the approach of MSI polling for CMD SYNC

[Barry Song]

Before commit 587e6c10a7ce ("iommu/arm-smmu-v3: Reduce contention during
command-queue insertion"), msi polling perhaps performed better since
it could run outside the spin_lock_irqsave() while the code polling cons
reg was running in the lock.

But after the great reorganization of smmu queue, neither of these two
polling methods are running in a spinlock. And real tests show polling
cons reg via sev means smaller latency. It is probably because polling
by msi will ask hardware to write memory but sev polling depends on the
update of register only.

Using 16 threads to run netperf on hns3 100G NIC with UDP packet size
in 32768bytes and set iommu to strict, TX throughput can improve from
25227.74Mbps to 27145.59Mbps by this patch. In this case, SMMU is super
busy as hns3 sends map/unmap requests extremely frequently.

Cc: Prime Zeng 
Signed-off-by: Barry Song 
---
 drivers/iommu/arm-smmu-v3.c | 46 +
 1 file changed, 1 insertion(+), 45 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index f578677a5c41..e55282a636c8 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -964,12 +964,7 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct 
arm_smmu_cmdq_ent *ent)
cmd[1] |= FIELD_PREP(CMDQ_PRI_1_RESP, ent->pri.resp);
break;
case CMDQ_OP_CMD_SYNC:
-   if (ent->sync.msiaddr) {
-   cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, 
CMDQ_SYNC_0_CS_IRQ);
-   cmd[1] |= ent->sync.msiaddr & CMDQ_SYNC_1_MSIADDR_MASK;
-   } else {
-   cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, 
CMDQ_SYNC_0_CS_SEV);
-   }
+   cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH);
cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIATTR, 
ARM_SMMU_MEMATTR_OIWB);
break;
@@ -983,21 +978,10 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct 
arm_smmu_cmdq_ent *ent)
 static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device 
*smmu,
 u32 prod)
 {
-   struct arm_smmu_queue *q = >cmdq.q;
struct arm_smmu_cmdq_ent ent = {
.opcode = CMDQ_OP_CMD_SYNC,
};
 
-   /*
-* Beware that Hi16xx adds an extra 32 bits of goodness to its MSI
-* payload, so the write will zero the entire command on that platform.
-*/
-   if (smmu->features & ARM_SMMU_FEAT_MSI &&
-   smmu->features & ARM_SMMU_FEAT_COHERENCY) {
-   ent.sync.msiaddr = q->base_dma + Q_IDX(>llq, prod) *
-  q->ent_dwords * 8;
-   }
-
arm_smmu_cmdq_build_cmd(cmd, );
 }
 
@@ -1251,30 +1235,6 @@ static int arm_smmu_cmdq_poll_until_not_full(struct 
arm_smmu_device *smmu,
return ret;
 }
 
-/*
- * Wait until the SMMU signals a CMD_SYNC completion MSI.
- * Must be called with the cmdq lock held in some capacity.
- */
-static int __arm_smmu_cmdq_poll_until_msi(struct arm_smmu_device *smmu,
- struct arm_smmu_ll_queue *llq)
-{
-   int ret = 0;
-   struct arm_smmu_queue_poll qp;
-   struct arm_smmu_cmdq *cmdq = >cmdq;
-   u32 *cmd = (u32 *)(Q_ENT(>q, llq->prod));
-
-   queue_poll_init(smmu, );
-
-   /*
-* The MSI won't generate an event, since it's being written back
-* into the command queue.
-*/
-   qp.wfe = false;
-   smp_cond_load_relaxed(cmd, !VAL || (ret = queue_poll()));
-   llq->cons = ret ? llq->prod : queue_inc_prod_n(llq, 1);
-   return ret;
-}
-
 /*
  * Wait until the SMMU cons index passes llq->prod.
  * Must be called with the cmdq lock held in some capacity.
@@ -1332,10 +1292,6 @@ static int __arm_smmu_cmdq_poll_until_consumed(struct 
arm_smmu_device *smmu,
 static int arm_smmu_cmdq_poll_until_sync(struct arm_smmu_device *smmu,
 struct arm_smmu_ll_queue *llq)
 {
-   if (smmu->features & ARM_SMMU_FEAT_MSI &&
-   smmu->features & ARM_SMMU_FEAT_COHERENCY)
-   return __arm_smmu_cmdq_poll_until_msi(smmu, llq);
-
return __arm_smmu_cmdq_poll_until_consumed(smmu, llq);
 }
 
-- 
2.27.0


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 03/15] iommu/smmu: Report empty domain nesting info

[Auger Eric]

Hi Jean,

On 7/16/20 5:39 PM, Jean-Philippe Brucker wrote:
> On Tue, Jul 14, 2020 at 10:12:49AM +, Liu, Yi L wrote:
>>> Have you verified that this doesn't break the existing usage of
>>> DOMAIN_ATTR_NESTING in drivers/vfio/vfio_iommu_type1.c?
>>
>> I didn't have ARM machine on my hand. But I contacted with Jean
>> Philippe, he confirmed no compiling issue. I didn't see any code
>> getting DOMAIN_ATTR_NESTING attr in current drivers/vfio/vfio_iommu_type1.c.
>> What I'm adding is to call iommu_domai_get_attr(, DOMAIN_ATTR_NESTIN)
>> and won't fail if the iommu_domai_get_attr() returns 0. This patch
>> returns an empty nesting info for DOMAIN_ATTR_NESTIN and return
>> value is 0 if no error. So I guess it won't fail nesting for ARM.
> 
> I confirm that this series doesn't break the current support for
> VFIO_IOMMU_TYPE1_NESTING with an SMMUv3. That said...
> 
> If the SMMU does not support stage-2 then there is a change in behavior
> (untested): after the domain is silently switched to stage-1 by the SMMU
> driver, VFIO will now query nesting info and obtain -ENODEV. Instead of
> succeding as before, the VFIO ioctl will now fail. I believe that's a fix
> rather than a regression, it should have been like this since the
> beginning. No known userspace has been using VFIO_IOMMU_TYPE1_NESTING so
> far, so I don't think it should be a concern.
But as Yi mentioned ealier, in the current vfio code there is no
DOMAIN_ATTR_NESTING query yet. In my SMMUV3 nested stage series, I added
such a query in vfio-pci.c to detect if I need to expose a fault region
but I already test both the returned value and the output arg. So to me
there is no issue with that change.
> 
> And if userspace queries the nesting properties using the new ABI
> introduced in this patchset, it will obtain an empty struct. I think
> that's acceptable, but it may be better to avoid adding the nesting cap if
> @format is 0?
agreed

Thanks

Eric
> 
> Thanks,
> Jean
> 
>>
>> @Eric, how about your opinion? your dual-stage vSMMU support may
>> also share the vfio_iommu_type1.c code.
>>
>> Regards,
>> Yi Liu
>>
>>> Will
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v5 2/5] iommu/uapi: Add argsz for user filled data

[Jacob Pan]

As IOMMU UAPI gets extended, user data size may increase. To support
backward compatibiliy, this patch introduces a size field to each UAPI
data structures. It is *always* the responsibility for the user to fill in
the correct size. Padding fields are adjusted to ensure 8 byte alignment.

Specific scenarios for user data handling are documented in:
Documentation/userspace-api/iommu.rst

Signed-off-by: Liu Yi L 
Signed-off-by: Jacob Pan 
---
 include/uapi/linux/iommu.h | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
index e907b7091a46..d5e9014f690e 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -135,6 +135,7 @@ enum iommu_page_response_code {
 
 /**
  * struct iommu_page_response - Generic page response information
+ * @argsz: User filled size of this data
  * @version: API version of this structure
  * @flags: encodes whether the corresponding fields are valid
  * (IOMMU_FAULT_PAGE_RESPONSE_* values)
@@ -143,6 +144,7 @@ enum iommu_page_response_code {
  * @code: response code from  iommu_page_response_code
  */
 struct iommu_page_response {
+   __u32   argsz;
 #define IOMMU_PAGE_RESP_VERSION_1  1
__u32   version;
 #define IOMMU_PAGE_RESP_PASID_VALID(1 << 0)
@@ -218,6 +220,7 @@ struct iommu_inv_pasid_info {
 /**
  * struct iommu_cache_invalidate_info - First level/stage invalidation
  * information
+ * @argsz: User filled size of this data
  * @version: API version of this structure
  * @cache: bitfield that allows to select which caches to invalidate
  * @granularity: defines the lowest granularity used for the invalidation:
@@ -246,6 +249,7 @@ struct iommu_inv_pasid_info {
  * must support the used granularity.
  */
 struct iommu_cache_invalidate_info {
+   __u32   argsz;
 #define IOMMU_CACHE_INVALIDATE_INFO_VERSION_1 1
__u32   version;
 /* IOMMU paging structure cache */
@@ -255,7 +259,7 @@ struct iommu_cache_invalidate_info {
 #define IOMMU_CACHE_INV_TYPE_NR(3)
__u8cache;
__u8granularity;
-   __u8padding[2];
+   __u8padding[6];
union {
struct iommu_inv_pasid_info pasid_info;
struct iommu_inv_addr_info addr_info;
@@ -292,6 +296,7 @@ struct iommu_gpasid_bind_data_vtd {
 
 /**
  * struct iommu_gpasid_bind_data - Information about device and guest PASID 
binding
+ * @argsz: User filled size of this data
  * @version:   Version of this data structure
  * @format:PASID table entry format
  * @flags: Additional information on guest bind request
@@ -309,17 +314,18 @@ struct iommu_gpasid_bind_data_vtd {
  * PASID to host PASID based on this bind data.
  */
 struct iommu_gpasid_bind_data {
+   __u32 argsz;
 #define IOMMU_GPASID_BIND_VERSION_11
__u32 version;
 #define IOMMU_PASID_FORMAT_INTEL_VTD   1
__u32 format;
+   __u32 addr_width;
 #define IOMMU_SVA_GPASID_VAL   (1 << 0) /* guest PASID valid */
__u64 flags;
__u64 gpgd;
__u64 hpasid;
__u64 gpasid;
-   __u32 addr_width;
-   __u8  padding[12];
+   __u8  padding[8];
/* Vendor specific data */
union {
struct iommu_gpasid_bind_data_vtd vtd;
-- 
2.7.4

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v5 5/5] iommu/vt-d: Check UAPI data processed by IOMMU core

[Jacob Pan]

IOMMU generic layer already does sanity checks UAPI data for version
match and argsz range under generic information.
Remove the redundant version check from VT-d driver and check for vendor
specific data size.

Signed-off-by: Jacob Pan 
---
 drivers/iommu/intel/iommu.c | 3 +--
 drivers/iommu/intel/svm.c   | 7 +--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f3a6ca88cf95..5e80484f0537 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -5383,8 +5383,7 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain, 
struct device *dev,
int ret = 0;
u64 size = 0;
 
-   if (!inv_info || !dmar_domain ||
-   inv_info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
+   if (!inv_info || !dmar_domain)
return -EINVAL;
 
if (!dev || !dev_is_pci(dev))
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 713b3a218483..55ea11e9c0f5 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -240,8 +240,11 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, 
struct device *dev,
if (WARN_ON(!iommu) || !data)
return -EINVAL;
 
-   if (data->version != IOMMU_GPASID_BIND_VERSION_1 ||
-   data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
+   if (data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
+   return -EINVAL;
+
+   /* IOMMU core ensures argsz is more than the start of the union */
+   if (data->argsz < offsetofend(struct iommu_gpasid_bind_data, 
vendor.vtd))
return -EINVAL;
 
if (!dev_is_pci(dev))
-- 
2.7.4

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v5 4/5] iommu/uapi: Handle data and argsz filled by users

[Jacob Pan]

IOMMU UAPI data has a user filled argsz field which indicates the data
length comes with the API call. User data is not trusted, argsz must be
validated based on the current kernel data size, mandatory data size,
and feature flags.

User data may also be extended, results in possible argsz increase.
Backward compatibility is ensured based on size and flags checking.

This patch adds sanity checks in the IOMMU layer. In addition to argsz,
reserved/unused fields in padding, flags, and version are also checked.
Details are documented in Documentation/userspace-api/iommu.rst

Signed-off-by: Liu Yi L 
Signed-off-by: Jacob Pan 
---
 drivers/iommu/iommu.c | 192 --
 include/linux/iommu.h |  20 --
 2 files changed, 200 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d43120eb1dc5..fe30a940d19e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1950,36 +1950,214 @@ int iommu_attach_device(struct iommu_domain *domain, 
struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_attach_device);
 
+/*
+ * Check flags and other user privided data for valid combinations. We also
+ * make sure no reserved fields or unused flags are not set. This is to ensure
+ * not breaking userspace in the future when these fields or flags are used.
+ */
+static int iommu_check_cache_invl_data(struct iommu_cache_invalidate_info 
*info)
+{
+   u32 mask;
+
+   if (info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
+   return -EINVAL;
+
+   mask = (1 << IOMMU_CACHE_INV_TYPE_NR) - 1;
+   if (info->cache & ~mask)
+   return -EINVAL;
+
+   if (info->granularity >= IOMMU_INV_GRANU_NR)
+   return -EINVAL;
+
+   switch (info->granularity) {
+   case IOMMU_INV_GRANU_ADDR:
+   mask = IOMMU_INV_ADDR_FLAGS_PASID |
+   IOMMU_INV_ADDR_FLAGS_ARCHID |
+   IOMMU_INV_ADDR_FLAGS_LEAF;
+
+   if (info->granu.addr_info.flags & ~mask)
+   return -EINVAL;
+   break;
+   case IOMMU_INV_GRANU_PASID:
+   mask = IOMMU_INV_PASID_FLAGS_PASID |
+   IOMMU_INV_PASID_FLAGS_ARCHID;
+   if (info->granu.pasid_info.flags & ~mask)
+   return -EINVAL;
+
+   break;
+   case IOMMU_INV_GRANU_DOMAIN:
+   /* No flags to check */
+   break;
+   default:
+   return -EINVAL;
+   }
+
+   if (info->padding[0] || info->padding[1])
+   return -EINVAL;
+
+   return 0;
+}
+
 int iommu_cache_invalidate(struct iommu_domain *domain, struct device *dev,
-  struct iommu_cache_invalidate_info *inv_info)
+  void __user *uinfo)
 {
+   struct iommu_cache_invalidate_info inv_info = { 0 };
+   u32 minsz, maxsz;
+   int ret = 0;
+
if (unlikely(!domain->ops->cache_invalidate))
return -ENODEV;
 
-   return domain->ops->cache_invalidate(domain, dev, inv_info);
+   /* Current kernel data size is the max to be copied from user */
+   maxsz = sizeof(struct iommu_cache_invalidate_info);
+
+   /*
+* No new spaces can be added before the variable sized union, the
+* minimum size is the offset to the union.
+*/
+   minsz = offsetof(struct iommu_cache_invalidate_info, granu);
+
+   /* Copy minsz from user to get flags and argsz */
+   if (copy_from_user(_info, uinfo, minsz))
+   return -EFAULT;
+
+   /* Fields before variable size union is mandatory */
+   if (inv_info.argsz < minsz)
+   return -EINVAL;
+
+   /* PASID and address granu requires additional info beyond minsz */
+   if (inv_info.argsz == minsz &&
+   ((inv_info.granularity == IOMMU_INV_GRANU_PASID) ||
+   (inv_info.granularity == IOMMU_INV_GRANU_ADDR)))
+   return -EINVAL;
+   /*
+* User might be using a newer UAPI header which has a larger data
+* size, we shall support the existing flags within the current
+* size. Copy the remaining user data _after_ minsz but not more
+* than the current kernel supported size.
+*/
+   if (copy_from_user((void *)_info + minsz, uinfo + minsz,
+   min(inv_info.argsz, maxsz) - minsz))
+   return -EFAULT;
+
+   /* Now the argsz is validated, check the content */
+   ret = iommu_check_cache_invl_data(_info);
+   if (ret)
+   return ret;
+
+   return domain->ops->cache_invalidate(domain, dev, _info);
 }
 EXPORT_SYMBOL_GPL(iommu_cache_invalidate);
 
-int iommu_sva_bind_gpasid(struct iommu_domain *domain,
-  struct device *dev, struct iommu_gpasid_bind_data 
*data)
+
+static int iommu_check_bind_data(struct iommu_gpasid_bind_data *data)
 {
+   u32 mask;

[PATCH v5 1/5] docs: IOMMU user API

[Jacob Pan]

IOMMU UAPI is newly introduced to support communications between guest
virtual IOMMU and host IOMMU. There has been lots of discussions on how
it should work with VFIO UAPI and userspace in general.

This document is indended to clarify the UAPI design and usage. The
mechanics of how future extensions should be achieved are also covered
in this documentation.

Signed-off-by: Liu Yi L 
Signed-off-by: Jacob Pan 
---
 Documentation/userspace-api/iommu.rst | 339 ++
 1 file changed, 339 insertions(+)
 create mode 100644 Documentation/userspace-api/iommu.rst

diff --git a/Documentation/userspace-api/iommu.rst 
b/Documentation/userspace-api/iommu.rst
new file mode 100644
index ..efc3e1838235
--- /dev/null
+++ b/Documentation/userspace-api/iommu.rst
@@ -0,0 +1,339 @@
+.. SPDX-License-Identifier: GPL-2.0
+.. iommu:
+
+=
+IOMMU Userspace API
+=
+
+IOMMU UAPI is used for virtualization cases where communications are
+needed between physical and virtual IOMMU drivers. For native
+usage, IOMMU is a system device which does not need to communicate
+with user space directly.
+
+The primary use cases are guest Shared Virtual Address (SVA) and
+guest IO virtual address (IOVA), wherein a virtual IOMMU (vIOMMU) is
+required to communicate with the physical IOMMU in the host.
+
+.. contents:: :local:
+
+Functionalities
+===
+Communications of user and kernel involve both directions. The
+supported user-kernel APIs are as follows:
+
+1. Alloc/Free PASID
+2. Bind/unbind guest PASID (e.g. Intel VT-d)
+3. Bind/unbind guest PASID table (e.g. ARM sMMU)
+4. Invalidate IOMMU caches
+5. Service IO page faults (page request and response)
+
+Requirements
+
+The IOMMU UAPIs are generic and extensible to meet the following
+requirements:
+
+1. Emulated and para-virtualised vIOMMUs
+2. Multiple vendors (Intel VT-d, ARM sMMU, etc.)
+3. Extensions to the UAPI shall not break existing user space
+
+Interfaces
+==
+Although the data structures defined in IOMMU UAPI are self-contained,
+there is no user API functions introduced. Instead, IOMMU UAPI is
+designed to work with existing user driver frameworks such as VFIO.
+
+Extension Rules & Precautions
+-
+When IOMMU UAPI gets extended, the data structures can *only* be
+modified in two ways:
+
+1. Adding new fields by re-purposing the padding[] field. No size change.
+2. Adding new union members at the end. May increase in size.
+
+No new fields can be added *after* the variable sized union in that it
+will break backward compatibility when offset moves. In both cases, a
+new flag must be accompanied with a new field such that the IOMMU
+driver can process the data based on the new flag. Version field is
+only reserved for the unlikely event of UAPI upgrade at its entirety.
+
+It's *always* the caller's responsibility to indicate the size of the
+structure passed by setting argsz appropriately.
+Though at the same time, argsz is user provided data which is not
+trusted. The argsz field allows the user to indicate how much data
+they're providing, it's still the kernel's responsibility to validate
+whether it's correct and sufficient for the requested operation.
+
+Compatibility Checking
+--
+When IOMMU UAPI extension results in size increase, IOMMU UAPI core
+and vendor driver shall handle the following cases:
+
+1. User and kernel has exact size match
+2. An older user with older kernel header (smaller UAPI size) running on a
+   newer kernel (larger UAPI size)
+3. A newer user with newer kernel header (larger UAPI size) running
+   on an older kernel.
+4. A malicious/misbehaving user pass illegal/invalid size but within
+   range. The data may contain garbage.
+
+Feature Checking
+
+While launching a guest with vIOMMU, it is important to ensure that host
+can support the UAPI data structures to be used for vIOMMU-pIOMMU
+communications. Without upfront compatibility checking, future faults
+are difficult to report even in normal conditions. For example, TLB
+invalidations should always succeed. There is no architectural way to
+report back to the vIOMMU if the UAPI data is incompatible. If that
+happens, in order to protect IOMMU iosolation guarantee, we have to
+resort to not giving completion status in vIOMMU. This may result in
+VM hang.
+
+For this reason the following IOMMU UAPIs cannot fail:
+
+1. Free PASID
+2. Unbind guest PASID
+3. Unbind guest PASID table (SMMU)
+4. Cache invalidate
+
+User applications such as QEMU are expected to import kernel UAPI
+headers. Backward compatibility is supported per feature flags.
+For example, an older QEMU (with older kernel header) can run on newer
+kernel. Newer QEMU (with new kernel header) may refuse to initialize
+on an older kernel if new feature flags are not supported by older
+kernel. Simply recompiling existing code with newer 

[PATCH v5 0/5] IOMMU user API enhancement

[Jacob Pan]

IOMMU user API header was introduced to support nested DMA translation and
related fault handling. The current UAPI data structures consist of three
areas that cover the interactions between host kernel and guest:
 - fault handling
 - cache invalidation
 - bind guest page tables, i.e. guest PASID

Future extensions are likely to support more architectures and vIOMMU features.

In the previous discussion, using user-filled data size and feature flags is
made a preferred approach over a unified version number.
https://lkml.org/lkml/2020/1/29/45

In addition to introduce argsz field to data structures, this patchset is also
trying to document the UAPI design, usage, and extension rules. VT-d driver
changes to utilize the new argsz field is included, VFIO usage is to follow.

Thanks,

Jacob

Changeog:
v5
- Addjusted paddings in UAPI data to be 8 byte aligned
- Do not clobber argsz in IOMMU core before passing on to vendor driver
- Removed pr_warn_ for invalid UAPI data check, just return -EINVAL
- Clarified VFIO responsibility in UAPI data handling
- Use iommu_uapi prefix to differentiate APIs has in-kernel caller
- Added comment for unchecked flags of invalidation granularity
- Added example in doc to show vendor data checking

v4
- Added checks of UAPI data for reserved fields, version, and flags.
- Removed version check from vendor driver (vt-d)
- Relaxed argsz check to match the UAPI struct size instead of variable
  union size
- Updated documentation

v3:
- Rewrote backward compatibility rule to support existing code
  re-compiled with newer kernel UAPI header that runs on older
  kernel. Based on review comment from Alex W.
  https://lore.kernel.org/linux-iommu/20200611094741.6d118...@w520.home/
- Take user pointer directly in UAPI functions. Perform argsz check
  and copy_from_user() in IOMMU driver. Eliminate the need for
  VFIO or other upper layer to parse IOMMU data.
- Create wrapper function for in-kernel users of UAPI functions
v2:
- Removed unified API version and helper
- Introduced argsz for each UAPI data
- Introduced UAPI doc


Jacob Pan (5):
  docs: IOMMU user API
  iommu/uapi: Add argsz for user filled data
  iommu/uapi: Use named union for user data
  iommu/uapi: Handle data and argsz filled by users
  iommu/vt-d: Check UAPI data processed by IOMMU core

 Documentation/userspace-api/iommu.rst | 338 ++
 drivers/iommu/intel/iommu.c   |  27 ++-
 drivers/iommu/intel/svm.c |   9 +-
 drivers/iommu/iommu.c | 192 ++-
 include/linux/iommu.h |  20 +-
 include/uapi/linux/iommu.h|  16 +-
 6 files changed, 568 insertions(+), 34 deletions(-)
 create mode 100644 Documentation/userspace-api/iommu.rst

-- 
2.7.4

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v5 3/5] iommu/uapi: Use named union for user data

[Jacob Pan]

IOMMU UAPI data size is filled by the user space which must be validated
by ther kernel. To ensure backward compatibility, user data can only be
extended by either re-purpose padding bytes or extend the variable sized
union at the end. No size change is allowed before the union. Therefore,
the minimum size is the offset of the union.

To use offsetof() on the union, we must make it named.

Link: https://lore.kernel.org/linux-iommu/20200611145518.0c281...@x1.home/
Signed-off-by: Jacob Pan 
Reviewed-by: Lu Baolu 
---
 drivers/iommu/intel/iommu.c | 24 
 drivers/iommu/intel/svm.c   |  2 +-
 include/uapi/linux/iommu.h  |  4 ++--
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 6ad8b6f20235..f3a6ca88cf95 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -5409,8 +5409,8 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain, 
struct device *dev,
 
/* Size is only valid in address selective invalidation */
if (inv_info->granularity == IOMMU_INV_GRANU_ADDR)
-   size = to_vtd_size(inv_info->addr_info.granule_size,
-  inv_info->addr_info.nb_granules);
+   size = to_vtd_size(inv_info->granu.addr_info.granule_size,
+  inv_info->granu.addr_info.nb_granules);
 
for_each_set_bit(cache_type,
 (unsigned long *)_info->cache,
@@ -5432,20 +5432,20 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain, 
struct device *dev,
 * granularity.
 */
if (inv_info->granularity == IOMMU_INV_GRANU_PASID &&
-   (inv_info->pasid_info.flags & IOMMU_INV_PASID_FLAGS_PASID))
-   pasid = inv_info->pasid_info.pasid;
+   (inv_info->granu.pasid_info.flags & 
IOMMU_INV_PASID_FLAGS_PASID))
+   pasid = inv_info->granu.pasid_info.pasid;
else if (inv_info->granularity == IOMMU_INV_GRANU_ADDR &&
-(inv_info->addr_info.flags & 
IOMMU_INV_ADDR_FLAGS_PASID))
-   pasid = inv_info->addr_info.pasid;
+(inv_info->granu.addr_info.flags & 
IOMMU_INV_ADDR_FLAGS_PASID))
+   pasid = inv_info->granu.addr_info.pasid;
 
switch (BIT(cache_type)) {
case IOMMU_CACHE_INV_TYPE_IOTLB:
/* HW will ignore LSB bits based on address mask */
if (inv_info->granularity == IOMMU_INV_GRANU_ADDR &&
size &&
-   (inv_info->addr_info.addr & ((BIT(VTD_PAGE_SHIFT + 
size)) - 1))) {
+   (inv_info->granu.addr_info.addr & 
((BIT(VTD_PAGE_SHIFT + size)) - 1))) {
WARN_ONCE(1, "Address out of range, 0x%llx, 
size order %llu\n",
- inv_info->addr_info.addr, size);
+ inv_info->granu.addr_info.addr, size);
}
 
/*
@@ -5453,9 +5453,9 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain, 
struct device *dev,
 * We use npages = -1 to indicate that.
 */
qi_flush_piotlb(iommu, did, pasid,
-   mm_to_dma_pfn(inv_info->addr_info.addr),
+   
mm_to_dma_pfn(inv_info->granu.addr_info.addr),
(granu == QI_GRAN_NONG_PASID) ? -1 : 1 
<< size,
-   inv_info->addr_info.flags & 
IOMMU_INV_ADDR_FLAGS_LEAF);
+   inv_info->granu.addr_info.flags & 
IOMMU_INV_ADDR_FLAGS_LEAF);
 
if (!info->ats_enabled)
break;
@@ -5476,13 +5476,13 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain, 
struct device *dev,
size = 64 - VTD_PAGE_SHIFT;
addr = 0;
} else if (inv_info->granularity == 
IOMMU_INV_GRANU_ADDR)
-   addr = inv_info->addr_info.addr;
+   addr = inv_info->granu.addr_info.addr;
 
if (info->ats_enabled)
qi_flush_dev_iotlb_pasid(iommu, sid,
info->pfsid, pasid,
info->ats_qdep,
-   inv_info->addr_info.addr,
+   inv_info->granu.addr_info.addr,
size);
else
pr_warn_ratelimited("Passdown device IOTLB 
flush w/o ATS!\n");
diff --git 

Re: [PATCH] dma-pool: Only allocate from CMA when in same memory zone

[Nicolas Saenz Julienne]

Hi Chritoph,

On Fri, 2020-07-10 at 16:10 +0200, Nicolas Saenz Julienne wrote:
> There is no guarantee to CMA's placement, so allocating a zone specific
> atomic pool from CMA might return memory from a completely different
> memory zone. To get around this double check CMA's placement before
> allocating from it.
> 
> Fixes: c84dc6e68a1d ("dma-pool: add additional coherent pools to map to gfp
> mask")
> Reported-by: Jeremy Linton 
> Signed-off-by: Nicolas Saenz Julienne 
> ---
> 
> This is a code intensive alternative to "dma-pool: Do not allocate pool
> memory from CMA"[1].

I see you applied "dma-pool: Do not allocate pool memory from CMA" on your
tree. Do you want me to send a v2 of this patch taking that into account
targeting v5.9? or you'd rather just follow another approach?

Regards,
Nicolas

> 
> [1] https://lkml.org/lkml/2020/7/8/1108
> 
>  kernel/dma/pool.c | 36 +++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
> index 8cfa01243ed2..ccf3eeb77e00 100644
> --- a/kernel/dma/pool.c
> +++ b/kernel/dma/pool.c
> @@ -3,6 +3,7 @@
>   * Copyright (C) 2012 ARM Ltd.
>   * Copyright (C) 2020 Google LLC
>   */
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -56,6 +57,39 @@ static void dma_atomic_pool_size_add(gfp_t gfp, size_t
> size)
>   pool_size_kernel += size;
>  }
>  
> +static bool cma_in_zone(gfp_t gfp)
> +{
> + u64 zone_dma_end, zone_dma32_end;
> + phys_addr_t base, end;
> + unsigned long size;
> + struct cma *cma;
> +
> + cma = dev_get_cma_area(NULL);
> + if (!cma)
> + return false;
> +
> + size = cma_get_size(cma);
> + if (!size)
> + return false;
> + base = cma_get_base(cma) - memblock_start_of_DRAM();
> + end = base + size - 1;
> +
> + zone_dma_end = IS_ENABLED(CONFIG_ZONE_DMA) ? DMA_BIT_MASK(zone_dma_bits)
> : 0;
> + zone_dma32_end = IS_ENABLED(CONFIG_ZONE_DMA32) ? DMA_BIT_MASK(32) : 0;
> +
> + /* CMA can't cross zone boundaries, see cma_activate_area() */
> + if (IS_ENABLED(CONFIG_ZONE_DMA) && gfp & GFP_DMA &&
> +end <= zone_dma_end)
> + return true;
> + else if (IS_ENABLED(CONFIG_ZONE_DMA32) && gfp & GFP_DMA32 &&
> + base > zone_dma_end && end <= zone_dma32_end)
> + return true;
> + else if (base > zone_dma32_end)
> + return true;
> +
> + return false;
> +}
> +
>  static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
> gfp_t gfp)
>  {
> @@ -70,7 +104,7 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t
> pool_size,
>   do {
>   pool_size = 1 << (PAGE_SHIFT + order);
>  
> - if (dev_get_cma_area(NULL))
> + if (cma_in_zone(gfp))
>   page = dma_alloc_from_contiguous(NULL, 1 << order,
>order, false);
>   else



signature.asc
Description: This is a digitally signed message part
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/4] iommu/arm-smmu-v3: Improve cmdq lock efficiency

[John Garry]

Perhaps a silly question (I'm too engrossed in PMU world ATM to get properly
back up to speed on this), but couldn't this be done without cmpxchg anyway?
Instinctively it feels like instead of maintaining a literal software copy
of the prod value, we could resolve the "claim my slot in the queue" part
with atomic_fetch_add on a free-running 32-bit "pseudo-prod" index, then
whoever updates the hardware deals with the truncation and wrap bit to
convert it to an actual register value.


Maybe, but it's easier said than done. The hard part is figuring how that
you have space and there's no way I'm touching that logic without a way to
test this.

I'm also not keen to complicate the code because of systems that don't scale
well with contended CAS [1]. If you put down loads of cores, you need an
interconnect/coherence protocol that can handle it.


JFYI, I added some debug to the driver to get the cmpxchg() attempt rate 
while running a testharness (below):


cpusrate
2   1.1
4   1.1
8   1.3
16  3.6
32  8.1
64  12.6
96  14.7

Ideal rate is 1. So it's not crazy high for many CPUs, but still 
drifting away from 1.


John



Will

[1] 
https://lore.kernel.org/lkml/20190607072652.GA5522@hc/T/#m0d00f107c29223045933292a8b5b90d2ca9b7e5c
.



//copied from Barry, thanks :)

static int ways = 64;
module_param(ways, int, S_IRUGO);

static int seconds = 20;
module_param(seconds, int, S_IRUGO);

int mappings[NR_CPUS];
struct semaphore sem[NR_CPUS];


#define COMPLETIONS_SIZE 50

static noinline dma_addr_t test_mapsingle(struct device *dev, void *buf, 
int size)

{
dma_addr_t dma_addr = dma_map_single(dev, buf, size, DMA_TO_DEVICE);
return dma_addr;
}

static noinline void test_unmapsingle(struct device *dev, void *buf, int 
size, dma_addr_t dma_addr)

{
dma_unmap_single(dev, dma_addr, size, DMA_TO_DEVICE);
}

static noinline void test_memcpy(void *out, void *in, int size)
{
memcpy(out, in, size);
}

//just a hack to get a dev h behind a SMMU
extern struct device *hisi_dev;

static int testthread(void *data)
{
unsigned long stop = jiffies +seconds*HZ;
struct device *dev = hisi_dev;
char *inputs[COMPLETIONS_SIZE];
char *outputs[COMPLETIONS_SIZE];
dma_addr_t dma_addr[COMPLETIONS_SIZE];
int i, cpu = smp_processor_id();
struct semaphore *sem = data;

for (i = 0; i < COMPLETIONS_SIZE; i++) {
inputs[i] = kzalloc(4096, GFP_KERNEL);
if (!inputs[i])
return -ENOMEM;
}

for (i = 0; i < COMPLETIONS_SIZE; i++) {
outputs[i] = kzalloc(4096, GFP_KERNEL);
if (!outputs[i])
return -ENOMEM;
}

while (time_before(jiffies, stop)) {
for (i = 0; i < COMPLETIONS_SIZE; i++) {
dma_addr[i] = test_mapsingle(dev, inputs[i], 4096);
test_memcpy(outputs[i], inputs[i], 4096);
}
for (i = 0; i < COMPLETIONS_SIZE; i++) {
test_unmapsingle(dev, inputs[i], 4096, dma_addr[i]);
}
mappings[cpu] += COMPLETIONS_SIZE;
}

for (i = 0; i < COMPLETIONS_SIZE; i++) {
kfree(outputs[i]);
kfree(inputs[i]);
}

up(sem);

return 0;
}

void smmu_test_core(int cpus)
{
struct task_struct *tsk;
int i;
int total_mappings = 0;

ways = cpus;

for(i=0;iprintk(KERN_ERR "finished total_mappings=%d (per way=%d) (rate=%d 
per second per cpu) ways=%d\n",
total_mappings, total_mappings / ways, total_mappings / (seconds* 
ways), ways);


}
EXPORT_SYMBOL(smmu_test_core);


static int __init test_init(void)
{
int i;

for(i=0;ihttps://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-debug: use named initializers for dir2name

[Robin Murphy]

On 2020-07-16 16:01, Christoph Hellwig wrote:

Make dir2name a little more readable and maintainable by using
named initializers.

Signed-off-by: Christoph Hellwig 
---
  kernel/dma/debug.c | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 36c962a86bf25d..41e720c3ab20c5 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -144,8 +144,12 @@ static const char *type2name[] = {
[dma_debug_resource] = "resource",
  };
  
-static const char *dir2name[4] = { "DMA_BIDIRECTIONAL", "DMA_TO_DEVICE",

-  "DMA_FROM_DEVICE", "DMA_NONE" };
+static const char *dir2name[4] = {


Nit: I think you can probably drop the explicit array size here.

Otherwise, very welcome clarity!

Reviewed-by: Robin Murphy 


+   [DMA_BIDIRECTIONAL] = "DMA_BIDIRECTIONAL",
+   [DMA_TO_DEVICE] = "DMA_TO_DEVICE",
+   [DMA_FROM_DEVICE]   = "DMA_FROM_DEVICE",
+   [DMA_NONE]  = "DMA_NONE",
+};
  
  /*

   * The access to some variables in this macro is racy. We can't use atomic_t


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v8 07/12] iommu/arm-smmu-v3: Share process page tables

[Jean-Philippe Brucker]

On Mon, Jul 13, 2020 at 09:22:37PM +0100, Will Deacon wrote:
> > +static struct arm_smmu_ctx_desc *arm_smmu_share_asid(u16 asid)
> > +{
> > +   struct arm_smmu_ctx_desc *cd;
> >  
> > -   xa_erase(_xa, cd->asid);
> > +   cd = xa_load(_xa, asid);
> > +   if (!cd)
> > +   return NULL;
> > +
> > +   if (cd->mm) {
> > +   /* All devices bound to this mm use the same cd struct. */
> > +   refcount_inc(>refs);
> > +   return cd;
> > +   }
> 
> How do you handle racing against a concurrent arm_smmu_free_asid() here?

Patch 8 adds an asid_lock to deal with this, but it should be introduced
in this patch. There is a potential use-after-free here, if
arm_smmu_domain_free() runs concurrently.

> 
> > +__maybe_unused
> > +static struct arm_smmu_ctx_desc *arm_smmu_alloc_shared_cd(struct mm_struct 
> > *mm)
> > +{
> > +   u16 asid;
> > +   int ret = 0;
> > +   u64 tcr, par, reg;
> > +   struct arm_smmu_ctx_desc *cd;
> > +   struct arm_smmu_ctx_desc *old_cd = NULL;
> > +
> > +   lockdep_assert_held(_lock);
> 
> Please don't bother with these for static functions (but I can see the
> value in having them for functions with external callers).
> 
> > +
> > +   asid = mm_context_get(mm);
> > +   if (!asid)
> > +   return ERR_PTR(-ESRCH);
> > +
> > +   cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> > +   if (!cd) {
> > +   ret = -ENOMEM;
> > +   goto err_put_context;
> > +   }
> > +
> > +   arm_smmu_init_cd(cd);
> > +
> > +   old_cd = arm_smmu_share_asid(asid);
> > +   if (IS_ERR(old_cd)) {
> > +   ret = PTR_ERR(old_cd);
> > +   goto err_free_cd;
> > +   } else if (old_cd) {
> 
> Don't need the 'else'
> 
> > +   if (WARN_ON(old_cd->mm != mm)) {
> > +   ret = -EINVAL;
> > +   goto err_free_cd;
> > +   }
> > +   kfree(cd);
> > +   mm_context_put(mm);
> > +   return old_cd;
> 
> This is a bit messy. Can you consolidate the return path so that ret is a
> pointer and you have an 'int err', e.g.:
> 
>   return err < 0 ? ERR_PTR(err) : ret;

Sure, I think it looks a little nicer this way

Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v8 04/12] arm64: mm: Pin down ASIDs for sharing mm with devices

[Jean-Philippe Brucker]

Hi,

Thanks for reviewing, I think these 3 or 4 patches are the trickiest in
the whole SVA series

On Mon, Jul 13, 2020 at 04:46:23PM +0100, Will Deacon wrote:
> On Thu, Jun 18, 2020 at 05:51:17PM +0200, Jean-Philippe Brucker wrote:
> > diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
> > index 68140fdd89d6b..bbdd291e31d59 100644
> > --- a/arch/arm64/include/asm/mmu.h
> > +++ b/arch/arm64/include/asm/mmu.h
> > @@ -19,6 +19,7 @@
> >  
> >  typedef struct {
> > atomic64_t  id;
> > +   unsigned long   pinned;
> 
> bool?

It's a refcount. I'll change it to refcount_t because it looks nicer
overall, even though it doesn't need to be atomic.

> >  static void set_kpti_asid_bits(void)
> >  {
> > +   unsigned int k;
> > +   u8 *dst = (u8 *)asid_map;
> > +   u8 *src = (u8 *)pinned_asid_map;
> > unsigned int len = BITS_TO_LONGS(NUM_USER_ASIDS) * sizeof(unsigned 
> > long);
> > /*
> >  * In case of KPTI kernel/user ASIDs are allocated in
> > @@ -81,7 +88,8 @@ static void set_kpti_asid_bits(void)
> >  * is set, then the ASID will map only userspace. Thus
> >  * mark even as reserved for kernel.
> >  */
> > -   memset(asid_map, 0xaa, len);
> > +   for (k = 0; k < len; k++)
> > +   dst[k] = src[k] | 0xaa;
> 
> Can you use __bitmap_replace() here? I think it would be clearer to use the
> bitmap API wherever possible, since casting 'unsigned long *' to 'u8 *'
> just makes me worry about endianness issues (although in this case I don't
> hink it's a problem).

I can do better actually: initialize pinned_asid_map with the kernel ASID
bits at boot and just copy the bitmap on rollover.

> > +unsigned long mm_context_get(struct mm_struct *mm)
> > +{
> > +   unsigned long flags;
> > +   u64 asid;
> > +
> > +   raw_spin_lock_irqsave(_asid_lock, flags);
> > +
> > +   asid = atomic64_read(>context.id);
> > +
> > +   if (mm->context.pinned) {
> > +   mm->context.pinned++;
> > +   asid &= ~ASID_MASK;
> > +   goto out_unlock;
> > +   }
> > +
> > +   if (nr_pinned_asids >= max_pinned_asids) {
> > +   asid = 0;
> > +   goto out_unlock;
> > +   }
> > +
> > +   if (!asid_gen_match(asid)) {
> > +   /*
> > +* We went through one or more rollover since that ASID was
> > +* used. Ensure that it is still valid, or generate a new one.
> > +*/
> > +   asid = new_context(mm);
> > +   atomic64_set(>context.id, asid);
> > +   }
> > +
> > +   asid &= ~ASID_MASK;
> > +
> > +   nr_pinned_asids++;
> > +   __set_bit(asid2idx(asid), pinned_asid_map);
> > +   mm->context.pinned++;
> > +
> > +out_unlock:
> > +   raw_spin_unlock_irqrestore(_asid_lock, flags);
> 
> Maybe stick the & ~ASID_MASK here so it's easier to read?
> 
> > +   /* Set the equivalent of USER_ASID_BIT */
> > +   if (asid && IS_ENABLED(CONFIG_UNMAP_KERNEL_AT_EL0))
> > +   asid |= 1;
> > +
> > +   return asid;
> > +}
> > +EXPORT_SYMBOL_GPL(mm_context_get);
> 
> That's quite a generic symbol name to export... maybe throw 'arm64_' in
> front of it?
> 
> > +
> > +void mm_context_put(struct mm_struct *mm)
> > +{
> > +   unsigned long flags;
> > +   u64 asid = atomic64_read(>context.id) & ~ASID_MASK;
> 
> I don't think you need the masking here.

Right, asid2idx() takes care of it.

> 
> > +   raw_spin_lock_irqsave(_asid_lock, flags);
> > +
> > +   if (--mm->context.pinned == 0) {
> > +   __clear_bit(asid2idx(asid), pinned_asid_map);
> > +   nr_pinned_asids--;
> > +   }
> > +
> > +   raw_spin_unlock_irqrestore(_asid_lock, flags);
> > +}
> > +EXPORT_SYMBOL_GPL(mm_context_put);
> 
> Same naming comment here.
> 
> >  /* Errata workaround post TTBRx_EL1 update. */
> >  asmlinkage void post_ttbr_update_workaround(void)
> >  {
> > @@ -303,6 +381,13 @@ static int asids_update_limit(void)
> > WARN_ON(num_available_asids - 1 <= num_possible_cpus());
> > pr_info("ASID allocator initialised with %lu entries\n",
> > num_available_asids);
> > +
> > +   /*
> > +* We assume that an ASID is always available after a rollover. This
> > +* means that even if all CPUs have a reserved ASID, there still is at
> > +* least one slot available in the asid map.
> > +*/
> > +   max_pinned_asids = num_available_asids - num_possible_cpus() - 2;
> 
> Is it worth checking that assumption, rather than setting max_pinned_asids
> to a massive value?

The comment isn't right, it should be something like:

"There must always be an ASID available after a rollover. Ensure
that, even if all CPUs have a reserved ASID and the maximum number
of ASIDs are pinned, there still is at least one empty slot in the
ASID map."

I do wonder if we should reduce max_pinned_asids, because the system will
probably become unusable if it gets close to a single available ASID.  But
I think we'll need to introduce higher-level controls for SVA such as
cgroups to prevent users from pinning too many 

Re: [PATCH v5 03/15] iommu/smmu: Report empty domain nesting info

[Jean-Philippe Brucker]

On Tue, Jul 14, 2020 at 10:12:49AM +, Liu, Yi L wrote:
> > Have you verified that this doesn't break the existing usage of
> > DOMAIN_ATTR_NESTING in drivers/vfio/vfio_iommu_type1.c?
> 
> I didn't have ARM machine on my hand. But I contacted with Jean
> Philippe, he confirmed no compiling issue. I didn't see any code
> getting DOMAIN_ATTR_NESTING attr in current drivers/vfio/vfio_iommu_type1.c.
> What I'm adding is to call iommu_domai_get_attr(, DOMAIN_ATTR_NESTIN)
> and won't fail if the iommu_domai_get_attr() returns 0. This patch
> returns an empty nesting info for DOMAIN_ATTR_NESTIN and return
> value is 0 if no error. So I guess it won't fail nesting for ARM.

I confirm that this series doesn't break the current support for
VFIO_IOMMU_TYPE1_NESTING with an SMMUv3. That said...

If the SMMU does not support stage-2 then there is a change in behavior
(untested): after the domain is silently switched to stage-1 by the SMMU
driver, VFIO will now query nesting info and obtain -ENODEV. Instead of
succeding as before, the VFIO ioctl will now fail. I believe that's a fix
rather than a regression, it should have been like this since the
beginning. No known userspace has been using VFIO_IOMMU_TYPE1_NESTING so
far, so I don't think it should be a concern.

And if userspace queries the nesting properties using the new ABI
introduced in this patchset, it will obtain an empty struct. I think
that's acceptable, but it may be better to avoid adding the nesting cap if
@format is 0?

Thanks,
Jean

> 
> @Eric, how about your opinion? your dual-stage vSMMU support may
> also share the vfio_iommu_type1.c code.
> 
> Regards,
> Yi Liu
> 
> > Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [Freedreno] [PATCH v9 4/7] iommu/arm-smmu: Add a pointer to the attached device to smmu_domain

[Jordan Crouse]

On Thu, Jul 16, 2020 at 09:50:53AM +0100, Will Deacon wrote:
> On Mon, Jul 13, 2020 at 11:19:17AM -0600, Jordan Crouse wrote:
> > On Mon, Jul 13, 2020 at 04:09:02PM +0100, Will Deacon wrote:
> > > On Fri, Jun 26, 2020 at 02:00:38PM -0600, Jordan Crouse wrote:
> > > > diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> > > > index 5f2de20e883b..d33cfe26b2f5 100644
> > > > --- a/drivers/iommu/arm-smmu.h
> > > > +++ b/drivers/iommu/arm-smmu.h
> > > > @@ -345,6 +345,7 @@ struct arm_smmu_domain {
> > > > struct mutexinit_mutex; /* Protects smmu 
> > > > pointer */
> > > > spinlock_t  cb_lock; /* Serialises ATS1* 
> > > > ops and TLB syncs */
> > > > struct iommu_domain domain;
> > > > +   struct device   *dev;   /* Device attached to 
> > > > this domain */
> > > 
> > > This really doesn't feel right to me -- you can generally have multiple
> > > devices attached to a domain and they can come and go without the domain
> > > being destroyed. Perhaps you could instead identify the GPU during
> > > cfg_probe() and squirrel that information away somewhere?
> > 
> > I need some help here. The SMMU device (qcom,adreno-smmu) will have at 
> > least two
> > stream ids from two different platform devices (GPU and GMU) and I need to
> > configure split-pagetable and stall/terminate differently on the two 
> > domains.
> 
> Hmm. How does the GPU driver know which context bank is assigned to the GPU
> and which one is assigned to the GMU? I assume it needs this information so
> that it can play its nasty tricks with the TTBR registers?
> 
> I ask because if we need to guarantee stability of the context-bank
> assignment, then you could match on that in the ->init_context() callback,
> but now I worry that it currently works by luck :/
 
Its more like "educated" luck. If we assign the domains in the right order we
know that the GPU will be on context bank 0 but you are entirely right that if
everything doesn't go exactly the way we need things go badly.

Some targets do have the ability to reprogram which context bank is used but
that would require a domain attribute from the SMMU driver to communicate that
information back to the GPU driver.

> Do we need to add an extra callback to allocate the context bank?

That seems like a reasonable option. That seems like it would work with legacy
targets and rely less on luck.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] dma-debug: use named initializers for dir2name

[Christoph Hellwig]

Make dir2name a little more readable and maintainable by using
named initializers.

Signed-off-by: Christoph Hellwig 
---
 kernel/dma/debug.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 36c962a86bf25d..41e720c3ab20c5 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -144,8 +144,12 @@ static const char *type2name[] = {
[dma_debug_resource] = "resource",
 };
 
-static const char *dir2name[4] = { "DMA_BIDIRECTIONAL", "DMA_TO_DEVICE",
-  "DMA_FROM_DEVICE", "DMA_NONE" };
+static const char *dir2name[4] = {
+   [DMA_BIDIRECTIONAL] = "DMA_BIDIRECTIONAL",
+   [DMA_TO_DEVICE] = "DMA_TO_DEVICE",
+   [DMA_FROM_DEVICE]   = "DMA_FROM_DEVICE",
+   [DMA_NONE]  = "DMA_NONE",
+};
 
 /*
  * The access to some variables in this macro is racy. We can't use atomic_t
-- 
2.27.0

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-coherent: switch to bitmap_zalloc() in dma_init_coherent_memory()

[Christoph Hellwig]

Applied to nvme-5.9.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [Freedreno] [PATCH v9 4/7] iommu/arm-smmu: Add a pointer to the attached device to smmu_domain

[Rob Clark]

On Thu, Jul 16, 2020 at 1:51 AM Will Deacon  wrote:
>
> On Mon, Jul 13, 2020 at 11:19:17AM -0600, Jordan Crouse wrote:
> > On Mon, Jul 13, 2020 at 04:09:02PM +0100, Will Deacon wrote:
> > > On Fri, Jun 26, 2020 at 02:00:38PM -0600, Jordan Crouse wrote:
> > > > diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> > > > index 5f2de20e883b..d33cfe26b2f5 100644
> > > > --- a/drivers/iommu/arm-smmu.h
> > > > +++ b/drivers/iommu/arm-smmu.h
> > > > @@ -345,6 +345,7 @@ struct arm_smmu_domain {
> > > >   struct mutexinit_mutex; /* Protects smmu pointer 
> > > > */
> > > >   spinlock_t  cb_lock; /* Serialises ATS1* ops and 
> > > > TLB syncs */
> > > >   struct iommu_domain domain;
> > > > + struct device   *dev;   /* Device attached to this 
> > > > domain */
> > >
> > > This really doesn't feel right to me -- you can generally have multiple
> > > devices attached to a domain and they can come and go without the domain
> > > being destroyed. Perhaps you could instead identify the GPU during
> > > cfg_probe() and squirrel that information away somewhere?
> >
> > I need some help here. The SMMU device (qcom,adreno-smmu) will have at 
> > least two
> > stream ids from two different platform devices (GPU and GMU) and I need to
> > configure split-pagetable and stall/terminate differently on the two 
> > domains.
>
> Hmm. How does the GPU driver know which context bank is assigned to the GPU
> and which one is assigned to the GMU? I assume it needs this information so
> that it can play its nasty tricks with the TTBR registers?
>
> I ask because if we need to guarantee stability of the context-bank
> assignment, then you could match on that in the ->init_context() callback,
> but now I worry that it currently works by luck :/

Yeah, it is basically by luck right now.. some sort of impl callback
which was passed the dev into impl->init_context() might do the trick
(ie. then the impl could match on compat string)

BR,
-R

> Do we need to add an extra callback to allocate the context bank?
>
> Will
> ___
> Freedreno mailing list
> freedr...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/4] iommu/arm-smmu-v3: Improve cmdq lock efficiency

[John Garry]

On 16/07/2020 11:28, Will Deacon wrote:

On Thu, Jul 16, 2020 at 11:22:33AM +0100, Will Deacon wrote:

On Thu, Jul 16, 2020 at 11:19:41AM +0100, Will Deacon wrote:

On Tue, Jun 23, 2020 at 01:28:36AM +0800, John Garry wrote:

As mentioned in [0], the CPU may consume many cycles processing
arm_smmu_cmdq_issue_cmdlist(). One issue we find is the cmpxchg() loop to
get space on the queue takes approx 25% of the cycles for this function.

This series removes that cmpxchg().


How about something much simpler like the diff below?


Ah, scratch that, I don't drop the lock if we fail the cas with it held.
Let me hack it some more (I have no hardware so I can only build-test this).


Right, second attempt...

Will


Unfortunately that hangs my machine during boot:

[10.902893] 00:01: ttyS0 at MMIO 0x3f3f8 (irq = 6, base_baud = 
115200) is a 16550A

[10.912048] SuperH (H)SCI(F) driver initialized
[10.916811] msm_serial: driver initialized
[10.921371] arm-smmu-v3 arm-smmu-v3.0.auto: option mask 0x0
[10.926946] arm-smmu-v3 arm-smmu-v3.0.auto: ias 48-bit, oas 48-bit 
(features 0x0fef)

[10.935374] arm-smmu-v3 arm-smmu-v3.0.auto: allocated 65536 entries for cmdq
[10.942522] arm-smmu-v3 arm-smmu-v3.0.auto: allocated 32768 entries for evtq




--->8

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index f578677a5c41..e6bcddd6ef69 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -560,6 +560,7 @@ struct arm_smmu_cmdq {
atomic_long_t   *valid_map;
atomic_towner_prod;
atomic_tlock;
+   spinlock_t  slock;
  };
  
  struct arm_smmu_cmdq_batch {

@@ -1378,7 +1379,7 @@ static int arm_smmu_cmdq_issue_cmdlist(struct 
arm_smmu_device *smmu,
u64 cmd_sync[CMDQ_ENT_DWORDS];
u32 prod;
unsigned long flags;
-   bool owner;
+   bool owner, locked = false;
struct arm_smmu_cmdq *cmdq = >cmdq;
struct arm_smmu_ll_queue llq = {
.max_n_shift = cmdq->q.llq.max_n_shift,
@@ -1387,27 +1388,38 @@ static int arm_smmu_cmdq_issue_cmdlist(struct 
arm_smmu_device *smmu,
  
  	/* 1. Allocate some space in the queue */

local_irq_save(flags);
-   llq.val = READ_ONCE(cmdq->q.llq.val);
do {
u64 old;
+   llq.val = READ_ONCE(cmdq->q.llq.val);
  
-		while (!queue_has_space(, n + sync)) {

+   if (queue_has_space(, n + sync))
+   goto try_cas;
+
+   if (locked)
+   spin_unlock(>slock);
+
+   do {
local_irq_restore(flags);
if (arm_smmu_cmdq_poll_until_not_full(smmu, ))
dev_err_ratelimited(smmu->dev, "CMDQ 
timeout\n");
local_irq_save(flags);
-   }
+   } while (!queue_has_space(, n + sync));
  
+try_cas:

head.cons = llq.cons;
head.prod = queue_inc_prod_n(, n + sync) |
 CMDQ_PROD_OWNED_FLAG;
  
  		old = cmpxchg_relaxed(>q.llq.val, llq.val, head.val);

-   if (old == llq.val)
+   if (old != llq.val)


Not sure why you changed this. And if I change it back, it seems that we 
could drop out of the loop with cmdq->slock held, so need to drop the 
lock also.


I tried that and it stops my machine hanging. Let me know that was the 
intention, so I can test.


Thanks,
John


break;
  
-		llq.val = old;

+   if (!locked) {
+   spin_lock(>slock);
+   locked = true;
+   }
} while (1);
+
owner = !(llq.prod & CMDQ_PROD_OWNED_FLAG);
head.prod &= ~CMDQ_PROD_OWNED_FLAG;
llq.prod &= ~CMDQ_PROD_OWNED_FLAG;
@@ -3192,6 +3204,7 @@ static int arm_smmu_cmdq_init(struct arm_smmu_device 
*smmu)
  
  	atomic_set(>owner_prod, 0);

atomic_set(>lock, 0);
+   spin_lock_init(>slock);
  
  	bitmap = (atomic_long_t *)bitmap_zalloc(nents, GFP_KERNEL);

if (!bitmap) {
.



___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 0/4] Add system mmu support for Armada-806

[Marcin Wojtas]

czw., 16 lip 2020 o 14:02 Will Deacon  napisał(a):
>
> On Thu, Jul 16, 2020 at 01:00:43PM +0100, Will Deacon wrote:
> > On Wed, 15 Jul 2020 09:06:45 +0200, Tomasz Nowicki wrote:
> > > The series is meant to support SMMU for AP806 and a workaround
> > > for accessing ARM SMMU 64bit registers is the gist of it.
> > >
> > > For the record, AP-806 can't access SMMU registers with 64bit width.
> > > This patches split the readq/writeq into two 32bit accesses instead
> > > and update DT bindings.
> > >
> > > [...]
> >
> > Applied to will (for-joerg/arm-smmu/updates), thanks!
> >
> > [1/3] iommu/arm-smmu: Call configuration impl hook before consuming features
> >   https://git.kernel.org/will/c/6a79a5a3842b
> > [2/3] iommu/arm-smmu: Workaround for Marvell Armada-AP806 SoC erratum 
> > #582743
> >   https://git.kernel.org/will/c/f2d9848aeb9f
> > [3/3] dt-bindings: arm-smmu: add compatible string for Marvell Armada-AP806 
> > SMMU-500
> >   https://git.kernel.org/will/c/e85e84d19b9d
>
> (note that I left patch 4 for arm-soc, as that's just updating .dts files)
>

Hi Gregory,

Can you please help with the review/merge of patch #4?

Best regards,
Marcin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 0/3] Allow for qcom-pdc to be loadable as a module

[Linus Walleij]

On Sat, Jul 11, 2020 at 1:18 AM John Stultz  wrote:

> This patch series provides exports and config tweaks to allow
> the qcom-pdc driver to be able to be configured as a permement
> modules (particularlly useful for the Android Generic Kernel
> Image efforts).
>
> This was part of a larger patch series, to enable qcom_scm
> driver to be a module as well, but I've split it out as there
> are some outstanding objections I still need to address with
> the follow-on patches, and wanted to see if progress could be
> made on this subset of the series in the meantime.
>
> New in v3:
> *  Drop conditional usage of IRQCHIP_DECLARE as suggested by
>Stephen Boyd and Marc Zyngier

This patch set looks entirely reasonable to me.
Reviewed-by: Linus Walleij 

Yours,
Linus Walleij
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 0/4] Add system mmu support for Armada-806

[Will Deacon]

On Thu, Jul 16, 2020 at 01:00:43PM +0100, Will Deacon wrote:
> On Wed, 15 Jul 2020 09:06:45 +0200, Tomasz Nowicki wrote:
> > The series is meant to support SMMU for AP806 and a workaround
> > for accessing ARM SMMU 64bit registers is the gist of it.
> > 
> > For the record, AP-806 can't access SMMU registers with 64bit width.
> > This patches split the readq/writeq into two 32bit accesses instead
> > and update DT bindings.
> > 
> > [...]
> 
> Applied to will (for-joerg/arm-smmu/updates), thanks!
> 
> [1/3] iommu/arm-smmu: Call configuration impl hook before consuming features
>   https://git.kernel.org/will/c/6a79a5a3842b
> [2/3] iommu/arm-smmu: Workaround for Marvell Armada-AP806 SoC erratum #582743
>   https://git.kernel.org/will/c/f2d9848aeb9f
> [3/3] dt-bindings: arm-smmu: add compatible string for Marvell Armada-AP806 
> SMMU-500
>   https://git.kernel.org/will/c/e85e84d19b9d

(note that I left patch 4 for arm-soc, as that's just updating .dts files)

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 0/4] Add system mmu support for Armada-806

[Will Deacon]

On Wed, 15 Jul 2020 09:06:45 +0200, Tomasz Nowicki wrote:
> The series is meant to support SMMU for AP806 and a workaround
> for accessing ARM SMMU 64bit registers is the gist of it.
> 
> For the record, AP-806 can't access SMMU registers with 64bit width.
> This patches split the readq/writeq into two 32bit accesses instead
> and update DT bindings.
> 
> [...]

Applied to will (for-joerg/arm-smmu/updates), thanks!

[1/3] iommu/arm-smmu: Call configuration impl hook before consuming features
  https://git.kernel.org/will/c/6a79a5a3842b
[2/3] iommu/arm-smmu: Workaround for Marvell Armada-AP806 SoC erratum #582743
  https://git.kernel.org/will/c/f2d9848aeb9f
[3/3] dt-bindings: arm-smmu: add compatible string for Marvell Armada-AP806 
SMMU-500
  https://git.kernel.org/will/c/e85e84d19b9d

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/4] iommu/arm-smmu-v3: Improve cmdq lock efficiency

[John Garry]

On 16/07/2020 12:22, Robin Murphy wrote:

On 2020-07-16 11:56, John Garry wrote:

On 16/07/2020 11:28, Will Deacon wrote:

On Thu, Jul 16, 2020 at 11:22:33AM +0100, Will Deacon wrote:

On Thu, Jul 16, 2020 at 11:19:41AM +0100, Will Deacon wrote:

On Tue, Jun 23, 2020 at 01:28:36AM +0800, John Garry wrote:

As mentioned in [0], the CPU may consume many cycles processing
arm_smmu_cmdq_issue_cmdlist(). One issue we find is the cmpxchg()
loop to
get space on the queue takes approx 25% of the cycles for this
function.

This series removes that cmpxchg().


How about something much simpler like the diff below? >>

Ah, scratch that, I don't drop the lock if we fail the cas with it held.
Let me hack it some more (I have no hardware so I can only build-test
this).


Right, second attempt...


I can try it, but if performance if not as good, then please check mine
further (patch 4/4 specifically) - performance is really good, IMHO.


Perhaps a silly question (I'm too engrossed in PMU world ATM to get
properly back up to speed on this), but couldn't this be done without
cmpxchg anyway? Instinctively it feels like instead of maintaining a
literal software copy of the prod value, we could resolve the "claim my
slot in the queue" part with atomic_fetch_add on a free-running 32-bit
"pseudo-prod" index, then whoever updates the hardware deals with the
truncation and wrap bit to convert it to an actual register value.



That's what mine does. But I also need to take care of cmdq locking and 
how we unconditionally provide space.


Cheers,
John
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/4] iommu/arm-smmu-v3: Improve cmdq lock efficiency

[Will Deacon]

On Thu, Jul 16, 2020 at 12:22:05PM +0100, Robin Murphy wrote:
> On 2020-07-16 11:56, John Garry wrote:
> > On 16/07/2020 11:28, Will Deacon wrote:
> > > On Thu, Jul 16, 2020 at 11:22:33AM +0100, Will Deacon wrote:
> > > > On Thu, Jul 16, 2020 at 11:19:41AM +0100, Will Deacon wrote:
> > > > > On Tue, Jun 23, 2020 at 01:28:36AM +0800, John Garry wrote:
> > > > > > As mentioned in [0], the CPU may consume many cycles processing
> > > > > > arm_smmu_cmdq_issue_cmdlist(). One issue we find is the
> > > > > > cmpxchg() loop to
> > > > > > get space on the queue takes approx 25% of the cycles
> > > > > > for this function.
> > > > > > 
> > > > > > This series removes that cmpxchg().
> > > > > 
> > > > > How about something much simpler like the diff below? >>
> > > > Ah, scratch that, I don't drop the lock if we fail the cas with it held.
> > > > Let me hack it some more (I have no hardware so I can only
> > > > build-test this).
> > > 
> > > Right, second attempt...
> > 
> > I can try it, but if performance if not as good, then please check mine
> > further (patch 4/4 specifically) - performance is really good, IMHO.
> 
> Perhaps a silly question (I'm too engrossed in PMU world ATM to get properly
> back up to speed on this), but couldn't this be done without cmpxchg anyway?
> Instinctively it feels like instead of maintaining a literal software copy
> of the prod value, we could resolve the "claim my slot in the queue" part
> with atomic_fetch_add on a free-running 32-bit "pseudo-prod" index, then
> whoever updates the hardware deals with the truncation and wrap bit to
> convert it to an actual register value.

Maybe, but it's easier said than done. The hard part is figuring how that
you have space and there's no way I'm touching that logic without a way to
test this.

I'm also not keen to complicate the code because of systems that don't scale
well with contended CAS [1]. If you put down loads of cores, you need an
interconnect/coherence protocol that can handle it.

Will

[1] 
https://lore.kernel.org/lkml/20190607072652.GA5522@hc/T/#m0d00f107c29223045933292a8b5b90d2ca9b7e5c
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/4] iommu/arm-smmu-v3: Improve cmdq lock efficiency

[Robin Murphy]

On 2020-07-16 11:56, John Garry wrote:

On 16/07/2020 11:28, Will Deacon wrote:

On Thu, Jul 16, 2020 at 11:22:33AM +0100, Will Deacon wrote:

On Thu, Jul 16, 2020 at 11:19:41AM +0100, Will Deacon wrote:

On Tue, Jun 23, 2020 at 01:28:36AM +0800, John Garry wrote:

As mentioned in [0], the CPU may consume many cycles processing
arm_smmu_cmdq_issue_cmdlist(). One issue we find is the cmpxchg() 
loop to
get space on the queue takes approx 25% of the cycles for this 
function.


This series removes that cmpxchg().


How about something much simpler like the diff below? >>

Ah, scratch that, I don't drop the lock if we fail the cas with it held.
Let me hack it some more (I have no hardware so I can only build-test 
this).


Right, second attempt...


I can try it, but if performance if not as good, then please check mine 
further (patch 4/4 specifically) - performance is really good, IMHO.


Perhaps a silly question (I'm too engrossed in PMU world ATM to get 
properly back up to speed on this), but couldn't this be done without 
cmpxchg anyway? Instinctively it feels like instead of maintaining a 
literal software copy of the prod value, we could resolve the "claim my 
slot in the queue" part with atomic_fetch_add on a free-running 32-bit 
"pseudo-prod" index, then whoever updates the hardware deals with the 
truncation and wrap bit to convert it to an actual register value.


Robin.



Thanks,



Will

--->8

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index f578677a5c41..e6bcddd6ef69 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -560,6 +560,7 @@ struct arm_smmu_cmdq {
  atomic_long_t    *valid_map;
  atomic_t    owner_prod;
  atomic_t    lock;
+    spinlock_t    slock;
  };
  struct arm_smmu_cmdq_batch {
@@ -1378,7 +1379,7 @@ static int arm_smmu_cmdq_issue_cmdlist(struct 
arm_smmu_device *smmu,

  u64 cmd_sync[CMDQ_ENT_DWORDS];
  u32 prod;
  unsigned long flags;
-    bool owner;
+    bool owner, locked = false;
  struct arm_smmu_cmdq *cmdq = >cmdq;
  struct arm_smmu_ll_queue llq = {
  .max_n_shift = cmdq->q.llq.max_n_shift,
@@ -1387,27 +1388,38 @@ static int arm_smmu_cmdq_issue_cmdlist(struct 
arm_smmu_device *smmu,

  /* 1. Allocate some space in the queue */
  local_irq_save(flags);
-    llq.val = READ_ONCE(cmdq->q.llq.val);
  do {
  u64 old;
+    llq.val = READ_ONCE(cmdq->q.llq.val);
-    while (!queue_has_space(, n + sync)) {
+    if (queue_has_space(, n + sync))
+    goto try_cas;
+
+    if (locked)
+    spin_unlock(>slock);
+
+    do {
  local_irq_restore(flags);
  if (arm_smmu_cmdq_poll_until_not_full(smmu, ))
  dev_err_ratelimited(smmu->dev, "CMDQ timeout\n");
  local_irq_save(flags);
-    }
+    } while (!queue_has_space(, n + sync));
+try_cas:
  head.cons = llq.cons;
  head.prod = queue_inc_prod_n(, n + sync) |
   CMDQ_PROD_OWNED_FLAG;
  old = cmpxchg_relaxed(>q.llq.val, llq.val, head.val);
-    if (old == llq.val)
+    if (old != llq.val)
  break;
-    llq.val = old;
+    if (!locked) {
+    spin_lock(>slock);
+    locked = true;
+    }
  } while (1);
+
  owner = !(llq.prod & CMDQ_PROD_OWNED_FLAG);
  head.prod &= ~CMDQ_PROD_OWNED_FLAG;
  llq.prod &= ~CMDQ_PROD_OWNED_FLAG;
@@ -3192,6 +3204,7 @@ static int arm_smmu_cmdq_init(struct 
arm_smmu_device *smmu)

  atomic_set(>owner_prod, 0);
  atomic_set(>lock, 0);
+    spin_lock_init(>slock);
  bitmap = (atomic_long_t *)bitmap_zalloc(nents, GFP_KERNEL);
  if (!bitmap) {
.




___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH RESEND v9 18/18] media: platform: Add jpeg enc feature

[Hans Verkuil]

On 08/07/2020 13:16, Tomasz Figa wrote:
> On Wed, Jul 8, 2020 at 9:14 AM Xia Jiang  wrote:
>>
>> On Tue, 2020-07-07 at 15:35 +0200, Tomasz Figa wrote:
>>> On Tue, Jul 7, 2020 at 8:47 AM Xia Jiang  wrote:

 On Tue, 2020-06-30 at 16:53 +, Tomasz Figa wrote:
> Hi Xia,
>
> On Tue, Jun 30, 2020 at 10:56:21AM +0800, Xia Jiang wrote:
>> On Thu, 2020-06-11 at 18:46 +, Tomasz Figa wrote:
>>> Hi Xia,
>>>
>>> On Thu, Jun 04, 2020 at 05:05:53PM +0800, Xia Jiang wrote:
> [snip]
 +static void mtk_jpeg_enc_device_run(void *priv)
 +{
 +   struct mtk_jpeg_ctx *ctx = priv;
 +   struct mtk_jpeg_dev *jpeg = ctx->jpeg;
 +   struct vb2_v4l2_buffer *src_buf, *dst_buf;
 +   enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
 +   unsigned long flags;
 +   struct mtk_jpeg_src_buf *jpeg_src_buf;
 +   struct mtk_jpeg_enc_bs enc_bs;
 +   int ret;
 +
 +   src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
 +   dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
 +   jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(_buf->vb2_buf);
 +
 +   ret = pm_runtime_get_sync(jpeg->dev);
 +   if (ret < 0)
 +   goto enc_end;
 +
 +   spin_lock_irqsave(>hw_lock, flags);
 +
 +   /*
 +* Resetting the hardware every frame is to ensure that all the
 +* registers are cleared. This is a hardware requirement.
 +*/
 +   mtk_jpeg_enc_reset(jpeg->reg_base);
 +
 +   mtk_jpeg_set_enc_dst(ctx, jpeg->reg_base, _buf->vb2_buf, 
 _bs);
 +   mtk_jpeg_set_enc_src(ctx, jpeg->reg_base, _buf->vb2_buf);
 +   mtk_jpeg_enc_set_config(jpeg->reg_base, 
 ctx->out_q.fmt->hw_format,
 +   ctx->enable_exif, ctx->enc_quality,
 +   ctx->restart_interval);
 +   mtk_jpeg_enc_start(jpeg->reg_base);
>>>
>>> Could we just move the above 5 functions into one function inside
>>> mtk_jpeg_enc_hw.c that takes mtk_jpeg_dev pointer as its argument, let's
>>> say mtk_jpeg_enc_hw_run() and simply program all the data to the 
>>> registers
>>> directly, without the extra level of abstractions?
>> I can move the 5 functions into one function(mtk_jpeg_enc_hw_run()), but
>> this function will be very long, because it contains computation code
>> such as setting dst addr, blk_num, quality.
>> In v4, you have adviced the following architecture:
>> How about the following model, as used by many other drivers:
>>
>> mtk_jpeg_enc_set_src()
>> {
>> // Set any registers related to source format and buffer
>> }
>>
>> mtk_jpeg_enc_set_dst()
>> {
>> // Set any registers related to destination format and buffer
>> }
>>
>> mtk_jpeg_enc_set_params()
>> {
>> // Set any registers related to additional encoding parameters
>> }
>>
>> mtk_jpeg_enc_device_run(enc, ctx)
>> {
>> mtk_jpeg_enc_set_src(enc, src_buf, src_fmt);
>> mtk_jpeg_enc_set_dst(enc, dst_buf, dst_fmt);
>> mtk_jpeg_enc_set_params(enc, ctx);
>> // Trigger the hardware run
>> }
>> I think that this architecture is more clear(mtk_jpeg_enc_set_config is
>> equivalent to mtk_jpeg_enc_set_params).
>> Should I keep the original architecture or move 5 functions into
>> mtk_jpeg_enc_hw_run?
>
> Sounds good to me.
>
> My biggest issue with the code that it ends up introducing one more
> level of abstraction, but with the approach you suggested, the arguments
> just accept standard structs, which avoids that problem.
>
> [snip]
 +
 +   ctx->fh.ctrl_handler = >ctrl_hdl;
 +   ctx->colorspace = V4L2_COLORSPACE_JPEG,
 +   ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
 +   ctx->quantization = V4L2_QUANTIZATION_DEFAULT;
 +   ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT;
>>>
>>> Since we already have a v4l2_pix_format_mplane struct which has fields 
>>> for
>>> the above 4 values, could we just store them there?
>>>
>>> Also, I don't see this driver handling the colorspaces in any way, but 
>>> it
>>> seems to allow changing them from the userspace. This is incorrect, 
>>> because
>>> the userspace has no way to know that the colorspace is not handled.
>>> Instead, the try_fmt implementation should always override the
>>> userspace-provided colorspace configuration with the ones that the 
>>> driver
>>> assumes.
>>>
 +   pix_mp->width = MTK_JPEG_MIN_WIDTH;
 +   pix_mp->height = MTK_JPEG_MIN_HEIGHT;
 +

Re: [PATCH 0/4] iommu/arm-smmu-v3: Improve cmdq lock efficiency

[John Garry]

On 16/07/2020 11:28, Will Deacon wrote:

On Thu, Jul 16, 2020 at 11:22:33AM +0100, Will Deacon wrote:

On Thu, Jul 16, 2020 at 11:19:41AM +0100, Will Deacon wrote:

On Tue, Jun 23, 2020 at 01:28:36AM +0800, John Garry wrote:

As mentioned in [0], the CPU may consume many cycles processing
arm_smmu_cmdq_issue_cmdlist(). One issue we find is the cmpxchg() loop to
get space on the queue takes approx 25% of the cycles for this function.

This series removes that cmpxchg().


How about something much simpler like the diff below? >>

Ah, scratch that, I don't drop the lock if we fail the cas with it held.
Let me hack it some more (I have no hardware so I can only build-test this).


Right, second attempt...


I can try it, but if performance if not as good, then please check mine 
further (patch 4/4 specifically) - performance is really good, IMHO.


Thanks,



Will

--->8

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index f578677a5c41..e6bcddd6ef69 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -560,6 +560,7 @@ struct arm_smmu_cmdq {
atomic_long_t   *valid_map;
atomic_towner_prod;
atomic_tlock;
+   spinlock_t  slock;
  };
  
  struct arm_smmu_cmdq_batch {

@@ -1378,7 +1379,7 @@ static int arm_smmu_cmdq_issue_cmdlist(struct 
arm_smmu_device *smmu,
u64 cmd_sync[CMDQ_ENT_DWORDS];
u32 prod;
unsigned long flags;
-   bool owner;
+   bool owner, locked = false;
struct arm_smmu_cmdq *cmdq = >cmdq;
struct arm_smmu_ll_queue llq = {
.max_n_shift = cmdq->q.llq.max_n_shift,
@@ -1387,27 +1388,38 @@ static int arm_smmu_cmdq_issue_cmdlist(struct 
arm_smmu_device *smmu,
  
  	/* 1. Allocate some space in the queue */

local_irq_save(flags);
-   llq.val = READ_ONCE(cmdq->q.llq.val);
do {
u64 old;
+   llq.val = READ_ONCE(cmdq->q.llq.val);
  
-		while (!queue_has_space(, n + sync)) {

+   if (queue_has_space(, n + sync))
+   goto try_cas;
+
+   if (locked)
+   spin_unlock(>slock);
+
+   do {
local_irq_restore(flags);
if (arm_smmu_cmdq_poll_until_not_full(smmu, ))
dev_err_ratelimited(smmu->dev, "CMDQ 
timeout\n");
local_irq_save(flags);
-   }
+   } while (!queue_has_space(, n + sync));
  
+try_cas:

head.cons = llq.cons;
head.prod = queue_inc_prod_n(, n + sync) |
 CMDQ_PROD_OWNED_FLAG;
  
  		old = cmpxchg_relaxed(>q.llq.val, llq.val, head.val);

-   if (old == llq.val)
+   if (old != llq.val)
break;
  
-		llq.val = old;

+   if (!locked) {
+   spin_lock(>slock);
+   locked = true;
+   }
} while (1);
+
owner = !(llq.prod & CMDQ_PROD_OWNED_FLAG);
head.prod &= ~CMDQ_PROD_OWNED_FLAG;
llq.prod &= ~CMDQ_PROD_OWNED_FLAG;
@@ -3192,6 +3204,7 @@ static int arm_smmu_cmdq_init(struct arm_smmu_device 
*smmu)
  
  	atomic_set(>owner_prod, 0);

atomic_set(>lock, 0);
+   spin_lock_init(>slock);
  
  	bitmap = (atomic_long_t *)bitmap_zalloc(nents, GFP_KERNEL);

if (!bitmap) {
.



___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/4] iommu/arm-smmu-v3: Improve cmdq lock efficiency

[Will Deacon]

On Thu, Jul 16, 2020 at 11:22:33AM +0100, Will Deacon wrote:
> On Thu, Jul 16, 2020 at 11:19:41AM +0100, Will Deacon wrote:
> > On Tue, Jun 23, 2020 at 01:28:36AM +0800, John Garry wrote:
> > > As mentioned in [0], the CPU may consume many cycles processing
> > > arm_smmu_cmdq_issue_cmdlist(). One issue we find is the cmpxchg() loop to
> > > get space on the queue takes approx 25% of the cycles for this function.
> > > 
> > > This series removes that cmpxchg().
> > 
> > How about something much simpler like the diff below?
> 
> Ah, scratch that, I don't drop the lock if we fail the cas with it held.
> Let me hack it some more (I have no hardware so I can only build-test this).

Right, second attempt...

Will

--->8

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index f578677a5c41..e6bcddd6ef69 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -560,6 +560,7 @@ struct arm_smmu_cmdq {
atomic_long_t   *valid_map;
atomic_towner_prod;
atomic_tlock;
+   spinlock_t  slock;
 };
 
 struct arm_smmu_cmdq_batch {
@@ -1378,7 +1379,7 @@ static int arm_smmu_cmdq_issue_cmdlist(struct 
arm_smmu_device *smmu,
u64 cmd_sync[CMDQ_ENT_DWORDS];
u32 prod;
unsigned long flags;
-   bool owner;
+   bool owner, locked = false;
struct arm_smmu_cmdq *cmdq = >cmdq;
struct arm_smmu_ll_queue llq = {
.max_n_shift = cmdq->q.llq.max_n_shift,
@@ -1387,27 +1388,38 @@ static int arm_smmu_cmdq_issue_cmdlist(struct 
arm_smmu_device *smmu,
 
/* 1. Allocate some space in the queue */
local_irq_save(flags);
-   llq.val = READ_ONCE(cmdq->q.llq.val);
do {
u64 old;
+   llq.val = READ_ONCE(cmdq->q.llq.val);
 
-   while (!queue_has_space(, n + sync)) {
+   if (queue_has_space(, n + sync))
+   goto try_cas;
+
+   if (locked)
+   spin_unlock(>slock);
+
+   do {
local_irq_restore(flags);
if (arm_smmu_cmdq_poll_until_not_full(smmu, ))
dev_err_ratelimited(smmu->dev, "CMDQ 
timeout\n");
local_irq_save(flags);
-   }
+   } while (!queue_has_space(, n + sync));
 
+try_cas:
head.cons = llq.cons;
head.prod = queue_inc_prod_n(, n + sync) |
 CMDQ_PROD_OWNED_FLAG;
 
old = cmpxchg_relaxed(>q.llq.val, llq.val, head.val);
-   if (old == llq.val)
+   if (old != llq.val)
break;
 
-   llq.val = old;
+   if (!locked) {
+   spin_lock(>slock);
+   locked = true;
+   }
} while (1);
+
owner = !(llq.prod & CMDQ_PROD_OWNED_FLAG);
head.prod &= ~CMDQ_PROD_OWNED_FLAG;
llq.prod &= ~CMDQ_PROD_OWNED_FLAG;
@@ -3192,6 +3204,7 @@ static int arm_smmu_cmdq_init(struct arm_smmu_device 
*smmu)
 
atomic_set(>owner_prod, 0);
atomic_set(>lock, 0);
+   spin_lock_init(>slock);
 
bitmap = (atomic_long_t *)bitmap_zalloc(nents, GFP_KERNEL);
if (!bitmap) {
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/4] iommu/arm-smmu-v3: Remove cmpxchg() in arm_smmu_cmdq_issue_cmdlist()

[John Garry]

On 16/07/2020 11:20, Will Deacon wrote:

On Tue, Jun 23, 2020 at 01:28:40AM +0800, John Garry wrote:

It has been shown that the cmpxchg() for finding space in the cmdq can
be a bottleneck:
- for more CPUs contending the cmdq, the cmpxchg() will fail more often
- since the software-maintained cons pointer is updated on the same 64b
   memory region, the chance of cmpxchg() failure increases again

The cmpxchg() is removed as part of 2 related changes:

- Update prod and cmdq owner in a single atomic add operation. For this, we
   count the prod and owner in separate regions in prod memory.

   As with simple binary counting, once the prod+wrap fields overflow, they
   will zero. They should never overflow into "owner" region, and we zero
   the non-owner, prod region for each owner. This maintains the prod
   pointer.

   As for the "owner", we now count this value, instead of setting a flag.
   Similar to before, once the owner has finished gathering, it will clear
   a mask. As such, a CPU declares itself as the "owner" when it reads zero
   for this region. This zeroing will also clear possible overflow in
   wrap+prod region, above.

   The owner is now responsible for all cmdq locking to avoid possible
   deadlock. The owner will lock the cmdq for all non-owers it has gathered
   when they have space in the queue and have written their entries.

- Check for space in the cmdq after the prod pointer has been assigned.

   We don't bother checking for space in the cmdq before assigning the prod
   pointer, as this would be racy.

   So since the prod pointer is updated unconditionally, it would be common
   for no space to be available in the cmdq when prod is assigned - that
   is, according the software-maintained prod and cons pointer. So now
   it must be ensured that the entries are not yet written and not until
   there is space.

   How the prod pointer is maintained also leads to a strange condition
   where the prod pointer can wrap past the cons pointer. We can detect this
   condition, and report no space here. However, a prod pointer progressed
   twice past the cons pointer cannot be detected. But it can be ensured that
   this that this scenario does not occur, as we limit the amount of
   commands any CPU can issue at any given time, such that we cannot
   progress prod pointer further.

Signed-off-by: John Garry 
---
  drivers/iommu/arm-smmu-v3.c | 101 ++--
  1 file changed, 61 insertions(+), 40 deletions(-)


I must admit, you made me smile putting triv...@kernel.org on cc for this ;)



Yes, quite ironic :)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/4] iommu/arm-smmu-v3: Improve cmdq lock efficiency

[Will Deacon]

On Thu, Jul 16, 2020 at 11:19:41AM +0100, Will Deacon wrote:
> On Tue, Jun 23, 2020 at 01:28:36AM +0800, John Garry wrote:
> > As mentioned in [0], the CPU may consume many cycles processing
> > arm_smmu_cmdq_issue_cmdlist(). One issue we find is the cmpxchg() loop to
> > get space on the queue takes approx 25% of the cycles for this function.
> > 
> > This series removes that cmpxchg().
> 
> How about something much simpler like the diff below?

Ah, scratch that, I don't drop the lock if we fail the cas with it held.
Let me hack it some more (I have no hardware so I can only build-test this).

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/4] iommu/arm-smmu-v3: Remove cmpxchg() in arm_smmu_cmdq_issue_cmdlist()

[Will Deacon]

On Tue, Jun 23, 2020 at 01:28:40AM +0800, John Garry wrote:
> It has been shown that the cmpxchg() for finding space in the cmdq can
> be a bottleneck:
> - for more CPUs contending the cmdq, the cmpxchg() will fail more often
> - since the software-maintained cons pointer is updated on the same 64b
>   memory region, the chance of cmpxchg() failure increases again
> 
> The cmpxchg() is removed as part of 2 related changes:
> 
> - Update prod and cmdq owner in a single atomic add operation. For this, we
>   count the prod and owner in separate regions in prod memory.
> 
>   As with simple binary counting, once the prod+wrap fields overflow, they
>   will zero. They should never overflow into "owner" region, and we zero
>   the non-owner, prod region for each owner. This maintains the prod
>   pointer.
> 
>   As for the "owner", we now count this value, instead of setting a flag.
>   Similar to before, once the owner has finished gathering, it will clear
>   a mask. As such, a CPU declares itself as the "owner" when it reads zero
>   for this region. This zeroing will also clear possible overflow in
>   wrap+prod region, above.
> 
>   The owner is now responsible for all cmdq locking to avoid possible
>   deadlock. The owner will lock the cmdq for all non-owers it has gathered
>   when they have space in the queue and have written their entries.
> 
> - Check for space in the cmdq after the prod pointer has been assigned.
> 
>   We don't bother checking for space in the cmdq before assigning the prod
>   pointer, as this would be racy.
> 
>   So since the prod pointer is updated unconditionally, it would be common
>   for no space to be available in the cmdq when prod is assigned - that
>   is, according the software-maintained prod and cons pointer. So now
>   it must be ensured that the entries are not yet written and not until
>   there is space.
> 
>   How the prod pointer is maintained also leads to a strange condition
>   where the prod pointer can wrap past the cons pointer. We can detect this
>   condition, and report no space here. However, a prod pointer progressed
>   twice past the cons pointer cannot be detected. But it can be ensured that
>   this that this scenario does not occur, as we limit the amount of
>   commands any CPU can issue at any given time, such that we cannot
>   progress prod pointer further.
> 
> Signed-off-by: John Garry 
> ---
>  drivers/iommu/arm-smmu-v3.c | 101 ++--
>  1 file changed, 61 insertions(+), 40 deletions(-)

I must admit, you made me smile putting triv...@kernel.org on cc for this ;)

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/4] iommu/arm-smmu-v3: Improve cmdq lock efficiency

[Will Deacon]

On Tue, Jun 23, 2020 at 01:28:36AM +0800, John Garry wrote:
> As mentioned in [0], the CPU may consume many cycles processing
> arm_smmu_cmdq_issue_cmdlist(). One issue we find is the cmpxchg() loop to
> get space on the queue takes approx 25% of the cycles for this function.
> 
> This series removes that cmpxchg().

How about something much simpler like the diff below?

Will

--->8

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index f578677a5c41..705d99ec8219 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -560,6 +560,7 @@ struct arm_smmu_cmdq {
atomic_long_t   *valid_map;
atomic_towner_prod;
atomic_tlock;
+   spinlock_t  slock;
 };
 
 struct arm_smmu_cmdq_batch {
@@ -1378,7 +1379,7 @@ static int arm_smmu_cmdq_issue_cmdlist(struct 
arm_smmu_device *smmu,
u64 cmd_sync[CMDQ_ENT_DWORDS];
u32 prod;
unsigned long flags;
-   bool owner;
+   bool owner, cas_failed = false;
struct arm_smmu_cmdq *cmdq = >cmdq;
struct arm_smmu_ll_queue llq = {
.max_n_shift = cmdq->q.llq.max_n_shift,
@@ -1387,27 +1388,35 @@ static int arm_smmu_cmdq_issue_cmdlist(struct 
arm_smmu_device *smmu,
 
/* 1. Allocate some space in the queue */
local_irq_save(flags);
-   llq.val = READ_ONCE(cmdq->q.llq.val);
do {
u64 old;
+   llq.val = READ_ONCE(cmdq->q.llq.val);
 
-   while (!queue_has_space(, n + sync)) {
+   if (queue_has_space(, n + sync))
+   goto try_cas;
+
+   if (cas_failed)
+   spin_unlock(>slock);
+
+   do {
local_irq_restore(flags);
if (arm_smmu_cmdq_poll_until_not_full(smmu, ))
dev_err_ratelimited(smmu->dev, "CMDQ 
timeout\n");
local_irq_save(flags);
-   }
+   } while (!queue_has_space(, n + sync));
 
+try_cas:
head.cons = llq.cons;
head.prod = queue_inc_prod_n(, n + sync) |
 CMDQ_PROD_OWNED_FLAG;
 
old = cmpxchg_relaxed(>q.llq.val, llq.val, head.val);
-   if (old == llq.val)
-   break;
+   cas_failed = old != llq.val;
+
+   if (cas_failed)
+   spin_lock(>slock);
+   } while (cas_failed);
 
-   llq.val = old;
-   } while (1);
owner = !(llq.prod & CMDQ_PROD_OWNED_FLAG);
head.prod &= ~CMDQ_PROD_OWNED_FLAG;
llq.prod &= ~CMDQ_PROD_OWNED_FLAG;
@@ -3192,6 +3201,7 @@ static int arm_smmu_cmdq_init(struct arm_smmu_device 
*smmu)
 
atomic_set(>owner_prod, 0);
atomic_set(>lock, 0);
+   spin_lock_init(>slock);
 
bitmap = (atomic_long_t *)bitmap_zalloc(nents, GFP_KERNEL);
if (!bitmap) {
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [Freedreno] [PATCH v9 4/7] iommu/arm-smmu: Add a pointer to the attached device to smmu_domain

[Will Deacon]

On Mon, Jul 13, 2020 at 11:19:17AM -0600, Jordan Crouse wrote:
> On Mon, Jul 13, 2020 at 04:09:02PM +0100, Will Deacon wrote:
> > On Fri, Jun 26, 2020 at 02:00:38PM -0600, Jordan Crouse wrote:
> > > diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> > > index 5f2de20e883b..d33cfe26b2f5 100644
> > > --- a/drivers/iommu/arm-smmu.h
> > > +++ b/drivers/iommu/arm-smmu.h
> > > @@ -345,6 +345,7 @@ struct arm_smmu_domain {
> > >   struct mutexinit_mutex; /* Protects smmu pointer */
> > >   spinlock_t  cb_lock; /* Serialises ATS1* ops and 
> > > TLB syncs */
> > >   struct iommu_domain domain;
> > > + struct device   *dev;   /* Device attached to this 
> > > domain */
> > 
> > This really doesn't feel right to me -- you can generally have multiple
> > devices attached to a domain and they can come and go without the domain
> > being destroyed. Perhaps you could instead identify the GPU during
> > cfg_probe() and squirrel that information away somewhere?
> 
> I need some help here. The SMMU device (qcom,adreno-smmu) will have at least 
> two
> stream ids from two different platform devices (GPU and GMU) and I need to
> configure split-pagetable and stall/terminate differently on the two domains.

Hmm. How does the GPU driver know which context bank is assigned to the GPU
and which one is assigned to the GMU? I assume it needs this information so
that it can play its nasty tricks with the TTBR registers?

I ask because if we need to guarantee stability of the context-bank
assignment, then you could match on that in the ->init_context() callback,
but now I worry that it currently works by luck :/

Do we need to add an extra callback to allocate the context bank?

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 6/7] iommu/vt-d: Warn on out-of-range invalidation address

[Auger Eric]

Hi Jacob,

On 7/7/20 2:12 AM, Jacob Pan wrote:
> For guest requested IOTLB invalidation, address and mask are provided as
> part of the invalidation data. VT-d HW silently ignores any address bits
> below the mask. SW shall also allow such case but give warning if
> address does not align with the mask. This patch relax the fault
> handling from error to warning and proceed with invalidation request
> with the given mask.
> 
> Fixes: 6ee1b77ba3ac0 ("iommu/vt-d: Add svm/sva invalidate function")
> Acked-by: Lu Baolu 
> Signed-off-by: Jacob Pan 
following your replies on my v3 comments,

Reviewed-by: Eric Auger 

Thanks

Eric

> ---
>  drivers/iommu/intel/iommu.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 3bf03c6cd15f..c3a9a85a3c3f 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -5439,13 +5439,12 @@ intel_iommu_sva_invalidate(struct iommu_domain 
> *domain, struct device *dev,
>  
>   switch (BIT(cache_type)) {
>   case IOMMU_CACHE_INV_TYPE_IOTLB:
> + /* HW will ignore LSB bits based on address mask */
>   if (inv_info->granularity == IOMMU_INV_GRANU_ADDR &&
>   size &&
>   (inv_info->addr_info.addr & ((BIT(VTD_PAGE_SHIFT + 
> size)) - 1))) {
> - pr_err_ratelimited("Address out of range, 
> 0x%llx, size order %llu\n",
> -inv_info->addr_info.addr, 
> size);
> - ret = -ERANGE;
> - goto out_unlock;
> + pr_err_ratelimited("User address not aligned, 
> 0x%llx, size order %llu\n",
> +   inv_info->addr_info.addr, size);
>   }
>  
>   /*
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [EXT] Re: [PATCH v2 12/12] bus: fsl-mc: Add ACPI support for fsl-mc

[Marc Zyngier]

On 2020-07-16 04:23, Makarand Pawagi wrote:

-Original Message-
From: Lorenzo Pieralisi 


[...]


Anyway - you need to seek feedback from Marc on whether patches
11 and 12 are OK from an irqchip perspective, it is possible we can 
take the rest
of the series independently if everyone agrees but I don't necessarily 
see a

reason for that.

Long story short: you need Marc's ACK on [11-12], it is your code.


Hi Marc, can you please review/ack this patch?


https://lore.kernel.org/linux-acpi/bd07f44dad1d029e0d023202cbf5f...@kernel.org/

M.
--
Jazz is not dead. It just smells funny...
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/7] iommu/vt-d: Fix PASID devTLB invalidation

[Auger Eric]

Hi Jacob,

On 7/7/20 2:12 AM, Jacob Pan wrote:
> DevTLB flush can be used for both DMA request with and without PASIDs.
> The former uses PASID#0 (RID2PASID), latter uses non-zero PASID for SVA
> usage.
> 
> This patch adds a check for PASID value such that devTLB flush with
> PASID is used for SVA case. This is more efficient in that multiple
> PASIDs can be used by a single device, when tearing down a PASID entry
> we shall flush only the devTLB specific to a PASID.
> 
> Fixes: 6f7db75e1c46 ("iommu/vt-d: Add second level page table")
> Acked-by: Lu Baolu 
> Signed-off-by: Jacob Pan 
Reviewed-by: Eric Auger 
sent on v3.

Thanks

Eric
> ---
>  drivers/iommu/intel/pasid.c | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index c81f0f17c6ba..fa0154cce537 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -486,7 +486,16 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
>   qdep = info->ats_qdep;
>   pfsid = info->pfsid;
>  
> - qi_flush_dev_iotlb(iommu, sid, pfsid, qdep, 0, 64 - VTD_PAGE_SHIFT);
> + /*
> +  * When PASID 0 is used, it indicates RID2PASID(DMA request w/o PASID),
> +  * devTLB flush w/o PASID should be used. For non-zero PASID under
> +  * SVA usage, device could do DMA with multiple PASIDs. It is more
> +  * efficient to flush devTLB specific to the PASID.
> +  */
> + if (pasid == PASID_RID2PASID)
> + qi_flush_dev_iotlb(iommu, sid, pfsid, qdep, 0, 64 - 
> VTD_PAGE_SHIFT);
> + else
> + qi_flush_dev_iotlb_pasid(iommu, sid, pfsid, pasid, qdep, 0, 64 
> - VTD_PAGE_SHIFT);
>  }
>  
>  void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device 
> *dev,
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 4/7] iommu/vt-d: Handle non-page aligned address

[Auger Eric]

Hi Jacob,

On 7/7/20 2:12 AM, Jacob Pan wrote:
> From: Liu Yi L 
> 
> Address information for device TLB invalidation comes from userspace
> when device is directly assigned to a guest with vIOMMU support.
> VT-d requires page aligned address. This patch checks and enforce
> address to be page aligned, otherwise reserved bits can be set in the
> invalidation descriptor. Unrecoverable fault will be reported due to
> non-zero value in the reserved bits.
> 
> Fixes: 61a06a16e36d8 ("iommu/vt-d: Support flushing more translation
> cache types")
> Acked-by: Lu Baolu 
> Signed-off-by: Liu Yi L 
> Signed-off-by: Jacob Pan 
Reviewed-by: Eric Auger 

Thanks

Eric
> 
> ---
>  drivers/iommu/intel/dmar.c | 20 ++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index d9f973fa1190..b2c53bada905 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -1455,9 +1455,25 @@ void qi_flush_dev_iotlb_pasid(struct intel_iommu 
> *iommu, u16 sid, u16 pfsid,
>* Max Invs Pending (MIP) is set to 0 for now until we have DIT in
>* ECAP.
>*/
> - desc.qw1 |= addr & ~mask;
> - if (size_order)
> + if (addr & GENMASK_ULL(size_order + VTD_PAGE_SHIFT, 0))
> + pr_warn_ratelimited("Invalidate non-aligned address %llx, order 
> %d\n", addr, size_order);
> +
> + /* Take page address */
> + desc.qw1 = QI_DEV_EIOTLB_ADDR(addr);
> +
> + if (size_order) {
> + /*
> +  * Existing 0s in address below size_order may be the least
> +  * significant bit, we must set them to 1s to avoid having
> +  * smaller size than desired.
> +  */
> + desc.qw1 |= GENMASK_ULL(size_order + VTD_PAGE_SHIFT,
> + VTD_PAGE_SHIFT);
> + /* Clear size_order bit to indicate size */
> + desc.qw1 &= ~mask;
> + /* Set the S bit to indicate flushing more than 1 page */
>   desc.qw1 |= QI_DEV_EIOTLB_SIZE;
> + }
>  
>   qi_submit_sync(iommu, , 1, 0);
>  }
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/4] dt-bindings: arm-smmu: add compatible string for Marvell Armada-AP806 SMMU-500

[Tomasz Nowicki]

On 15.07.2020 12:36, Robin Murphy wrote:

On 2020-07-15 08:06, Tomasz Nowicki wrote:

Add specific compatible string for Marvell usage due to errata of
accessing 64bits registers of ARM SMMU, in AP806.

AP806 SoC uses the generic ARM-MMU500, and there's no specific
implementation of Marvell, this compatible is used for errata only.


Reviewed-by: Robin Murphy 

Presumably Will can pick up these first 3 patches for 5.9 and #4 can go 
via arm-soc.


Thanks Robin for review and valuable comments.

Tomasz
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/4] iommu/arm-smmu: Workaround for Marvell Armada-AP806 SoC erratum #582743

[Tomasz Nowicki]

On 15.07.2020 12:32, Robin Murphy wrote:

On 2020-07-15 08:06, Tomasz Nowicki wrote:

From: Hanna Hawa 

Due to erratum #582743, the Marvell Armada-AP806 can't access 64bit to
ARM SMMUv2 registers.

Provide implementation relevant hooks:
- split the writeq/readq to two accesses of writel/readl.
- mask the MMU_IDR2.PTFSv8 fields to not use AArch64 format (but
only AARCH32_L) since with AArch64 format 32 bits access is not 
supported.


Note that most 64-bit registers like TTBRn can be accessed as two 32-bit
halves without issue, and AArch32 format ensures that the register writes
which must be atomic (for TLBI etc.) need only be 32-bit.


Thanks Tomasz, this has ended up as clean as I'd hoped it could, and 
there's still room to come back and play more complicated games later if 
a real need for AARCH64_64K at stage 2 crops up.


Based on your implementation infrastructure rework, indeed the code 
looks much cleaner :)




Reviewed-by: Robin Murphy 



Thanks!

Tomasz
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu