Implement SMMU passthrough using the default domain
Subsystems (2)
v2 cover Implement SMMU passthrough using the default domain
Hi all,
This is version two of the patches previously posted here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-January/481275.html
I've addressed the review feedback I receive there, but the main change is that the SMMUv3 driver now handles passthrough domains independently of the disable_bypass parameter.
All feedback welcome,
Will
--->8
Will Deacon (5):
iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains iommu/arm-smmu: Install bypass S2CRs for IOMMU_DOMAIN_IDENTITY domains iommu/arm-smmu-v3: Make arm_smmu_install_ste_for_dev return void iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY
domains
iommu: Allow default domain type to be set on the kernel command line
Documentation/admin-guide/kernel-parameters.txt | 6 ++ drivers/iommu/arm-smmu-v3.c | 76 +++++++++++++++---------- drivers/iommu/arm-smmu.c | 26 ++++++++- drivers/iommu/iommu.c | 17 +++++- 4 files changed, 90 insertions(+), 35 deletions(-)
Signature · 1 line
2.1.4
PATCH v2 1/5 iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains
The ARM SMMU drivers provide a DOMAIN_ATTR_NESTING domain attribute, which allows callers of the IOMMU API to request that the page table for a domain is installed at stage-2, if supported by the hardware.
Since setting this attribute only makes sense for UNMANAGED domains, this patch returns -ENODEV if the domain_{get,set}_attr operations are called on other domain types.
--- drivers/iommu/arm-smmu-v3.c | 6 ++++++ drivers/iommu/arm-smmu.c | 6 ++++++ 2 files changed, 12 insertions(+)
drivers/iommu/arm-smmu-v3.c +6 -0
@@ -1837,6 +1837,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain, {struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain); +if (domain->type !=IOMMU_DOMAIN_UNMANAGED ) +return -EINVAL ; +switch (attr) {case DOMAIN_ATTR_NESTING : *(int *)data = (smmu_domain->stage ==ARM_SMMU_DOMAIN_NESTED );
@@ -1852,6 +1855,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,int ret =0 ;struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain); +if (domain->type !=IOMMU_DOMAIN_UNMANAGED ) +return -EINVAL ; +mutex_lock (&smmu_domain->init_mutex);switch (attr) {
drivers/iommu/arm-smmu.c +6 -0
@@ -1549,6 +1549,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain, {struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain); +if (domain->type !=IOMMU_DOMAIN_UNMANAGED ) +return -EINVAL ; +switch (attr) {case DOMAIN_ATTR_NESTING : *(int *)data = (smmu_domain->stage ==ARM_SMMU_DOMAIN_NESTED );
@@ -1564,6 +1567,9 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,int ret =0 ;struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain); +if (domain->type !=IOMMU_DOMAIN_UNMANAGED ) +return -EINVAL ; +mutex_lock (&smmu_domain->init_mutex);switch (attr) {
Signature · 1 line
2.1.4
PATCH v2 2/5 iommu/arm-smmu: Install bypass S2CRs for IOMMU_DOMAIN_IDENTITY domains
In reply to Will DeaconIn preparation for allowing the default domain type to be overridden, this patch adds support for IOMMU_DOMAIN_IDENTITY domains to the ARM SMMU driver.
An identity domain is created by placing the corresponding S2CR registers into "bypass" mode, which allows transactions to flow through the SMMU without any translation.
--- drivers/iommu/arm-smmu.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
drivers/iommu/arm-smmu.c +17 -3
@@ -416,6 +416,7 @@ enum arm_smmu_domain_stage {ARM_SMMU_DOMAIN_S1 =0 ,ARM_SMMU_DOMAIN_S2 ,ARM_SMMU_DOMAIN_NESTED , + ARM_SMMU_DOMAIN_BYPASS, }; struct arm_smmu_domain {
@@ -838,6 +839,12 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,if (smmu_domain->smmu) goto out_unlock; +if (domain->type ==IOMMU_DOMAIN_IDENTITY ) { + smmu_domain->stage =ARM_SMMU_DOMAIN_BYPASS ; + smmu_domain->smmu = smmu; + goto out_unlock; + } +/* * Mapping the requested stage onto what we support is surprisingly * complicated, mainly because the spec allows S1+S2 SMMUs without
@@ -998,7 +1005,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)void __iomem *cb_base;int irq; -if (!smmu) +if (!smmu || domain->type ==IOMMU_DOMAIN_IDENTITY )return ;/*
@@ -1021,7 +1028,9 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) {struct arm_smmu_domain *smmu_domain; -if (type !=IOMMU_DOMAIN_UNMANAGED && type !=IOMMU_DOMAIN_DMA ) +if (type !=IOMMU_DOMAIN_UNMANAGED && + type !=IOMMU_DOMAIN_DMA && + type !=IOMMU_DOMAIN_IDENTITY )return NULL ;/* * Allocate the domain and initialise some of its data structures.
@@ -1250,10 +1259,15 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain, {struct arm_smmu_device *smmu = smmu_domain->smmu;struct arm_smmu_s2cr *s2cr = smmu->s2crs; -enum arm_smmu_s2cr_type type =S2CR_TYPE_TRANS ;u8 cbndx = smmu_domain->cfg.cbndx; +enum arm_smmu_s2cr_type type;int i, idx; +if (smmu_domain->stage ==ARM_SMMU_DOMAIN_BYPASS ) + type =S2CR_TYPE_BYPASS ; +else + type =S2CR_TYPE_TRANS ; +for_each_cfg_sme (fwspec, i, idx) {if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)continue ;
Signature · 1 line
2.1.4
PATCH v2 3/5 iommu/arm-smmu-v3: Make arm_smmu_install_ste_for_dev return void
In reply to Will Deaconarm_smmu_install_ste_for_dev cannot fail and always returns 0, however the fact that it returns int means that callers end up implementing redundant error handling code which complicates STE tracking and is never executed.
This patch changes the return type of arm_smmu_install_ste_for_dev to avoid, to make it explicit that it cannot fail.
--- drivers/iommu/arm-smmu-v3.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
drivers/iommu/arm-smmu-v3.c +3 -9
@@ -1579,7 +1579,7 @@ static __le64 *arm_smmu_get_step_for_sid(struct arm_smmu_device *smmu, u32 sid)return step; } -static int arm_smmu_install_ste_for_dev (struct iommu_fwspec *fwspec) +static void arm_smmu_install_ste_for_dev (struct iommu_fwspec *fwspec) {int i;struct arm_smmu_master_data *master = fwspec->iommu_priv;
@@ -1591,8 +1591,6 @@ static int arm_smmu_install_ste_for_dev(struct iommu_fwspec *fwspec)arm_smmu_write_strtab_ent (smmu, sid, step, &master->ste); } - -return 0 ; }static void arm_smmu_detach_dev (struct device *dev)
@@ -1600,8 +1598,7 @@ static void arm_smmu_detach_dev(struct device *dev)struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv; master->ste.bypass = true; -if (arm_smmu_install_ste_for_dev (dev->iommu_fwspec) <0 ) -dev_warn (dev,"failed to install bypass STE\n" ); +arm_smmu_install_ste_for_dev (dev->iommu_fwspec); }static int arm_smmu_attach_dev (struct iommu_domain *domain,struct device *dev)
@@ -1653,10 +1650,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) ste->s2_cfg = &smmu_domain->s2_cfg; } - ret =arm_smmu_install_ste_for_dev (dev->iommu_fwspec); -if (ret <0 ) - ste->valid = false; - +arm_smmu_install_ste_for_dev (dev->iommu_fwspec); out_unlock:mutex_unlock (&smmu_domain->init_mutex);return ret;
Signature · 1 line
2.1.4
PATCH v2 3/5 iommu/arm-smmu-v3: Make arm_smmu_install_ste_for_dev return void
Hi Will,
Quoted 7 lines · Will Deacon
arm_smmu_install_ste_for_devcannot fail and always returns 0, however the fact that it returns int means that callers end up implementing redundant error handling code which complicates STE tracking and is never executed.This patch changes the return type of
arm_smmu_install_ste_for_devto avoid, to make it explicit that it cannot fail.
Did you mean "a void" or just "void" instead of "avoid"?
Quoted 55 lines
Signed-off-by Will Deacon <[email protected]>--- drivers/iommu/arm-smmu-v3.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)drivers/iommu/arm-smmu-v3.c +1
-1@@ -1579,7 +1579,7 @@ static __le64 *arm_smmu_get_step_for_sid(struct arm_smmu_device *smmu, u32 sid) return step; } -static int arm_smmu_install_ste_for_dev (struct iommu_fwspec *fwspec) +static void arm_smmu_install_ste_for_dev (struct iommu_fwspec *fwspec) {int i;drivers/iommu/arm-smmu-v3.c +0
-2struct arm_smmu_master_data *master = fwspec->iommu_priv;@@ -1591,8 +1591,6 @@ static int arm_smmu_install_ste_for_dev(struct iommu_fwspec *fwspec) arm_smmu_write_strtab_ent(smmu, sid, step, &master->ste); } - -return 0 ; }static void
arm_smmu_detach_dev(struct device *dev)drivers/iommu/arm-smmu-v3.c +1
-2@@ -1600,8 +1598,7 @@ static void arm_smmu_detach_dev(struct device *dev)struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv; master->ste.bypass = true; -if (arm_smmu_install_ste_for_dev (dev->iommu_fwspec) <0 ) -dev_warn (dev,"failed to install bypass STE\n" ); +arm_smmu_install_ste_for_dev (dev->iommu_fwspec); }static int arm_smmu_attach_dev(struct
iommu_domain*domain, struct
device *dev)drivers/iommu/arm-smmu-v3.c +1
-4@@ -1653,10 +1650,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) ste->s2_cfg = &smmu_domain->s2_cfg; } - ret =arm_smmu_install_ste_for_dev (dev->iommu_fwspec); -if (ret <0 ) - ste->valid = false; - +arm_smmu_install_ste_for_dev (dev->iommu_fwspec); out_unlock:mutex_unlock (&smmu_domain->init_mutex);return ret;
Signature · 5 lines
Qualcomm Datacenter Technologies, Inc. on behalf of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v2 4/5 iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains
In reply to Will DeaconIn preparation for allowing the default domain type to be overridden, this patch adds support for IOMMU_DOMAIN_IDENTITY domains to the ARM SMMUv3 driver.
An identity domain is created by placing the corresponding stream table entries into "bypass" mode, which allows transactions to flow through the SMMU without any translation.
--- drivers/iommu/arm-smmu-v3.c | 58 +++++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 21 deletions(-)
drivers/iommu/arm-smmu-v3.c +37 -21
@@ -554,9 +554,14 @@ struct arm_smmu_s2_cfg { };struct arm_smmu_strtab_ent { -bool valid; - -bool bypass;/* Overrides s1/s2 config */ +/* +* An STE is "assigned" if the master emitting the corresponding SID +* is attached to a domain. The behaviour of an unassigned STE is +* determined by the disable_bypass parameter, whereas an assigned +* STE behaves according to s1_cfg/s2_cfg, which themselves are +* configured according to the domain type. +*/ +bool assigned;struct arm_smmu_s1_cfg *s1_cfg;struct arm_smmu_s2_cfg *s2_cfg; };
@@ -632,6 +637,7 @@ enum arm_smmu_domain_stage {ARM_SMMU_DOMAIN_S1 =0 ,ARM_SMMU_DOMAIN_S2 ,ARM_SMMU_DOMAIN_NESTED , + ARM_SMMU_DOMAIN_BYPASS, }; struct arm_smmu_domain {
@@ -1005,9 +1011,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,* This is hideously complicated, but we only really care about * three cases at the moment: * -* 1. Invalid (all zero) -> bypass (init) -* 2. Bypass -> translation (attach) -* 3. Translation -> bypass (detach) +* 1. Invalid (all zero) -> bypass/fault (init) +* 2. Bypass/fault -> translation/bypass (attach) +* 3. Translation/bypass -> bypass/fault (detach) * * Given that we can't update the STE atomically and the SMMU * doesn't read the thing in a defined order, that leaves us
@@ -1046,11 +1052,15 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, }/* Nuke the existing STE_0 value, as we're going to rewrite it */ - val = ste->valid ?STRTAB_STE_0_V :0 ; + val =STRTAB_STE_0_V ; + +/* Bypass/fault */ +if (!ste->assigned || !(ste->s1_cfg || ste->s2_cfg)) { +if (!ste->assigned && disable_bypass) + val |=STRTAB_STE_0_CFG_ABORT ; +else + val |=STRTAB_STE_0_CFG_BYPASS ; -if (ste->bypass) { - val |= disable_bypass ?STRTAB_STE_0_CFG_ABORT - :STRTAB_STE_0_CFG_BYPASS ; dst[0 ] =cpu_to_le64 (val); dst[1 ] =cpu_to_le64 (STRTAB_STE_1_SHCFG_INCOMING <<STRTAB_STE_1_SHCFG_SHIFT );
@@ -1111,10 +1121,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,static void arm_smmu_init_bypass_stes (u64 *strtab,unsigned int nent) {unsigned int i; -struct arm_smmu_strtab_ent ste = { - .valid = true, - .bypass = true, - }; +struct arm_smmu_strtab_ent ste = { .assigned = false };for (i =0 ; i < nent; ++i) {arm_smmu_write_strtab_ent (NULL ,-1 , strtab, &ste);
@@ -1378,7 +1385,9 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) {struct arm_smmu_domain *smmu_domain; -if (type !=IOMMU_DOMAIN_UNMANAGED && type !=IOMMU_DOMAIN_DMA ) +if (type !=IOMMU_DOMAIN_UNMANAGED && + type !=IOMMU_DOMAIN_DMA && + type !=IOMMU_DOMAIN_IDENTITY )return NULL ;/*
@@ -1509,6 +1518,11 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain);struct arm_smmu_device *smmu = smmu_domain->smmu; +if (domain->type ==IOMMU_DOMAIN_IDENTITY ) { + smmu_domain->stage =ARM_SMMU_DOMAIN_BYPASS ; +return 0 ; + } +/* Restrict the stage to what we can actually support */ if (!(smmu->features &ARM_SMMU_FEAT_TRANS_S1 )) smmu_domain->stage =ARM_SMMU_DOMAIN_S2 ;
@@ -1597,7 +1611,7 @@ static void arm_smmu_detach_dev(struct device *dev) {struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv; - master->ste.bypass = true; + master->ste.assigned = false;arm_smmu_install_ste_for_dev (dev->iommu_fwspec); }
@@ -1617,7 +1631,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) ste = &master->ste;/* Already attached to a different domain? */ -if (!ste->bypass) +if (ste->assigned)arm_smmu_detach_dev (dev);mutex_lock (&smmu_domain->init_mutex);
@@ -1638,10 +1652,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) goto out_unlock; } - ste->bypass = false; - ste->valid = true; + ste->assigned = true; -if (smmu_domain->stage ==ARM_SMMU_DOMAIN_S1 ) { +if (smmu_domain->stage ==ARM_SMMU_DOMAIN_BYPASS ) { + ste->s1_cfg =NULL ; + ste->s2_cfg =NULL ; + }else if (smmu_domain->stage ==ARM_SMMU_DOMAIN_S1 ) { ste->s1_cfg = &smmu_domain->s1_cfg; ste->s2_cfg =NULL ;arm_smmu_write_ctx_desc (smmu, ste->s1_cfg);
@@ -1801,7 +1817,7 @@ static void arm_smmu_remove_device(struct device *dev) master = fwspec->iommu_priv; smmu = master->smmu; -if (master && master->ste.valid) +if (master && master->ste.assigned)arm_smmu_detach_dev (dev);iommu_group_remove_device (dev);iommu_device_unlink (&smmu->iommu, dev);
Signature · 1 line
2.1.4
PATCH v2 4/5 iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains
Hi Will,
Quoted 8 lines · Will Deacon
In preparation for allowing the default domain type to be overridden, this patch adds support for
IOMMU_DOMAIN_IDENTITYdomains to the ARM SMMUv3 driver.An identity domain is created by placing the corresponding stream table entries into "bypass" mode, which allows transactions to flow through the SMMU without any translation.
What about masters that require SMMU intervention to override their native memory attributes to make them consistent with the CCA (acpi) or dma-coherent (dt) values specified in FW? To make sure those cases are handled, you could store away the master's coherency setting in its strtab_ent at attach time and then setup STE[MemAttr/ALLOCCFG/SHCFG] so the attributes are forced to the correct values while still bypassing translation.
Quoted 156 lines
Signed-off-by Will Deacon <[email protected]>--- drivers/iommu/arm-smmu-v3.c | 58 +++++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 21 deletions(-)drivers/iommu/arm-smmu-v3.c +12
-6@@ -554,9 +554,14 @@ struct arm_smmu_s2_cfg { };struct arm_smmu_strtab_ent { -bool valid; - -bool bypass;/* Overrides s1/s2 config */ +/* +* An STE is "assigned" if the master emitting the corresponding SID +* is attached to a domain. The behaviour of an unassigned STE is +* determined by the disable_bypass parameter, whereas an assigned +* STE behaves according to s1_cfg/s2_cfg, which themselves are +* configured according to the domain type. +*/ +bool assigned;struct arm_smmu_s1_cfg *s1_cfg;struct arm_smmu_s2_cfg *s2_cfg; };@@ -632,6 +637,7 @@ enum arm_smmu_domain_stage {ARM_SMMU_DOMAIN_S1 =0 ,ARM_SMMU_DOMAIN_S2 ,ARM_SMMU_DOMAIN_NESTED , + ARM_SMMU_DOMAIN_BYPASS, }; struct arm_smmu_domain {@@ -1005,9 +1011,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, * This is hideously complicated, but we only really care about * three cases at the moment: * - * 1. Invalid (all zero) -> bypass (init) - * 2. Bypass -> translation (attach) - * 3. Translation -> bypass (detach) + * 1. Invalid (all zero) -> bypass/fault (init) + * 2. Bypass/fault -> translation/bypass (attach) + * 3. Translation/bypass -> bypass/fault (detach) * * Given that we can't update the STE atomically and the SMMUdrivers/iommu/arm-smmu-v3.c +8
-4* doesn't read the thing in a defined order, that leaves us @@ -1046,11 +1052,15 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, }/* Nuke the existing STE_0 value, as we're going to rewrite it */ - val = ste->valid ?STRTAB_STE_0_V :0 ; + val =STRTAB_STE_0_V ; + +/* Bypass/fault */ +if (!ste->assigned || !(ste->s1_cfg || ste->s2_cfg)) { +if (!ste->assigned && disable_bypass) + val |=STRTAB_STE_0_CFG_ABORT ; +else + val |=STRTAB_STE_0_CFG_BYPASS ; -if (ste->bypass) { - val |= disable_bypass ?STRTAB_STE_0_CFG_ABORT - :STRTAB_STE_0_CFG_BYPASS ; dst[0 ] =cpu_to_le64 (val); dst[1] = cpu_to_le64(STRTAB_STE_1_SHCFG_INCOMINGdrivers/iommu/arm-smmu-v3.c +1
-4<< STRTAB_STE_1_SHCFG_SHIFT);@@ -1111,10 +1121,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, static void arm_smmu_init_bypass_stes(u64 *strtab, unsigned int nent) { unsigned int i; -struct arm_smmu_strtab_ent ste = { - .valid = true, - .bypass = true, - }; +struct arm_smmu_strtab_ent ste = { .assigned = false };for (i =0 ; i < nent; ++i) {drivers/iommu/arm-smmu-v3.c +3
-1arm_smmu_write_strtab_ent (NULL ,-1 , strtab, &ste);@@ -1378,7 +1385,9 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) {struct arm_smmu_domain *smmu_domain; -if (type !=IOMMU_DOMAIN_UNMANAGED && type !=IOMMU_DOMAIN_DMA ) +if (type !=IOMMU_DOMAIN_UNMANAGED && + type !=IOMMU_DOMAIN_DMA && + type !=IOMMU_DOMAIN_IDENTITY )return NULL ;drivers/iommu/arm-smmu-v3.c +5
-0/* @@ -1509,6 +1518,11 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain);struct arm_smmu_device *smmu = smmu_domain->smmu; +if (domain->type ==IOMMU_DOMAIN_IDENTITY ) { + smmu_domain->stage =ARM_SMMU_DOMAIN_BYPASS ; +return 0 ; + } +/* Restrict the stage to what we can actually support */ if (!(smmu->features &ARM_SMMU_FEAT_TRANS_S1 ))drivers/iommu/arm-smmu-v3.c +2
-2smmu_domain->stage =ARM_SMMU_DOMAIN_S2 ;@@ -1597,7 +1611,7 @@ static void arm_smmu_detach_dev(struct device *dev) {struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv; - master->ste.bypass = true; + master->ste.assigned = false;arm_smmu_install_ste_for_dev (dev->iommu_fwspec); }@@ -1617,7 +1631,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) ste = &master->ste;/* Already attached to a different domain? */ -if (!ste->bypass) +if (ste->assigned)arm_smmu_detach_dev (dev);drivers/iommu/arm-smmu-v3.c +5
-3mutex_lock (&smmu_domain->init_mutex);@@ -1638,10 +1652,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) goto out_unlock; } - ste->bypass = false; - ste->valid = true; + ste->assigned = true; -if (smmu_domain->stage ==ARM_SMMU_DOMAIN_S1 ) { +if (smmu_domain->stage ==ARM_SMMU_DOMAIN_BYPASS ) { + ste->s1_cfg =NULL ; + ste->s2_cfg =NULL ; + }else if (smmu_domain->stage ==ARM_SMMU_DOMAIN_S1 ) { ste->s1_cfg = &smmu_domain->s1_cfg; ste->s2_cfg =NULL ;drivers/iommu/arm-smmu-v3.c +1
-1arm_smmu_write_ctx_desc (smmu, ste->s1_cfg);@@ -1801,7 +1817,7 @@ static void arm_smmu_remove_device(struct device *dev) master = fwspec->iommu_priv; smmu = master->smmu; -if (master && master->ste.valid) +if (master && master->ste.assigned)arm_smmu_detach_dev (dev);iommu_group_remove_device (dev);
iommu_device_unlink(&smmu->iommu, dev);
Signature · 5 lines
Qualcomm Datacenter Technologies, Inc. on behalf of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Robin Murphy
reviewerPATCH v2 4/5 iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains
Hi Nate, Will,
Quoted 7 lines · Nate Watterson
Hi Will,
Quoted 4 lines · Will Deacon
In preparation for allowing the default domain type to be overridden, this patch adds support for
IOMMU_DOMAIN_IDENTITYdomains to the ARM SMMUv3 driver.
Quoted 8 lines
Quoted 4 lines
An identity domain is created by placing the corresponding stream table entries into "bypass" mode, which allows transactions to flow through the SMMU without any translation.
What about masters that require SMMU intervention to override their native memory attributes to make them consistent with the CCA (acpi) or dma-coherent (dt) values specified in FW?
Well, we've already broken them ;) My interpretation of "dma-coherent" is as the equivalent of DACS=1,CPM=1, i.e. not dependent on SMMU override. For the CCA=1,DACS=0 case (I'm going to pretend the DT equivalent will never exist...) the first problem to solve is how to inherit the appropriate configuration from the firmware, because right now we're not even pretending to support that.
Quoted 5 lines
To make sure those cases
are handled, you could store away the master's coherency setting in itsstrtab_entat attach time and then setup STE[MemAttr/ALLOCCFG/SHCFG] so the attributes are forced to the correct values while still bypassing translation.
However, for this particular piece of the puzzle, that does sound about right - the attribute overrides are pretty much orthogonal to the stage of translation (or bypass), so the master's strtab_ent will indeed probably be the most appropriate place to keep them once we get there (cf. the master's s2cr in SMMUv2).
Now, while I'm here...
Quoted 54 lines
Quoted 54 lines
Signed-off-by Will Deacon <[email protected]>--- drivers/iommu/arm-smmu-v3.c | 58 +++++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 21 deletions(-)drivers/iommu/arm-smmu-v3.c +12
-6@@ -554,9 +554,14 @@ struct arm_smmu_s2_cfg { };struct arm_smmu_strtab_ent { -bool valid; - -bool bypass;/* Overrides s1/s2 config */ +/* +* An STE is "assigned" if the master emitting the corresponding SID +* is attached to a domain. The behaviour of an unassigned STE is +* determined by the disable_bypass parameter, whereas an assigned +* STE behaves according to s1_cfg/s2_cfg, which themselves are +* configured according to the domain type. +*/ +bool assigned;struct arm_smmu_s1_cfg *s1_cfg;struct arm_smmu_s2_cfg *s2_cfg; };@@ -632,6 +637,7 @@ enum arm_smmu_domain_stage {ARM_SMMU_DOMAIN_S1 =0 ,ARM_SMMU_DOMAIN_S2 ,ARM_SMMU_DOMAIN_NESTED , + ARM_SMMU_DOMAIN_BYPASS, }; struct arm_smmu_domain {@@ -1005,9 +1011,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, * This is hideously complicated, but we only really care about * three cases at the moment: * - * 1. Invalid (all zero) -> bypass (init) - * 2. Bypass -> translation (attach) - * 3. Translation -> bypass (detach) + * 1. Invalid (all zero) -> bypass/fault (init) + * 2. Bypass/fault -> translation/bypass (attach) + * 3. Translation/bypass -> bypass/fault (detach) * * Given that we can't update the STE atomically and the SMMU* doesn't read the thing in a defined order, that leaves us
drivers/iommu/arm-smmu-v3.c +0
-0@@ -1046,11 +1052,15 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, }
Quoted 7 lines
drivers/iommu/arm-smmu-v3.c static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, · line 1059
Quoted 7 lines
drivers/iommu/arm-smmu-v3.c +5
-1/* Nuke the existing STE_0 value, as we're going to rewrite it */ - val = ste->valid ?STRTAB_STE_0_V :0 ; + val =STRTAB_STE_0_V ; + +/* Bypass/fault */ +if (!ste->assigned || !(ste->s1_cfg || ste->s2_cfg)) { +if (!ste->assigned && disable_bypass)
...yuck. After about 5 minutes of staring at that, I've convinced myself that it would make much more sense to always clear the strtab_ent configs on detach, such that you never need the outer !ste->assigned check here...
Quoted 76 lines
Quoted 76 lines
drivers/iommu/arm-smmu-v3.c +3
-3+ val |=STRTAB_STE_0_CFG_ABORT ; + else + val |= STRTAB_STE_0_CFG_BYPASS; -if (ste->bypass) { - val |= disable_bypass ?STRTAB_STE_0_CFG_ABORT - :STRTAB_STE_0_CFG_BYPASS ; dst[0 ] =cpu_to_le64 (val); dst[1] = cpu_to_le64(STRTAB_STE_1_SHCFG_INCOMING<< STRTAB_STE_1_SHCFG_SHIFT);drivers/iommu/arm-smmu-v3.c +1
-4@@ -1111,10 +1121,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, static void arm_smmu_init_bypass_stes(u64 *strtab, unsigned int nent) { unsigned int i; -struct arm_smmu_strtab_ent ste = { - .valid = true, - .bypass = true, - }; +struct arm_smmu_strtab_ent ste = { .assigned = false };for (i =0 ; i < nent; ++i) {arm_smmu_write_strtab_ent(NULL, -1, strtab, &ste);drivers/iommu/arm-smmu-v3.c +3
-1@@ -1378,7 +1385,9 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) {struct arm_smmu_domain *smmu_domain; -if (type !=IOMMU_DOMAIN_UNMANAGED && type !=IOMMU_DOMAIN_DMA ) +if (type !=IOMMU_DOMAIN_UNMANAGED && + type !=IOMMU_DOMAIN_DMA && + type !=IOMMU_DOMAIN_IDENTITY )return NULL ;/*drivers/iommu/arm-smmu-v3.c +5
-0@@ -1509,6 +1518,11 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain);struct arm_smmu_device *smmu = smmu_domain->smmu; +if (domain->type ==IOMMU_DOMAIN_IDENTITY ) { + smmu_domain->stage =ARM_SMMU_DOMAIN_BYPASS ; +return 0 ; + } +/* Restrict the stage to what we can actually support */ if (!(smmu->features &ARM_SMMU_FEAT_TRANS_S1 ))smmu_domain->stage = ARM_SMMU_DOMAIN_S2;drivers/iommu/arm-smmu-v3.c +2
-2@@ -1597,7 +1611,7 @@ static void arm_smmu_detach_dev(struct device *dev) {struct arm_smmu_master_data *master = dev->iommu_fwspec->iommu_priv; - master->ste.bypass = true; + master->ste.assigned = false;arm_smmu_install_ste_for_dev (dev->iommu_fwspec); }@@ -1617,7 +1631,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) ste = &master->ste;/* Already attached to a different domain? */ -if (!ste->bypass) +if (ste->assigned)arm_smmu_detach_dev (dev);mutex_lock(&smmu_domain->init_mutex);drivers/iommu/arm-smmu-v3.c +1
-2@@ -1638,10 +1652,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) goto out_unlock; } - ste->bypass = false; - ste->valid = true; + ste->assigned = true;
Quoted 7 lines
drivers/iommu/arm-smmu-v3.c static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) · line 1662
Quoted 7 lines
drivers/iommu/arm-smmu-v3.c +4
-1-if (smmu_domain->stage ==ARM_SMMU_DOMAIN_S1 ) { +if (smmu_domain->stage ==ARM_SMMU_DOMAIN_BYPASS ) { + ste->s1_cfg =NULL ; + ste->s2_cfg =NULL ; + }else if (smmu_domain->stage ==ARM_SMMU_DOMAIN_S1 ) { ste->s1_cfg = &smmu_domain->s1_cfg; ste->s2_cfg =NULL ;
...or all these explicit NULL assignments (and indeed the entire ARM_SMMU_DOMAIN_BYPASS case) here. Deliberately keeping potentially stale context pointers hanging around in unassigned strtab_ents seems silly (it might trick kmemleak, for another thing).
Robin.
Quoted 12 lines
Quoted 11 lines
arm_smmu_write_ctx_desc(smmu, ste->s1_cfg);drivers/iommu/arm-smmu-v3.c +1
-1@@ -1801,7 +1817,7 @@ static void arm_smmu_remove_device(struct device *dev) master = fwspec->iommu_priv; smmu = master->smmu; -if (master && master->ste.valid) +if (master && master->ste.assigned)arm_smmu_detach_dev (dev);iommu_group_remove_device (dev);iommu_device_unlink(&smmu->iommu, dev);
PATCH v2 4/5 iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains
Hi Robin,
Quoted 15 lines · Robin Murphy
Quoted 5 lines · Nate Watterson
Quoted 4 lines · Will Deacon
In preparation for allowing the default domain type to be overridden, this patch adds support for
IOMMU_DOMAIN_IDENTITYdomains to the ARM SMMUv3 driver.Quoted 8 lines
Quoted 4 lines
An identity domain is created by placing the corresponding stream table entries into "bypass" mode, which allows transactions to flow through the SMMU without any translation.
What about masters that require SMMU intervention to override their native memory attributes to make them consistent with the CCA (acpi) or dma-coherent (dt) values specified in FW?
Quoted 6 lines
Well, we've already broken them ;) My interpretation of "dma-coherent" is as the equivalent of DACS=1,CPM=1, i.e. not dependent on SMMU override. For the CCA=1,DACS=0 case (I'm going to pretend the DT equivalent will never exist...) the first problem to solve is how to inherit the appropriate configuration from the firmware, because right now we're not even pretending to support that.
Indeed, and that would need to be added as a separate patch series when the need arises.
Quoted 8 lines
Quoted 7 lines
drivers/iommu/arm-smmu-v3.c static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, · line 1059
Quoted 7 lines
/* Nuke the existing STE_0 value, as we're going to rewrite it */drivers/iommu/arm-smmu-v3.c +5
-1- val = ste->valid ?STRTAB_STE_0_V :0 ; + val =STRTAB_STE_0_V ; + +/* Bypass/fault */ +if (!ste->assigned || !(ste->s1_cfg || ste->s2_cfg)) { +if (!ste->assigned && disable_bypass)
Quoted 4 lines
...yuck. After about 5 minutes of staring at that, I've convinced myself that it would make much more sense to always clear the
strtab_entconfigs on detach, such that you never need the outer !ste->assigned check here...
I was deliberately keeping the strtab_ent intact in case we ever grow support for nested translation, where we might well want to detach a stage 1 but keep the stage 2 installed. I don't think the code is that bad, so I'd like to leave it like it is for now.
Will
Robin Murphy
reviewerPATCH v2 4/5 iommu/arm-smmu-v3: Install bypass STEs for IOMMU_DOMAIN_IDENTITY domains
Quoted 41 lines · Will Deacon
Hi Robin,
Quoted 15 lines · Robin Murphy
Quoted 5 lines · Nate Watterson
Quoted 4 lines · Will Deacon
In preparation for allowing the default domain type to be overridden, this patch adds support for
IOMMU_DOMAIN_IDENTITYdomains to the ARM SMMUv3 driver.Quoted 8 lines
Quoted 4 lines
An identity domain is created by placing the corresponding stream table entries into "bypass" mode, which allows transactions to flow through the SMMU without any translation.
What about masters that require SMMU intervention to override their native memory attributes to make them consistent with the CCA (acpi) or dma-coherent (dt) values specified in FW?
Quoted 6 lines
Well, we've already broken them ;) My interpretation of "dma-coherent" is as the equivalent of DACS=1,CPM=1, i.e. not dependent on SMMU override. For the CCA=1,DACS=0 case (I'm going to pretend the DT equivalent will never exist...) the first problem to solve is how to inherit the appropriate configuration from the firmware, because right now we're not even pretending to support that.
Indeed, and that would need to be added as a separate patch series when the need arises.
Quoted 8 lines
Quoted 7 lines
drivers/iommu/arm-smmu-v3.c static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid, · line 1059
Quoted 7 lines
/* Nuke the existing STE_0 value, as we're going to rewrite it */drivers/iommu/arm-smmu-v3.c +5
-1- val = ste->valid ?STRTAB_STE_0_V :0 ; + val =STRTAB_STE_0_V ; + +/* Bypass/fault */ +if (!ste->assigned || !(ste->s1_cfg || ste->s2_cfg)) { +if (!ste->assigned && disable_bypass)Quoted 4 lines
...yuck. After about 5 minutes of staring at that, I've convinced myself that it would make much more sense to always clear the
strtab_entconfigs on detach, such that you never need the outer !ste->assigned check here...
Quoted 4 lines
I was deliberately keeping the
strtab_entintact in case we ever grow support for nested translation, where we might well want to detach a stage 1 but keep the stage 2 installed. I don't think the code is that bad, so I'd like to leave it like it is for now.
Sure, it would certainly be more awkward to recreate this logic from scratch in future if we need it again. I suggested the cleanup since it looked like an oversight, but if it's a conscious decision then that's fine by me.
Robin.
Quoted 3 lines
Will
PATCH v2 5/5 iommu: Allow default domain type to be set on the kernel command line
In reply to Will DeaconThe IOMMU core currently initialises the default domain for each group to IOMMU_DOMAIN_DMA, under the assumption that devices will use IOMMU-backed DMA ops by default. However, in some cases it is desirable for the DMA ops to bypass the IOMMU for performance reasons, reserving use of translation for subsystems such as VFIO that require it for enforcing device isolation.
Rather than modify each IOMMU driver to provide different semantics for DMA domains, instead we introduce a command line parameter that can be used to change the type of the default domain. Passthrough can then be specified using "iommu.passthrough=1" on the kernel command line.
--- Documentation/admin-guide/kernel-parameters.txt | 6 ++++++ drivers/iommu/iommu.c | 17 +++++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-)
Documentation/admin-guide/kernel-parameters.txt +6 -0
@@ -1635,6 +1635,12 @@ nobypass [PPC/POWERNV] Disable IOMMU bypass, using IOMMU for PCI devices. + iommu.passthrough= + [ARM64] Configure DMA to bypass the IOMMU by default. + Format: { "0" | "1" } + 0 - Use IOMMU translation for DMA. + 1 - Bypass the IOMMU for DMA. + unset - Use IOMMU translation for DMA. io7= [HW] IO7 for Marvel based alpha systems See comment before marvel_specify_io7 in
drivers/iommu/iommu.c +15 -2
@@ -36,6 +36,7 @@static struct kset *iommu_group_kset;static DEFINE_IDA (iommu_group_ida); +static unsigned int iommu_def_domain_type =IOMMU_DOMAIN_DMA ;struct iommu_callback_data {const struct iommu_ops *ops;
@@ -111,6 +112,18 @@ static int __iommu_attach_group(struct iommu_domain *domain,static void __iommu_detach_group (struct iommu_domain *domain,struct iommu_group *group); +static int __init iommu_set_def_domain_type (char *str) +{ +bool pt; + +if (!str ||strtobool (str, &pt)) +return -EINVAL ; + + iommu_def_domain_type = pt ?IOMMU_DOMAIN_IDENTITY :IOMMU_DOMAIN_DMA ; +return 0 ; +} +early_param ("iommu.passthrough" , iommu_set_def_domain_type); +static ssize_t iommu_group_attr_show (struct kobject *kobj,struct attribute *__attr,char *buf) {
@@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)* IOMMU driver. */ if (!group->default_domain) { - group->default_domain =__iommu_domain_alloc (dev->bus, -IOMMU_DOMAIN_DMA ); + group->default_domain = +__iommu_domain_alloc (dev->bus, iommu_def_domain_type);if (!group->domain) group->domain = group->default_domain; }
Signature · 1 line
2.1.4
Joerg Roedel
maintainerPATCH v2 5/5 iommu: Allow default domain type to be set on the kernel command line
Hi Will,
drivers/iommu/iommu.c struct iommu_group *iommu_group_get_for_dev(struct device *dev) · line 1031
Quoted 8 lines · Will Deacon
drivers/iommu/iommu.c +2
-2@@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)* IOMMU driver. */ if (!group->default_domain) { - group->default_domain =__iommu_domain_alloc (dev->bus, -IOMMU_DOMAIN_DMA ); + group->default_domain = +__iommu_domain_alloc (dev->bus, iommu_def_domain_type);
It would be good to have a fall-back here if we are talking to an IOMMU driver that uses default domains, but does not support identity-mapped domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this category. A dev_warn() also makes sense in case allocating a identity domain fails.
Joerg
PATCH v2 5/5 iommu: Allow default domain type to be set on the kernel command line
Quoted 15 lines · Joerg Roedel
Review struct iommu_group *iommu_group_get_for_dev(struct device *dev) · line 1031
Quoted 8 lines · Will Deacon
(quoted diff) +2
-2@@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev) * IOMMU driver. */ if (!group->default_domain) { - group->default_domain = __iommu_domain_alloc(dev->bus, - IOMMU_DOMAIN_DMA); + group->default_domain = + __iommu_domain_alloc(dev->bus, iommu_def_domain_type);It would be good to have a fall-back here if we are talking to an IOMMU driver that uses default domains, but does not support identity-mapped domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this category. A
dev_warn()also makes sense in case allocating a identity domain fails.
Sure, something like the diff below?
Will
--->8
drivers/iommu/iommu.c +12 -3
@@ -1027,10 +1027,19 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)* IOMMU driver. */ if (!group->default_domain) { - group->default_domain = -__iommu_domain_alloc (dev->bus, iommu_def_domain_type); +struct iommu_domain *dom; + + dom =__iommu_domain_alloc (dev->bus, iommu_def_domain_type); +if (!dom) { +dev_warn (dev, +"failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA" , + iommu_def_domain_type); + dom =__iommu_domain_alloc (dev->bus,IOMMU_DOMAIN_DMA ); + } + + group->default_domain = dom;if (!group->domain) - group->domain = group->default_domain; + group->domain = dom; } ret =iommu_group_add_device (group, dev);
Robin Murphy
reviewerPATCH v2 5/5 iommu: Allow default domain type to be set on the kernel command line
Quoted 31 lines · Will Deacon
Quoted 15 lines · Joerg Roedel
Review struct iommu_group *iommu_group_get_for_dev(struct device *dev) · line 1031
Quoted 8 lines · Will Deacon
(quoted diff) +2
-2@@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev) * IOMMU driver. */ if (!group->default_domain) { - group->default_domain = __iommu_domain_alloc(dev->bus, - IOMMU_DOMAIN_DMA); + group->default_domain = + __iommu_domain_alloc(dev->bus, iommu_def_domain_type);It would be good to have a fall-back here if we are talking to an IOMMU driver that uses default domains, but does not support identity-mapped domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this category. A
dev_warn()also makes sense in case allocating a identity domain fails.Sure, something like the diff below?
Will
--->8
drivers/iommu/iommu.c +0
-0@@ -1027,10 +1027,19 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)* IOMMU driver. */
drivers/iommu/iommu.c struct iommu_group *iommu_group_get_for_dev(struct device *dev) · line 1036
Quoted 10 lines
drivers/iommu/iommu.c +7
-2if (!group->default_domain) { - group->default_domain = -__iommu_domain_alloc (dev->bus, iommu_def_domain_type); +struct iommu_domain *dom; + + dom =__iommu_domain_alloc (dev->bus, iommu_def_domain_type); +if (!dom) { +dev_warn (dev, +"failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA" , + iommu_def_domain_type);
Conversely, that's going to be noisy if iommu_def_domain_type was IOMMU_DOMAIN_DMA to begin with. I think it makes sense to warn if the user asked for a specific default domain type on the command line and that didn't work, but maybe not to bother otherwise. Plus, if they asked for passthrough, then not allocating a default domain at all is probably closer to the desired result than installing a DMA ops domain would be.
Robin.
Quoted 11 lines
drivers/iommu/iommu.c +5
-1+ dom =__iommu_domain_alloc (dev->bus,IOMMU_DOMAIN_DMA ); + } + + group->default_domain = dom;if (!group->domain) - group->domain = group->default_domain; + group->domain = dom; } ret =iommu_group_add_device (group, dev);
PATCH v2 5/5 iommu: Allow default domain type to be set on the kernel command line
Quoted 41 lines · Robin Murphy
Quoted 40 lines · Will Deacon
Quoted 15 lines · Joerg Roedel
Review struct iommu_group *iommu_group_get_for_dev(struct device *dev) · line 1031
Quoted 8 lines · Will Deacon
(quoted diff) +2
-2@@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev) * IOMMU driver. */ if (!group->default_domain) { - group->default_domain = __iommu_domain_alloc(dev->bus, - IOMMU_DOMAIN_DMA); + group->default_domain = + __iommu_domain_alloc(dev->bus, iommu_def_domain_type);It would be good to have a fall-back here if we are talking to an IOMMU driver that uses default domains, but does not support identity-mapped domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this category. A
dev_warn()also makes sense in case allocating a identity domain fails.Sure, something like the diff below?
Will
--->8
drivers/iommu/iommu.c +6
-2@@ -1027,10 +1027,19 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)* IOMMU driver. */ if (!group->default_domain) { - group->default_domain = -__iommu_domain_alloc (dev->bus, iommu_def_domain_type); +struct iommu_domain *dom; + + dom =__iommu_domain_alloc (dev->bus, iommu_def_domain_type); +if (!dom) { + dev_warn(dev, + "failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA",
Quoted 8 lines
drivers/iommu/iommu.c struct iommu_group *iommu_group_get_for_dev(struct device *dev) · line 1036
Quoted 1 line
drivers/iommu/iommu.c +1
-0+ iommu_def_domain_type);Conversely, that's going to be noisy if
iommu_def_domain_typewasIOMMU_DOMAIN_DMAto begin with. I think it makes sense to warn if the user asked for a specific default domain type on the command line and that didn't work, but maybe not to bother otherwise. Plus, if they asked for passthrough, then not allocating a default domain at all is probably closer to the desired result than installing a DMA ops domain would be.
You're right -- I'll hack this about to check if the default type isn't DOMAIN_DMA before warning about the allocation failure.
Cheers,
Will
PATCH v2 5/5 iommu: Allow default domain type to be set on the kernel command line
Hi,
Quoted 51 lines · Will Deacon
Quoted 41 lines · Robin Murphy
Quoted 40 lines · Will Deacon
Quoted 15 lines · Joerg Roedel
Review struct iommu_group *iommu_group_get_for_dev(struct device *dev) · line 1031
Quoted 8 lines · Will Deacon
(quoted diff) +2
-2@@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev) * IOMMU driver. */ if (!group->default_domain) { - group->default_domain = __iommu_domain_alloc(dev->bus, - IOMMU_DOMAIN_DMA); + group->default_domain = + __iommu_domain_alloc(dev->bus, iommu_def_domain_type);It would be good to have a fall-back here if we are talking to an IOMMU driver that uses default domains, but does not support identity-mapped domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this category. A
dev_warn()also makes sense in case allocating a identity domain fails.Sure, something like the diff below?
Will
--->8
drivers/iommu/iommu.c +6
-2@@ -1027,10 +1027,19 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)* IOMMU driver. */ if (!group->default_domain) { - group->default_domain = -__iommu_domain_alloc (dev->bus, iommu_def_domain_type); +struct iommu_domain *dom; + + dom =__iommu_domain_alloc (dev->bus, iommu_def_domain_type); +if (!dom) { + dev_warn(dev, + "failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA",Quoted 8 lines
drivers/iommu/iommu.c struct iommu_group *iommu_group_get_for_dev(struct device *dev) · line 1036
Quoted 1 line
drivers/iommu/iommu.c +1
-0+ iommu_def_domain_type);Conversely, that's going to be noisy if
iommu_def_domain_typewasIOMMU_DOMAIN_DMAto begin with. I think it makes sense to warn if the user asked for a specific default domain type on the command line and that didn't work, but maybe not to bother otherwise. Plus, if they asked for passthrough, then not allocating a default domain at all is probably closer to the desired result than installing a DMA ops domain would be.
Quoted 2 lines
You're right -- I'll hack this about to check if the default type isn't
DOMAIN_DMAbefore warning about the allocation failure.
if some master devices want 'IDENTITY_DOMAIN' as default (because those devices do not want any iommu resources to be used/dma_ops to be set) and some 'DMA_DOMAIN' as default, then should the default be 'DMA_DOMAIN' and then masters needing IDENTITY_DOMAIN explicitly do an detach_dev later. This [1] was adding the support for detach_dev of the default DMA_DOMAINs.
[1] https://patchwork.codeaurora.org/patch/164933/
Regards,
Sricharan
PATCH v2 5/5 iommu: Allow default domain type to be set on the kernel command line
In reply to Will DeaconHi,
Replying again, as there was some issue with mailer last time.
Quoted 51 lines · Will Deacon
Quoted 41 lines · Robin Murphy
Quoted 40 lines · Will Deacon
Quoted 15 lines · Joerg Roedel
Review struct iommu_group *iommu_group_get_for_dev(struct device *dev) · line 1031
Quoted 8 lines · Will Deacon
(quoted diff) +2
-2@@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev) * IOMMU driver. */ if (!group->default_domain) { - group->default_domain = __iommu_domain_alloc(dev->bus, - IOMMU_DOMAIN_DMA); + group->default_domain = + __iommu_domain_alloc(dev->bus, iommu_def_domain_type);It would be good to have a fall-back here if we are talking to an IOMMU driver that uses default domains, but does not support identity-mapped domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this category. A
dev_warn()also makes sense in case allocating a identity domain fails.Sure, something like the diff below?
Will
--->8
drivers/iommu/iommu.c +6
-2@@ -1027,10 +1027,19 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)* IOMMU driver. */ if (!group->default_domain) { - group->default_domain = -__iommu_domain_alloc (dev->bus, iommu_def_domain_type); +struct iommu_domain *dom; + + dom =__iommu_domain_alloc (dev->bus, iommu_def_domain_type); +if (!dom) { + dev_warn(dev, + "failed to allocate default IOMMU domain of type %u; falling back to IOMMU_DOMAIN_DMA",Quoted 8 lines
drivers/iommu/iommu.c struct iommu_group *iommu_group_get_for_dev(struct device *dev) · line 1036
Quoted 1 line
drivers/iommu/iommu.c +1
-0+ iommu_def_domain_type);Conversely, that's going to be noisy if
iommu_def_domain_typewasIOMMU_DOMAIN_DMAto begin with. I think it makes sense to warn if the user asked for a specific default domain type on the command line and that didn't work, but maybe not to bother otherwise. Plus, if they asked for passthrough, then not allocating a default domain at all is probably closer to the desired result than installing a DMA ops domain would be.
Quoted 2 lines
You're right -- I'll hack this about to check if the default type isn't
DOMAIN_DMAbefore warning about the allocation failure.
if some master devices want 'IDENTITY_DOMAIN' as default (because those devices do not want any iommu resources to be used/dma_ops to be set) and some 'DMA_DOMAIN' as default, then should the default be 'DMA_DOMAIN' and then masters needing IDENTITY_DOMAIN explicitly do an detach_dev later. This [1] was adding the support for detach_dev of the default DMA_DOMAINs.
[1] https://patchwork.codeaurora.org/patch/164933/
Regards,
Sricharan
Joerg Roedel
maintainerPATCH v2 5/5 iommu: Allow default domain type to be set on the kernel command line
In reply to Will DeaconQuoted 17 lines · Will Deacon
Quoted 15 lines · Joerg Roedel
Review struct iommu_group *iommu_group_get_for_dev(struct device *dev) · line 1031
Quoted 8 lines · Will Deacon
(quoted diff) +2
-2@@ -1014,8 +1027,8 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev) * IOMMU driver. */ if (!group->default_domain) { - group->default_domain = __iommu_domain_alloc(dev->bus, - IOMMU_DOMAIN_DMA); + group->default_domain = + __iommu_domain_alloc(dev->bus, iommu_def_domain_type);It would be good to have a fall-back here if we are talking to an IOMMU driver that uses default domains, but does not support identity-mapped domains (yet). Exynos and Rockchip IOMMU drivers seem to fall into this category. A
dev_warn()also makes sense in case allocating a identity domain fails.
Quoted 1 line
Sure, something like the diff below?
Yes, this looks good.
Thanks,
Joerg
Joerg Roedel
maintainerv2 cover Implement SMMU passthrough using the default domain
In reply to Will DeaconQuoted 8 lines · Will Deacon
Will Deacon (5):
iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains iommu/arm-smmu: Install bypass S2CRs forIOMMU_DOMAIN_IDENTITYdomains iommu/arm-smmu-v3: Makearm_smmu_install_ste_for_devreturn void iommu/arm-smmu-v3: Install bypass STEs forIOMMU_DOMAIN_IDENTITY
domains
iommu: Allow default domain type to be set on the kernel command line
Quoted 5 lines
Documentation/admin-guide/kernel-parameters.txt | 6 ++ drivers/iommu/arm-smmu-v3.c | 76 +++++++++++++++---------- drivers/iommu/arm-smmu.c | 26 ++++++++- drivers/iommu/iommu.c | 17 +++++- 4 files changed, 90 insertions(+), 35 deletions(-)
Besides my one comment on the last patch this series looks good to me. Do you plan to include it (with the fall-back) into your pull-request?
Joerg
v2 cover Implement SMMU passthrough using the default domain
Hi Joerg,
Quoted 9 lines · Joerg Roedel
Quoted 8 lines · Will Deacon
Will Deacon (5):
iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains iommu/arm-smmu: Install bypass S2CRs forIOMMU_DOMAIN_IDENTITYdomains iommu/arm-smmu-v3: Makearm_smmu_install_ste_for_devreturn void iommu/arm-smmu-v3: Install bypass STEs forIOMMU_DOMAIN_IDENTITY
domains
iommu: Allow default domain type to be set on the kernel command line
Quoted 8 lines
Quoted 5 lines
Documentation/admin-guide/kernel-parameters.txt | 6 ++ drivers/iommu/arm-smmu-v3.c | 76 +++++++++++++++---------- drivers/iommu/arm-smmu.c | 26 ++++++++- drivers/iommu/iommu.c | 17 +++++- 4 files changed, 90 insertions(+), 35 deletions(-)Besides my one comment on the last patch this series looks good to me. Do you plan to include it (with the fall-back) into your pull-request?
Yes, that would certainly be easiest for me, given the changes to the SMMU drivers. However, if you prefer it separately then I can do that too.
Cheers,
Will
Joerg Roedel
maintainerv2 cover Implement SMMU passthrough using the default domain
Quoted 18 lines · Will Deacon
Hi Joerg,
Quoted 15 lines · Joerg Roedel
Quoted 13 lines · Will Deacon
Will Deacon (5):
iommu/arm-smmu: Restrict domain attributes to UNMANAGED domains iommu/arm-smmu: Install bypass S2CRs forIOMMU_DOMAIN_IDENTITYdomains iommu/arm-smmu-v3: Makearm_smmu_install_ste_for_devreturn void iommu/arm-smmu-v3: Install bypass STEs forIOMMU_DOMAIN_IDENTITY
domains
iommu: Allow default domain type to be set on the kernel command lineDocumentation/admin-guide/kernel-parameters.txt | 6 ++ drivers/iommu/arm-smmu-v3.c | 76 +++++++++++++++---------- drivers/iommu/arm-smmu.c | 26 ++++++++- drivers/iommu/iommu.c | 17 +++++- 4 files changed, 90 insertions(+), 35 deletions(-)
Quoted 5 lines
Quoted 2 lines
Besides my one comment on the last patch this series looks good to me. Do you plan to include it (with the fall-back) into your pull-request?
Yes, that would certainly be easiest for me, given the changes to the SMMU drivers. However, if you prefer it separately then I can do that too.
No, just send it with you usual bulk of other smmu patches.
Thanks,
Joerg