When an iommu_domain is set to IOMMU_DOMAIN_IDENTITY, the driver would
skip the allocation of a CD table and set the CONFIG field of the STE
to STRTAB_STE_0_CFG_BYPASS. This works well for devices that only have
one substream, i.e. PASID disabled.
However, there could be a use case, for a pasid capable device, that
allows bypassing the translation at the default substream while still
enabling the pasid feature, which means the driver should not skip the
allocation of a CD table nor simply bypass the CONFIG field. Instead,
the S1DSS field should be set to STRTAB_STE_1_S1DSS_BYPASS and the
SHCFG field should be set to STRTAB_STE_1_SHCFG_INCOMING.
Add s1dss and shcfg in struct arm_smmu_s1_cfg, to allow configurations
in the finalise() to support that use case. Then, set them accordingly
depending on the iommu_domain->type and the master->ssid_bits.
Also, add STRTAB_STE_1_SHCFG_NONSHAREABLE of the default configuration
to distinguish from STRTAB_STE_1_SHCFG_INCOMING of the bypass one.
Signed-off-by: Nicolin Chen <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 +++++++++++++++---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 9b0dc3505601..8dc7934a0175 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1350,11 +1350,12 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
BUG_ON(ste_live);
dst[1] = cpu_to_le64(
- FIELD_PREP(STRTAB_STE_1_S1DSS, STRTAB_STE_1_S1DSS_SSID0) |
+ FIELD_PREP(STRTAB_STE_1_S1DSS, s1_cfg->s1dss) |
FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_WBRA) |
FIELD_PREP(STRTAB_STE_1_S1COR, STRTAB_STE_1_S1C_CACHE_WBRA) |
FIELD_PREP(STRTAB_STE_1_S1CSH, ARM_SMMU_SH_ISH) |
- FIELD_PREP(STRTAB_STE_1_STRW, strw));
+ FIELD_PREP(STRTAB_STE_1_STRW, strw) |
+ FIELD_PREP(STRTAB_STE_1_SHCFG, s1_cfg->shcfg));
if (smmu->features & ARM_SMMU_FEAT_STALLS &&
!master->stall_enabled)
@@ -2119,6 +2120,13 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
goto out_unlock;
cfg->s1cdmax = master->ssid_bits;
+ if (smmu_domain->domain.type == IOMMU_DOMAIN_IDENTITY) {
+ cfg->s1dss = STRTAB_STE_1_S1DSS_BYPASS;
+ cfg->shcfg = STRTAB_STE_1_SHCFG_INCOMING;
+ } else {
+ cfg->s1dss = STRTAB_STE_1_S1DSS_SSID0;
+ cfg->shcfg = STRTAB_STE_1_SHCFG_NONSHAREABLE;
+ }
smmu_domain->stall_enabled = master->stall_enabled;
@@ -2198,7 +2206,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) {
+ /*
+ * A master with a pasid capability might need a CD table, so only set
+ * ARM_SMMU_DOMAIN_BYPASS if IOMMU_DOMAIN_IDENTITY and non-pasid master
+ */
+ if (domain->type == IOMMU_DOMAIN_IDENTITY && !master->ssid_bits) {
smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
return 0;
}
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index dcab85698a4e..8052d02770d0 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -244,6 +244,7 @@
#define STRTAB_STE_1_STRW_EL2 2UL
#define STRTAB_STE_1_SHCFG GENMASK_ULL(45, 44)
+#define STRTAB_STE_1_SHCFG_NONSHAREABLE 0UL
#define STRTAB_STE_1_SHCFG_INCOMING 1UL
#define STRTAB_STE_2_S2VMID GENMASK_ULL(15, 0)
@@ -601,6 +602,8 @@ struct arm_smmu_s1_cfg {
struct arm_smmu_ctx_desc_cfg cdcfg;
struct arm_smmu_ctx_desc cd;
u8 s1fmt;
+ u8 s1dss;
+ u8 shcfg;
u8 s1cdmax;
};
--
2.41.0
On 2023-06-27 04:33, Nicolin Chen wrote:
> When an iommu_domain is set to IOMMU_DOMAIN_IDENTITY, the driver would
> skip the allocation of a CD table and set the CONFIG field of the STE
> to STRTAB_STE_0_CFG_BYPASS. This works well for devices that only have
> one substream, i.e. PASID disabled.
>
> However, there could be a use case, for a pasid capable device, that
> allows bypassing the translation at the default substream while still
> enabling the pasid feature, which means the driver should not skip the
> allocation of a CD table nor simply bypass the CONFIG field. Instead,
> the S1DSS field should be set to STRTAB_STE_1_S1DSS_BYPASS and the
> SHCFG field should be set to STRTAB_STE_1_SHCFG_INCOMING.
>
> Add s1dss and shcfg in struct arm_smmu_s1_cfg, to allow configurations
> in the finalise() to support that use case. Then, set them accordingly
> depending on the iommu_domain->type and the master->ssid_bits.
>
> Also, add STRTAB_STE_1_SHCFG_NONSHAREABLE of the default configuration
> to distinguish from STRTAB_STE_1_SHCFG_INCOMING of the bypass one.
Why? The "default configuration" is that the S1 shareability attribute
is determined by the S1 translation itself, so the incoming value is
irrelevant.
> Signed-off-by: Nicolin Chen <[email protected]>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 +++++++++++++++---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +++
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 9b0dc3505601..8dc7934a0175 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1350,11 +1350,12 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
>
> BUG_ON(ste_live);
> dst[1] = cpu_to_le64(
> - FIELD_PREP(STRTAB_STE_1_S1DSS, STRTAB_STE_1_S1DSS_SSID0) |
> + FIELD_PREP(STRTAB_STE_1_S1DSS, s1_cfg->s1dss) |
> FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_WBRA) |
> FIELD_PREP(STRTAB_STE_1_S1COR, STRTAB_STE_1_S1C_CACHE_WBRA) |
> FIELD_PREP(STRTAB_STE_1_S1CSH, ARM_SMMU_SH_ISH) |
> - FIELD_PREP(STRTAB_STE_1_STRW, strw));
> + FIELD_PREP(STRTAB_STE_1_STRW, strw) |
> + FIELD_PREP(STRTAB_STE_1_SHCFG, s1_cfg->shcfg));
>
> if (smmu->features & ARM_SMMU_FEAT_STALLS &&
> !master->stall_enabled)
> @@ -2119,6 +2120,13 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
> goto out_unlock;
>
> cfg->s1cdmax = master->ssid_bits;
> + if (smmu_domain->domain.type == IOMMU_DOMAIN_IDENTITY) {
> + cfg->s1dss = STRTAB_STE_1_S1DSS_BYPASS;
> + cfg->shcfg = STRTAB_STE_1_SHCFG_INCOMING;
> + } else {
> + cfg->s1dss = STRTAB_STE_1_S1DSS_SSID0;
> + cfg->shcfg = STRTAB_STE_1_SHCFG_NONSHAREABLE;
> + }
>
> smmu_domain->stall_enabled = master->stall_enabled;
>
> @@ -2198,7 +2206,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) {
> + /*
> + * A master with a pasid capability might need a CD table, so only set
> + * ARM_SMMU_DOMAIN_BYPASS if IOMMU_DOMAIN_IDENTITY and non-pasid master
> + */
> + if (domain->type == IOMMU_DOMAIN_IDENTITY && !master->ssid_bits) {
> smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> return 0;
> }
This means we'll now go on to allocate a pagetable for an identity
domain, which doesn't seem ideal :/
Thanks,
Robin.
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index dcab85698a4e..8052d02770d0 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -244,6 +244,7 @@
> #define STRTAB_STE_1_STRW_EL2 2UL
>
> #define STRTAB_STE_1_SHCFG GENMASK_ULL(45, 44)
> +#define STRTAB_STE_1_SHCFG_NONSHAREABLE 0UL
> #define STRTAB_STE_1_SHCFG_INCOMING 1UL
>
> #define STRTAB_STE_2_S2VMID GENMASK_ULL(15, 0)
> @@ -601,6 +602,8 @@ struct arm_smmu_s1_cfg {
> struct arm_smmu_ctx_desc_cfg cdcfg;
> struct arm_smmu_ctx_desc cd;
> u8 s1fmt;
> + u8 s1dss;
> + u8 shcfg;
> u8 s1cdmax;
> };
>
Hi Robin,
On Tue, Jun 27, 2023 at 10:00:18AM +0100, Robin Murphy wrote:
> On 2023-06-27 04:33, Nicolin Chen wrote:
> > When an iommu_domain is set to IOMMU_DOMAIN_IDENTITY, the driver would
> > skip the allocation of a CD table and set the CONFIG field of the STE
> > to STRTAB_STE_0_CFG_BYPASS. This works well for devices that only have
> > one substream, i.e. PASID disabled.
> >
> > However, there could be a use case, for a pasid capable device, that
> > allows bypassing the translation at the default substream while still
> > enabling the pasid feature, which means the driver should not skip the
> > allocation of a CD table nor simply bypass the CONFIG field. Instead,
> > the S1DSS field should be set to STRTAB_STE_1_S1DSS_BYPASS and the
> > SHCFG field should be set to STRTAB_STE_1_SHCFG_INCOMING.
> >
> > Add s1dss and shcfg in struct arm_smmu_s1_cfg, to allow configurations
> > in the finalise() to support that use case. Then, set them accordingly
> > depending on the iommu_domain->type and the master->ssid_bits.
> >
> > Also, add STRTAB_STE_1_SHCFG_NONSHAREABLE of the default configuration
> > to distinguish from STRTAB_STE_1_SHCFG_INCOMING of the bypass one.
>
> Why? The "default configuration" is that the S1 shareability attribute
> is determined by the S1 translation itself, so the incoming value is
> irrelevant.
That was for a consistency since the driver set the SHCFG field
to 0x0 (STRTAB_STE_1_SHCFG_NONSHAREABLE). I was not quite sure,
in a long run, if leaving an uncleared s1_cfg->shcfg potentially
can result in an unexpected behavior if it's passed in the STE.
Yet, we could be seemingly sure that the !IOMMU_DOMAIN_IDENTITY
means the S1 translation must be enabled and so the SHCFG would
be irrelevant?
If so, I make make it:
+ if (smmu_domain->domain.type == IOMMU_DOMAIN_IDENTITY) {
+ cfg->s1dss = STRTAB_STE_1_S1DSS_BYPASS;
+ cfg->shcfg = STRTAB_STE_1_SHCFG_INCOMING;
+ } else {
+ cfg->s1dss = STRTAB_STE_1_S1DSS_SSID0;
+ }
> > @@ -2198,7 +2206,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) {
> > + /*
> > + * A master with a pasid capability might need a CD table, so only set
> > + * ARM_SMMU_DOMAIN_BYPASS if IOMMU_DOMAIN_IDENTITY and non-pasid master
> > + */
> > + if (domain->type == IOMMU_DOMAIN_IDENTITY && !master->ssid_bits) {
> > smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> > return 0;
> > }
>
> This means we'll now go on to allocate a pagetable for an identity
> domain, which doesn't seem ideal :/
Do you suggest to bypass alloc_io_pgtable_ops()? That would zero
out the TCR fields in the CD. Not sure if it'd work seamlessly,
but I can give it a try.
Thanks
Nic
On 2023-06-27 18:06, Nicolin Chen wrote:
> Hi Robin,
>
> On Tue, Jun 27, 2023 at 10:00:18AM +0100, Robin Murphy wrote:
>> On 2023-06-27 04:33, Nicolin Chen wrote:
>>> When an iommu_domain is set to IOMMU_DOMAIN_IDENTITY, the driver would
>>> skip the allocation of a CD table and set the CONFIG field of the STE
>>> to STRTAB_STE_0_CFG_BYPASS. This works well for devices that only have
>>> one substream, i.e. PASID disabled.
>>>
>>> However, there could be a use case, for a pasid capable device, that
>>> allows bypassing the translation at the default substream while still
>>> enabling the pasid feature, which means the driver should not skip the
>>> allocation of a CD table nor simply bypass the CONFIG field. Instead,
>>> the S1DSS field should be set to STRTAB_STE_1_S1DSS_BYPASS and the
>>> SHCFG field should be set to STRTAB_STE_1_SHCFG_INCOMING.
>>>
>>> Add s1dss and shcfg in struct arm_smmu_s1_cfg, to allow configurations
>>> in the finalise() to support that use case. Then, set them accordingly
>>> depending on the iommu_domain->type and the master->ssid_bits.
>>>
>>> Also, add STRTAB_STE_1_SHCFG_NONSHAREABLE of the default configuration
>>> to distinguish from STRTAB_STE_1_SHCFG_INCOMING of the bypass one.
>>
>> Why? The "default configuration" is that the S1 shareability attribute
>> is determined by the S1 translation itself, so the incoming value is
>> irrelevant.
>
> That was for a consistency since the driver set the SHCFG field
> to 0x0 (STRTAB_STE_1_SHCFG_NONSHAREABLE). I was not quite sure,
> in a long run, if leaving an uncleared s1_cfg->shcfg potentially
> can result in an unexpected behavior if it's passed in the STE.
> Yet, we could be seemingly sure that the !IOMMU_DOMAIN_IDENTITY
> means the S1 translation must be enabled and so the SHCFG would
> be irrelevant?
>
> If so, I make make it:
>
> + if (smmu_domain->domain.type == IOMMU_DOMAIN_IDENTITY) {
> + cfg->s1dss = STRTAB_STE_1_S1DSS_BYPASS;
> + cfg->shcfg = STRTAB_STE_1_SHCFG_INCOMING;
> + } else {
> + cfg->s1dss = STRTAB_STE_1_S1DSS_SSID0;
> + }
What I mean is we don't need a cfg->shcfg field at all - without loss of
generality it can simply be hard-coded to 1 when S1 is active, same as
for stream bypass.
The only case where explicitly setting STE.SHCFG=0 makes some sense is
for a stage-2-only domain if a device's incoming attribute is stronger
than it needs to be, but even then there are multiple levels of
IMP-DEFness around whether SHCFG actually does anything anyway.
>>> @@ -2198,7 +2206,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) {
>>> + /*
>>> + * A master with a pasid capability might need a CD table, so only set
>>> + * ARM_SMMU_DOMAIN_BYPASS if IOMMU_DOMAIN_IDENTITY and non-pasid master
>>> + */
>>> + if (domain->type == IOMMU_DOMAIN_IDENTITY && !master->ssid_bits) {
>>> smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
>>> return 0;
>>> }
>>
>> This means we'll now go on to allocate a pagetable for an identity
>> domain, which doesn't seem ideal :/
>
> Do you suggest to bypass alloc_io_pgtable_ops()? That would zero
> out the TCR fields in the CD. Not sure if it'd work seamlessly,
> but I can give it a try.
I think if there's a good reason to support this then it's worth
supporting properly, i.e. refactor a bit harder to separate the CD table
parts which are common to both S1DSS bypass and S1 translation, from the
CD/pagetable parts that are only relevant for translation. S1DSS bypass
remains the same as Stream bypass in the sense that there is no
structure corresponding to the identity domain itself, so not only does
it not make sense to have a pagetable, there's also no valid place to
put one anyway - touching the CD belonging to SSID 0 is strictly wrong.
Thanks,
Robin.
Thanks for the reply.
On Wed, Jun 28, 2023 at 12:29:52AM +0100, Robin Murphy wrote:
> > > > Also, add STRTAB_STE_1_SHCFG_NONSHAREABLE of the default configuration
> > > > to distinguish from STRTAB_STE_1_SHCFG_INCOMING of the bypass one.
> > >
> > > Why? The "default configuration" is that the S1 shareability attribute
> > > is determined by the S1 translation itself, so the incoming value is
> > > irrelevant.
> >
> > That was for a consistency since the driver set the SHCFG field
> > to 0x0 (STRTAB_STE_1_SHCFG_NONSHAREABLE). I was not quite sure,
> > in a long run, if leaving an uncleared s1_cfg->shcfg potentially
> > can result in an unexpected behavior if it's passed in the STE.
> > Yet, we could be seemingly sure that the !IOMMU_DOMAIN_IDENTITY
> > means the S1 translation must be enabled and so the SHCFG would
> > be irrelevant?
> >
> > If so, I make make it:
> >
> > + if (smmu_domain->domain.type == IOMMU_DOMAIN_IDENTITY) {
> > + cfg->s1dss = STRTAB_STE_1_S1DSS_BYPASS;
> > + cfg->shcfg = STRTAB_STE_1_SHCFG_INCOMING;
> > + } else {
> > + cfg->s1dss = STRTAB_STE_1_S1DSS_SSID0;
> > + }
>
> What I mean is we don't need a cfg->shcfg field at all - without loss of
> generality it can simply be hard-coded to 1 when S1 is active, same as
> for stream bypass.
OK.
--------------------------------------------------------------------------------------------------
@@ -1350,7 +1350,8 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
dst[1] = cpu_to_le64(
- FIELD_PREP(STRTAB_STE_1_S1DSS, STRTAB_STE_1_S1DSS_SSID0) |
+ FIELD_PREP(STRTAB_STE_1_S1DSS, s1_cfg->s1dss) |
+ FIELD_PREP(STRTAB_STE_1_SHCFG, STRTAB_STE_1_SHCFG_INCOMING) |
FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_WBRA) |
--------------------------------------------------------------------------------------------------
> The only case where explicitly setting STE.SHCFG=0 makes some sense is
> for a stage-2-only domain if a device's incoming attribute is stronger
> than it needs to be, but even then there are multiple levels of
> IMP-DEFness around whether SHCFG actually does anything anyway.
I see. Thanks for elaborating.
> > > > @@ -2198,7 +2206,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) {
> > > > + /*
> > > > + * A master with a pasid capability might need a CD table, so only set
> > > > + * ARM_SMMU_DOMAIN_BYPASS if IOMMU_DOMAIN_IDENTITY and non-pasid master
> > > > + */
> > > > + if (domain->type == IOMMU_DOMAIN_IDENTITY && !master->ssid_bits) {
> > > > smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> > > > return 0;
> > > > }
> > >
> > > This means we'll now go on to allocate a pagetable for an identity
> > > domain, which doesn't seem ideal :/
> >
> > Do you suggest to bypass alloc_io_pgtable_ops()? That would zero
> > out the TCR fields in the CD. Not sure if it'd work seamlessly,
> > but I can give it a try.
>
> I think if there's a good reason to support this then it's worth
There is an unignorable perf difference that we see on a real HW.
So the reason or (I should say) the requirement is pretty strong.
> supporting properly, i.e. refactor a bit harder to separate the CD table
> parts which are common to both S1DSS bypass and S1 translation, from the
> CD/pagetable parts that are only relevant for translation. S1DSS bypass
> remains the same as Stream bypass in the sense that there is no
> structure corresponding to the identity domain itself, so not only does
> it not make sense to have a pagetable, there's also no valid place to
> put one anyway - touching the CD belonging to SSID 0 is strictly wrong.
I can try that. Yet, I think the S1DSS bypass case still belongs
to ARM_SMMU_DOMAIN_S1/arm_smmu_domain_finalise_s1, right?
I'd try keeping most of the parts intact while adding a pointer
to a structure holding pagetable stuff, to make it cleaner. Then
the S1DSS bypass case can be flagged by an empty pointer.
Thanks
Nic
On Wed, Jun 28, 2023 at 04:39:48PM +0100, Robin Murphy wrote:
> > I'd try keeping most of the parts intact while adding a pointer
> > to a structure holding pagetable stuff, to make it cleaner. Then
> > the S1DSS bypass case can be flagged by an empty pointer.
>
> I'd expect that what you need for this is much the same as what Michael has
> already proposed for the PASID-generalisation series. The current inside-out
> notion of S1 domains owning CD tables is what's getting in the way of doing
> the right thing cleanly, in both cases.
Yeah, that was sort of my guessed feeling as well..
What do you think of Michael's series?
Jason
On 2023-06-28 01:13, Nicolin Chen wrote:
> Thanks for the reply.
>
> On Wed, Jun 28, 2023 at 12:29:52AM +0100, Robin Murphy wrote:
>
>>>>> Also, add STRTAB_STE_1_SHCFG_NONSHAREABLE of the default configuration
>>>>> to distinguish from STRTAB_STE_1_SHCFG_INCOMING of the bypass one.
>>>>
>>>> Why? The "default configuration" is that the S1 shareability attribute
>>>> is determined by the S1 translation itself, so the incoming value is
>>>> irrelevant.
>>>
>>> That was for a consistency since the driver set the SHCFG field
>>> to 0x0 (STRTAB_STE_1_SHCFG_NONSHAREABLE). I was not quite sure,
>>> in a long run, if leaving an uncleared s1_cfg->shcfg potentially
>>> can result in an unexpected behavior if it's passed in the STE.
>>> Yet, we could be seemingly sure that the !IOMMU_DOMAIN_IDENTITY
>>> means the S1 translation must be enabled and so the SHCFG would
>>> be irrelevant?
>>>
>>> If so, I make make it:
>>>
>>> + if (smmu_domain->domain.type == IOMMU_DOMAIN_IDENTITY) {
>>> + cfg->s1dss = STRTAB_STE_1_S1DSS_BYPASS;
>>> + cfg->shcfg = STRTAB_STE_1_SHCFG_INCOMING;
>>> + } else {
>>> + cfg->s1dss = STRTAB_STE_1_S1DSS_SSID0;
>>> + }
>>
>> What I mean is we don't need a cfg->shcfg field at all - without loss of
>> generality it can simply be hard-coded to 1 when S1 is active, same as
>> for stream bypass.
>
> OK.
> --------------------------------------------------------------------------------------------------
> @@ -1350,7 +1350,8 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
> dst[1] = cpu_to_le64(
> - FIELD_PREP(STRTAB_STE_1_S1DSS, STRTAB_STE_1_S1DSS_SSID0) |
> + FIELD_PREP(STRTAB_STE_1_S1DSS, s1_cfg->s1dss) |
> + FIELD_PREP(STRTAB_STE_1_SHCFG, STRTAB_STE_1_SHCFG_INCOMING) |
> FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_WBRA) |
> --------------------------------------------------------------------------------------------------
>
>> The only case where explicitly setting STE.SHCFG=0 makes some sense is
>> for a stage-2-only domain if a device's incoming attribute is stronger
>> than it needs to be, but even then there are multiple levels of
>> IMP-DEFness around whether SHCFG actually does anything anyway.
>
> I see. Thanks for elaborating.
>
>>>>> @@ -2198,7 +2206,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) {
>>>>> + /*
>>>>> + * A master with a pasid capability might need a CD table, so only set
>>>>> + * ARM_SMMU_DOMAIN_BYPASS if IOMMU_DOMAIN_IDENTITY and non-pasid master
>>>>> + */
>>>>> + if (domain->type == IOMMU_DOMAIN_IDENTITY && !master->ssid_bits) {
>>>>> smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
>>>>> return 0;
>>>>> }
>>>>
>>>> This means we'll now go on to allocate a pagetable for an identity
>>>> domain, which doesn't seem ideal :/
>>>
>>> Do you suggest to bypass alloc_io_pgtable_ops()? That would zero
>>> out the TCR fields in the CD. Not sure if it'd work seamlessly,
>>> but I can give it a try.
>>
>> I think if there's a good reason to support this then it's worth
>
> There is an unignorable perf difference that we see on a real HW.
> So the reason or (I should say) the requirement is pretty strong.
>
>> supporting properly, i.e. refactor a bit harder to separate the CD table
>> parts which are common to both S1DSS bypass and S1 translation, from the
>> CD/pagetable parts that are only relevant for translation. S1DSS bypass
>> remains the same as Stream bypass in the sense that there is no
>> structure corresponding to the identity domain itself, so not only does
>> it not make sense to have a pagetable, there's also no valid place to
>> put one anyway - touching the CD belonging to SSID 0 is strictly wrong.
>
> I can try that. Yet, I think the S1DSS bypass case still belongs
> to ARM_SMMU_DOMAIN_S1/arm_smmu_domain_finalise_s1, right?
That's what I'm disagreeing with - although S1DSS behaviour requires S1
translation to be nominally enabled for the stream as a whole, the
bypass domain is distinctly *not* an S1 translation domain, and there is
no S1 context to finalise. I think it's either a generalisation of
ARM_SMMU_DOMAIN_BYPASS based on s1cdmax, or it's its own new thing.
> I'd try keeping most of the parts intact while adding a pointer
> to a structure holding pagetable stuff, to make it cleaner. Then
> the S1DSS bypass case can be flagged by an empty pointer.
I'd expect that what you need for this is much the same as what Michael
has already proposed for the PASID-generalisation series. The current
inside-out notion of S1 domains owning CD tables is what's getting in
the way of doing the right thing cleanly, in both cases.
Thanks,
Robin.
Hi Robin,
On Wed, Jun 28, 2023 at 04:39:48PM +0100, Robin Murphy wrote:
> > > supporting properly, i.e. refactor a bit harder to separate the CD table
> > > parts which are common to both S1DSS bypass and S1 translation, from the
> > > CD/pagetable parts that are only relevant for translation. S1DSS bypass
> > > remains the same as Stream bypass in the sense that there is no
> > > structure corresponding to the identity domain itself, so not only does
> > > it not make sense to have a pagetable, there's also no valid place to
> > > put one anyway - touching the CD belonging to SSID 0 is strictly wrong.
> >
> > I can try that. Yet, I think the S1DSS bypass case still belongs
> > to ARM_SMMU_DOMAIN_S1/arm_smmu_domain_finalise_s1, right?
>
> That's what I'm disagreeing with - although S1DSS behaviour requires S1
> translation to be nominally enabled for the stream as a whole, the
> bypass domain is distinctly *not* an S1 translation domain, and there is
> no S1 context to finalise. I think it's either a generalisation of
> ARM_SMMU_DOMAIN_BYPASS based on s1cdmax, or it's its own new thing.
Hmm, the fundamental of my view is that the ARM_SMMU_DOMAIN_*
list is quite matching with the CONFIG field of an STE, which
turns the S1DSS bypass into the ARM_SMMU_DOMAIN_S1 category.
And after all S1DSS is named "S1" :)
Following your view, How about ARM_SMMU_DOMAIN_S1DSS_BYPASS?
> > I'd try keeping most of the parts intact while adding a pointer
> > to a structure holding pagetable stuff, to make it cleaner. Then
> > the S1DSS bypass case can be flagged by an empty pointer.
>
> I'd expect that what you need for this is much the same as what Michael
> has already proposed for the PASID-generalisation series. The current
> inside-out notion of S1 domains owning CD tables is what's getting in
> the way of doing the right thing cleanly, in both cases.
Yea, Jason had the same remarks when I discussed this matter
with him. But I took the chance after I saw a shortcut :)
I have been aligning with Michael already, trying to figure
out how much ground that our use cases can share.
Thanks
Nic
We have a use-case for this patch to help in avoiding the map/unmap
overhead in some cases but still maintaining support for PASIDs.
Thanks.
On Tue, Sep 12, 2023 at 1:30 PM Nicolin Chen <[email protected]> wrote:
>
> When an iommu_domain is set to IOMMU_DOMAIN_IDENTITY, the driver would
> skip the allocation of a CD table and set the CONFIG field of the STE
> to STRTAB_STE_0_CFG_BYPASS. This works well for devices that only have
> one substream, i.e. PASID disabled.
>
> However, there could be a use case, for a pasid capable device, that
> allows bypassing the translation at the default substream while still
> enabling the pasid feature, which means the driver should not skip the
> allocation of a CD table nor simply bypass the CONFIG field. Instead,
> the S1DSS field should be set to STRTAB_STE_1_S1DSS_BYPASS and the
> SHCFG field should be set to STRTAB_STE_1_SHCFG_INCOMING.
>
> Add s1dss and shcfg in struct arm_smmu_s1_cfg, to allow configurations
> in the finalise() to support that use case. Then, set them accordingly
> depending on the iommu_domain->type and the master->ssid_bits.
>
> Also, add STRTAB_STE_1_SHCFG_NONSHAREABLE of the default configuration
> to distinguish from STRTAB_STE_1_SHCFG_INCOMING of the bypass one.
>
> Signed-off-by: Nicolin Chen <[email protected]>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 +++++++++++++++---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +++
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 9b0dc3505601..8dc7934a0175 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1350,11 +1350,12 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
>
> BUG_ON(ste_live);
> dst[1] = cpu_to_le64(
> - FIELD_PREP(STRTAB_STE_1_S1DSS, STRTAB_STE_1_S1DSS_SSID0) |
> + FIELD_PREP(STRTAB_STE_1_S1DSS, s1_cfg->s1dss) |
> FIELD_PREP(STRTAB_STE_1_S1CIR, STRTAB_STE_1_S1C_CACHE_WBRA) |
> FIELD_PREP(STRTAB_STE_1_S1COR, STRTAB_STE_1_S1C_CACHE_WBRA) |
> FIELD_PREP(STRTAB_STE_1_S1CSH, ARM_SMMU_SH_ISH) |
> - FIELD_PREP(STRTAB_STE_1_STRW, strw));
> + FIELD_PREP(STRTAB_STE_1_STRW, strw) |
> + FIELD_PREP(STRTAB_STE_1_SHCFG, s1_cfg->shcfg));
>
> if (smmu->features & ARM_SMMU_FEAT_STALLS &&
> !master->stall_enabled)
> @@ -2119,6 +2120,13 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
> goto out_unlock;
>
> cfg->s1cdmax = master->ssid_bits;
> + if (smmu_domain->domain.type == IOMMU_DOMAIN_IDENTITY) {
> + cfg->s1dss = STRTAB_STE_1_S1DSS_BYPASS;
> + cfg->shcfg = STRTAB_STE_1_SHCFG_INCOMING;
> + } else {
> + cfg->s1dss = STRTAB_STE_1_S1DSS_SSID0;
> + cfg->shcfg = STRTAB_STE_1_SHCFG_NONSHAREABLE;
> + }
>
> smmu_domain->stall_enabled = master->stall_enabled;
>
> @@ -2198,7 +2206,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) {
> + /*
> + * A master with a pasid capability might need a CD table, so only set
> + * ARM_SMMU_DOMAIN_BYPASS if IOMMU_DOMAIN_IDENTITY and non-pasid master
> + */
> + if (domain->type == IOMMU_DOMAIN_IDENTITY && !master->ssid_bits) {
> smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> return 0;
> }
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index dcab85698a4e..8052d02770d0 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -244,6 +244,7 @@
> #define STRTAB_STE_1_STRW_EL2 2UL
>
> #define STRTAB_STE_1_SHCFG GENMASK_ULL(45, 44)
> +#define STRTAB_STE_1_SHCFG_NONSHAREABLE 0UL
> #define STRTAB_STE_1_SHCFG_INCOMING 1UL
>
> #define STRTAB_STE_2_S2VMID GENMASK_ULL(15, 0)
> @@ -601,6 +602,8 @@ struct arm_smmu_s1_cfg {
> struct arm_smmu_ctx_desc_cfg cdcfg;
> struct arm_smmu_ctx_desc cd;
> u8 s1fmt;
> + u8 s1dss;
> + u8 shcfg;
> u8 s1cdmax;
> };
>
> --
> 2.41.0
>
On Thu, Sep 14, 2023 at 9:03 AM Nicolin Chen <[email protected]> wrote:
>
> This has another version [1] rebased on top of Michael's refactor
> series [2]. We'd need Michael to respin the refactor sereis and I
> will resend mine.
Just to clarify, you'd like me to rebase off master and re-send? Or do
you mean something else by respin?
On Tue, Sep 12, 2023 at 03:13:25PM -0700, Aahil Awatramani wrote:
>
> We have a use-case for this patch to help in avoiding the map/unmap
> overhead in some cases but still maintaining support for PASIDs.
This has another version [1] rebased on top of Michael's refactor
series [2]. We'd need Michael to respin the refactor sereis and I
will resend mine.
[1] https://lore.kernel.org/linux-iommu/[email protected]/
[2] https://lore.kernel.org/linux-iommu/ZPEpl9D%2FCqQWYhzk@Asurada-Nvidia/
Thanks
Nicolin
On Thu, Sep 14, 2023 at 05:01:58PM +0800, Michael Shavit wrote:
> On Thu, Sep 14, 2023 at 9:03 AM Nicolin Chen <[email protected]> wrote:
> >
> > This has another version [1] rebased on top of Michael's refactor
> > series [2]. We'd need Michael to respin the refactor sereis and I
> > will resend mine.
>
> Just to clarify, you'd like me to rebase off master and re-send? Or do
> you mean something else by respin?
Usually we resend patches that were not accepted in the previous
cycle, rebasing on top of the new rc1. So, I would checkout the
6.6-rc1 branch and rebase the series with necessary editing, and
then send another version.
That refactor series is quite crucial for other feature series.
So, it'd be nicer to merge it as early as possible.
Thanks
Nicolin