[v7,2/9] iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry()

Commit Message

Jason Gunthorpe April 16, 2024, 7:28 p.m. UTC

CD table entries and STE's have the same essential programming sequence,
just with different types.

Have arm_smmu_write_ctx_desc() generate a target CD and call
arm_smmu_write_entry() to do the programming. Due to the way the target CD
is generated by modifying the existing CD this alone is not enough for the
CD callers to be freed of the ordering requirements.

The following patches will make the rest of the CD flow mirror the STE
flow with precise CD contents generated in all cases.

Signed-off-by: Michael Shavit <[e-mail]>
Tested-by: Nicolin Chen <[e-mail]>
Tested-by: Shameer Kolothum <[e-mail]>
Reviewed-by: Moritz Fischer <[e-mail]>
Signed-off-by: Jason Gunthorpe <[e-mail]>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 94 ++++++++++++++++-----
 1 file changed, 74 insertions(+), 20 deletions(-)

Comments

Nicolin Chen April 16, 2024, 8:48 p.m. UTC | #1

On Tue, Apr 16, 2024 at 04:28:13PM -0300, Jason Gunthorpe wrote:
> CD table entries and STE's have the same essential programming sequence,
> just with different types.
> 
> Have arm_smmu_write_ctx_desc() generate a target CD and call
> arm_smmu_write_entry() to do the programming. Due to the way the target CD
> is generated by modifying the existing CD this alone is not enough for the
> CD callers to be freed of the ordering requirements.
> 
> The following patches will make the rest of the CD flow mirror the STE
> flow with precise CD contents generated in all cases.
> 
> Signed-off-by: Michael Shavit <[e-mail]>
> Tested-by: Nicolin Chen <[e-mail]>
> Tested-by: Shameer Kolothum <[e-mail]>
> Reviewed-by: Moritz Fischer <[e-mail]>
> Signed-off-by: Jason Gunthorpe <[e-mail]>

Reviewed-by: Nicolin Chen <[e-mail]>

Robin Murphy April 18, 2024, 1:01 p.m. UTC | #2

On 16/04/2024 8:28 pm, Jason Gunthorpe wrote:
> CD table entries and STE's have the same essential programming sequence,
> just with different types.
> 
> Have arm_smmu_write_ctx_desc() generate a target CD and call
> arm_smmu_write_entry() to do the programming. Due to the way the target CD
> is generated by modifying the existing CD this alone is not enough for the
> CD callers to be freed of the ordering requirements.
> 
> The following patches will make the rest of the CD flow mirror the STE
> flow with precise CD contents generated in all cases.
> 
> Signed-off-by: Michael Shavit <[e-mail]>
> Tested-by: Nicolin Chen <[e-mail]>
> Tested-by: Shameer Kolothum <[e-mail]>
> Reviewed-by: Moritz Fischer <[e-mail]>
> Signed-off-by: Jason Gunthorpe <[e-mail]>
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 94 ++++++++++++++++-----
>   1 file changed, 74 insertions(+), 20 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 bf105e914d38b1..3983de90c2fa01 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -56,6 +56,7 @@ struct arm_smmu_entry_writer_ops {
>   
>   #define NUM_ENTRY_QWORDS 8
>   static_assert(sizeof(struct arm_smmu_ste) == NUM_ENTRY_QWORDS * sizeof(u64));
> +static_assert(sizeof(struct arm_smmu_cd) == NUM_ENTRY_QWORDS * sizeof(u64));
>   
>   static phys_addr_t arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = {
>   	[EVTQ_MSI_INDEX] = {
> @@ -1231,6 +1232,67 @@ static struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
>   	return &l1_desc->l2ptr[idx];
>   }
>   
> +struct arm_smmu_cd_writer {
> +	struct arm_smmu_entry_writer writer;
> +	unsigned int ssid;
> +};
> +
> +static void arm_smmu_get_cd_used(const __le64 *ent, __le64 *used_bits)
> +{
> +	used_bits[0] = cpu_to_le64(CTXDESC_CD_0_V);
> +	if (!(ent[0] & cpu_to_le64(CTXDESC_CD_0_V)))
> +		return;
> +	memset(used_bits, 0xFF, sizeof(struct arm_smmu_cd));
> +
> +	/* EPD0 means T0SZ/TG0/IR0/OR0/SH0/TTB0 are IGNORED */

They're ignored if the *effective value* of EPD0 is 1, which means you 
also need to account for when EPD0 itself is ignored, or all this 
complication is essentially meaningless.

Thanks,
Robin.

> +	if (ent[0] & cpu_to_le64(CTXDESC_CD_0_TCR_EPD0)) {
> +		used_bits[0] &= ~cpu_to_le64(
> +			CTXDESC_CD_0_TCR_T0SZ | CTXDESC_CD_0_TCR_TG0 |
> +			CTXDESC_CD_0_TCR_IRGN0 | CTXDESC_CD_0_TCR_ORGN0 |
> +			CTXDESC_CD_0_TCR_SH0);
> +		used_bits[1] &= ~cpu_to_le64(CTXDESC_CD_1_TTB0_MASK);
> +	}
> +}
> +
> +static void arm_smmu_cd_writer_sync_entry(struct arm_smmu_entry_writer *writer)
> +{
> +	struct arm_smmu_cd_writer *cd_writer =
> +		container_of(writer, struct arm_smmu_cd_writer, writer);
> +
> +	arm_smmu_sync_cd(writer->master, cd_writer->ssid, true);
> +}
> +
> +static const struct arm_smmu_entry_writer_ops arm_smmu_cd_writer_ops = {
> +	.sync = arm_smmu_cd_writer_sync_entry,
> +	.get_used = arm_smmu_get_cd_used,
> +	.v_bit = cpu_to_le64(CTXDESC_CD_0_V),
> +};
> +
> +static void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
> +				    struct arm_smmu_cd *cdptr,
> +				    const struct arm_smmu_cd *target)
> +{
> +	struct arm_smmu_cd_writer cd_writer = {
> +		.writer = {
> +			.ops = &arm_smmu_cd_writer_ops,
> +			.master = master,
> +		},
> +		.ssid = ssid,
> +	};
> +
> +	arm_smmu_write_entry(&cd_writer.writer, cdptr->data, target->data);
> +}
> +
> +static void arm_smmu_clean_cd_entry(struct arm_smmu_cd *target)
> +{
> +	struct arm_smmu_cd used = {};
> +	int i;
> +
> +	arm_smmu_get_cd_used(target->data, used.data);
> +	for (i = 0; i != ARRAY_SIZE(target->data); i++)
> +		target->data[i] &= used.data[i];
> +}
> +
>   int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
>   			    struct arm_smmu_ctx_desc *cd)
>   {
> @@ -1247,17 +1309,20 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
>   	 */
>   	u64 val;
>   	bool cd_live;
> -	struct arm_smmu_cd *cdptr;
> +	struct arm_smmu_cd target;
> +	struct arm_smmu_cd *cdptr = &target;
> +	struct arm_smmu_cd *cd_table_entry;
>   	struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
>   	struct arm_smmu_device *smmu = master->smmu;
>   
>   	if (WARN_ON(ssid >= (1 << cd_table->s1cdmax)))
>   		return -E2BIG;
>   
> -	cdptr = arm_smmu_get_cd_ptr(master, ssid);
> -	if (!cdptr)
> +	cd_table_entry = arm_smmu_get_cd_ptr(master, ssid);
> +	if (!cd_table_entry)
>   		return -ENOMEM;
>   
> +	target = *cd_table_entry;
>   	val = le64_to_cpu(cdptr->data[0]);
>   	cd_live = !!(val & CTXDESC_CD_0_V);
>   
> @@ -1279,13 +1344,6 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
>   		cdptr->data[2] = 0;
>   		cdptr->data[3] = cpu_to_le64(cd->mair);
>   
> -		/*
> -		 * STE may be live, and the SMMU might read dwords of this CD in any
> -		 * order. Ensure that it observes valid values before reading
> -		 * V=1.
> -		 */
> -		arm_smmu_sync_cd(master, ssid, true);
> -
>   		val = cd->tcr |
>   #ifdef __BIG_ENDIAN
>   			CTXDESC_CD_0_ENDI |
> @@ -1299,18 +1357,14 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
>   		if (cd_table->stall_enabled)
>   			val |= CTXDESC_CD_0_S;
>   	}
> -
> +	cdptr->data[0] = cpu_to_le64(val);
>   	/*
> -	 * The SMMU accesses 64-bit values atomically. See IHI0070Ca 3.21.3
> -	 * "Configuration structures and configuration invalidation completion"
> -	 *
> -	 *   The size of single-copy atomic reads made by the SMMU is
> -	 *   IMPLEMENTATION DEFINED but must be at least 64 bits. Any single
> -	 *   field within an aligned 64-bit span of a structure can be altered
> -	 *   without first making the structure invalid.
> +	 * Since the above is updating the CD entry based on the current value
> +	 * without zeroing unused bits it needs fixing before being passed to
> +	 * the programming logic.
>   	 */
> -	WRITE_ONCE(cdptr->data[0], cpu_to_le64(val));
> -	arm_smmu_sync_cd(master, ssid, true);
> +	arm_smmu_clean_cd_entry(&target);
> +	arm_smmu_write_cd_entry(master, ssid, cd_table_entry, &target);
>   	return 0;
>   }
>

Jason Gunthorpe April 18, 2024, 4:08 p.m. UTC | #3

On Thu, Apr 18, 2024 at 02:01:31PM +0100, Robin Murphy wrote:
> On 16/04/2024 8:28 pm, Jason Gunthorpe wrote:
> > CD table entries and STE's have the same essential programming sequence,
> > just with different types.
> > 
> > Have arm_smmu_write_ctx_desc() generate a target CD and call
> > arm_smmu_write_entry() to do the programming. Due to the way the target CD
> > is generated by modifying the existing CD this alone is not enough for the
> > CD callers to be freed of the ordering requirements.
> > 
> > The following patches will make the rest of the CD flow mirror the STE
> > flow with precise CD contents generated in all cases.
> > 
> > Signed-off-by: Michael Shavit <[e-mail]>
> > Tested-by: Nicolin Chen <[e-mail]>
> > Tested-by: Shameer Kolothum <[e-mail]>
> > Reviewed-by: Moritz Fischer <[e-mail]>
> > Signed-off-by: Jason Gunthorpe <[e-mail]>
> > ---
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 94 ++++++++++++++++-----
> >   1 file changed, 74 insertions(+), 20 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 bf105e914d38b1..3983de90c2fa01 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -56,6 +56,7 @@ struct arm_smmu_entry_writer_ops {
> >   #define NUM_ENTRY_QWORDS 8
> >   static_assert(sizeof(struct arm_smmu_ste) == NUM_ENTRY_QWORDS * sizeof(u64));
> > +static_assert(sizeof(struct arm_smmu_cd) == NUM_ENTRY_QWORDS * sizeof(u64));
> >   static phys_addr_t arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = {
> >   	[EVTQ_MSI_INDEX] = {
> > @@ -1231,6 +1232,67 @@ static struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
> >   	return &l1_desc->l2ptr[idx];
> >   }
> > +struct arm_smmu_cd_writer {
> > +	struct arm_smmu_entry_writer writer;
> > +	unsigned int ssid;
> > +};
> > +
> > +static void arm_smmu_get_cd_used(const __le64 *ent, __le64 *used_bits)
> > +{
> > +	used_bits[0] = cpu_to_le64(CTXDESC_CD_0_V);
> > +	if (!(ent[0] & cpu_to_le64(CTXDESC_CD_0_V)))
> > +		return;
> > +	memset(used_bits, 0xFF, sizeof(struct arm_smmu_cd));
> > +
> > +	/* EPD0 means T0SZ/TG0/IR0/OR0/SH0/TTB0 are IGNORED */
> 
> They're ignored if the *effective value* of EPD0 is 1, which means you also
> need to account for when EPD0 itself is ignored, or all this complication is
> essentially meaningless.

Do you mean this?

 Consistent with Armv8-A translation, the EPD0 and EPD1 fields are
 IGNORED (and their effective value is 0) if this CD is located from
 an STE with StreamWorld of any-EL2 or EL3. It is only possible for an
 EL1 (Secure or non-secure) or any-EL2-E2H stream to disable
 translation table walk sing EPD0 or EPD1.

Regardless, part of the design is that the make functions don't set
IGNORED bits and get_used only has to process what the make functions
build, not the universe of all descriptors.

In this case the make function sets EPD0 and constructs a CD that is
only valid if EPD0 is available. It also zeros the TTB0/etc values
because they are expected to be IGNORED and the code has no valid
value to provide anyhow.

The comment was intened to be read as: if EPD0 is set [by the make
function] then TTB0/etc will be IGNORED.

I will update the comment for clarity.

The complexity you are talking about must be delt with by the make
side. If we do need to support something where EPD0 doesn't work then
make functions must never set it.

Do we have a problem here? Can SVA activate and EPD0 will be ignored?
That would be a security bug.

Thanks,
Jason

Mostafa Saleh April 19, 2024, 9:07 p.m. UTC | #4

Hi Jason,

On Tue, Apr 16, 2024 at 04:28:13PM -0300, Jason Gunthorpe wrote:
> CD table entries and STE's have the same essential programming sequence,
> just with different types.
> 
> Have arm_smmu_write_ctx_desc() generate a target CD and call
> arm_smmu_write_entry() to do the programming. Due to the way the target CD
> is generated by modifying the existing CD this alone is not enough for the
> CD callers to be freed of the ordering requirements.
> 
> The following patches will make the rest of the CD flow mirror the STE
> flow with precise CD contents generated in all cases.
> 
> Signed-off-by: Michael Shavit <[e-mail]>
> Tested-by: Nicolin Chen <[e-mail]>
> Tested-by: Shameer Kolothum <[e-mail]>
> Reviewed-by: Moritz Fischer <[e-mail]>
> Signed-off-by: Jason Gunthorpe <[e-mail]>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 94 ++++++++++++++++-----
>  1 file changed, 74 insertions(+), 20 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 bf105e914d38b1..3983de90c2fa01 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -56,6 +56,7 @@ struct arm_smmu_entry_writer_ops {
>  
>  #define NUM_ENTRY_QWORDS 8
>  static_assert(sizeof(struct arm_smmu_ste) == NUM_ENTRY_QWORDS * sizeof(u64));
> +static_assert(sizeof(struct arm_smmu_cd) == NUM_ENTRY_QWORDS * sizeof(u64));
>  
>  static phys_addr_t arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = {
>  	[EVTQ_MSI_INDEX] = {
> @@ -1231,6 +1232,67 @@ static struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
>  	return &l1_desc->l2ptr[idx];
>  }
>  
> +struct arm_smmu_cd_writer {
> +	struct arm_smmu_entry_writer writer;
> +	unsigned int ssid;
> +};
> +
> +static void arm_smmu_get_cd_used(const __le64 *ent, __le64 *used_bits)
> +{
> +	used_bits[0] = cpu_to_le64(CTXDESC_CD_0_V);
> +	if (!(ent[0] & cpu_to_le64(CTXDESC_CD_0_V)))
> +		return;
> +	memset(used_bits, 0xFF, sizeof(struct arm_smmu_cd));
> +
> +	/* EPD0 means T0SZ/TG0/IR0/OR0/SH0/TTB0 are IGNORED */
> +	if (ent[0] & cpu_to_le64(CTXDESC_CD_0_TCR_EPD0)) {
> +		used_bits[0] &= ~cpu_to_le64(
> +			CTXDESC_CD_0_TCR_T0SZ | CTXDESC_CD_0_TCR_TG0 |
> +			CTXDESC_CD_0_TCR_IRGN0 | CTXDESC_CD_0_TCR_ORGN0 |
> +			CTXDESC_CD_0_TCR_SH0);
> +		used_bits[1] &= ~cpu_to_le64(CTXDESC_CD_1_TTB0_MASK);
> +	}
> +}
> +
> +static void arm_smmu_cd_writer_sync_entry(struct arm_smmu_entry_writer *writer)
> +{
> +	struct arm_smmu_cd_writer *cd_writer =
> +		container_of(writer, struct arm_smmu_cd_writer, writer);
> +
> +	arm_smmu_sync_cd(writer->master, cd_writer->ssid, true);
> +}
> +
> +static const struct arm_smmu_entry_writer_ops arm_smmu_cd_writer_ops = {
> +	.sync = arm_smmu_cd_writer_sync_entry,
> +	.get_used = arm_smmu_get_cd_used,
> +	.v_bit = cpu_to_le64(CTXDESC_CD_0_V),
> +};
> +
> +static void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
> +				    struct arm_smmu_cd *cdptr,
> +				    const struct arm_smmu_cd *target)
> +{
> +	struct arm_smmu_cd_writer cd_writer = {
> +		.writer = {
> +			.ops = &arm_smmu_cd_writer_ops,
> +			.master = master,
> +		},
> +		.ssid = ssid,
> +	};
> +
> +	arm_smmu_write_entry(&cd_writer.writer, cdptr->data, target->data);
> +}
> +
> +static void arm_smmu_clean_cd_entry(struct arm_smmu_cd *target)
> +{
> +	struct arm_smmu_cd used = {};
> +	int i;
> +
> +	arm_smmu_get_cd_used(target->data, used.data);
> +	for (i = 0; i != ARRAY_SIZE(target->data); i++)
> +		target->data[i] &= used.data[i];
> +}
> +
>  int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
>  			    struct arm_smmu_ctx_desc *cd)
>  {
> @@ -1247,17 +1309,20 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
>  	 */
>  	u64 val;
>  	bool cd_live;
> -	struct arm_smmu_cd *cdptr;
> +	struct arm_smmu_cd target;
> +	struct arm_smmu_cd *cdptr = &target;
> +	struct arm_smmu_cd *cd_table_entry;
>  	struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
>  	struct arm_smmu_device *smmu = master->smmu;
>  
>  	if (WARN_ON(ssid >= (1 << cd_table->s1cdmax)))
>  		return -E2BIG;
>  
> -	cdptr = arm_smmu_get_cd_ptr(master, ssid);
> -	if (!cdptr)
> +	cd_table_entry = arm_smmu_get_cd_ptr(master, ssid);
> +	if (!cd_table_entry)
>  		return -ENOMEM;
>  
> +	target = *cd_table_entry;

As this changes the logic where all CD manipulation is not on the actual
CD, I believe a comment would be helpful here.

>  	val = le64_to_cpu(cdptr->data[0]);
>  	cd_live = !!(val & CTXDESC_CD_0_V);
>  
> @@ -1279,13 +1344,6 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
>  		cdptr->data[2] = 0;
>  		cdptr->data[3] = cpu_to_le64(cd->mair);
>  
> -		/*
> -		 * STE may be live, and the SMMU might read dwords of this CD in any
> -		 * order. Ensure that it observes valid values before reading
> -		 * V=1.
> -		 */
> -		arm_smmu_sync_cd(master, ssid, true);
> -
>  		val = cd->tcr |
>  #ifdef __BIG_ENDIAN
>  			CTXDESC_CD_0_ENDI |
> @@ -1299,18 +1357,14 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
>  		if (cd_table->stall_enabled)
>  			val |= CTXDESC_CD_0_S;
>  	}
> -
> +	cdptr->data[0] = cpu_to_le64(val);
>  	/*
> -	 * The SMMU accesses 64-bit values atomically. See IHI0070Ca 3.21.3
> -	 * "Configuration structures and configuration invalidation completion"
> -	 *
> -	 *   The size of single-copy atomic reads made by the SMMU is
> -	 *   IMPLEMENTATION DEFINED but must be at least 64 bits. Any single
> -	 *   field within an aligned 64-bit span of a structure can be altered
> -	 *   without first making the structure invalid.
> +	 * Since the above is updating the CD entry based on the current value
> +	 * without zeroing unused bits it needs fixing before being passed to
> +	 * the programming logic.
>  	 */
> -	WRITE_ONCE(cdptr->data[0], cpu_to_le64(val));
> -	arm_smmu_sync_cd(master, ssid, true);
> +	arm_smmu_clean_cd_entry(&target);

I am not sure I understand the logic here, is that only needed for entry[0]
As I see the other entries are set and not reused.

If so, I think it’d be better to make that clear, also as used_bits are always 0xff
for all cases, I believe the EPD0 logic should be integrated in populating the CD so
it is correct by construction, as this looks like a hack to me.

Thanks,
Mostafa

> +	arm_smmu_write_cd_entry(master, ssid, cd_table_entry, &target);
>  	return 0;
>  }
>  
> -- 
> 2.43.2
>

Jason Gunthorpe April 22, 2024, 1:29 p.m. UTC | #5

On Fri, Apr 19, 2024 at 09:07:19PM +0000, Mostafa Saleh wrote:
> > -	cdptr = arm_smmu_get_cd_ptr(master, ssid);
> > -	if (!cdptr)
> > +	cd_table_entry = arm_smmu_get_cd_ptr(master, ssid);
> > +	if (!cd_table_entry)
> >  		return -ENOMEM;
> >  
> > +	target = *cd_table_entry;
> 
> As this changes the logic where all CD manipulation is not on the actual
> CD, I believe a comment would be helpful here.

This is all deleted in a few patches, doesn't seem worth it to
me. These steps exist only for bisection.

> > @@ -1299,18 +1357,14 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
> >  		if (cd_table->stall_enabled)
> >  			val |= CTXDESC_CD_0_S;
> >  	}
> > -
> > +	cdptr->data[0] = cpu_to_le64(val);
> >  	/*
> > -	 * The SMMU accesses 64-bit values atomically. See IHI0070Ca 3.21.3
> > -	 * "Configuration structures and configuration invalidation completion"
> > -	 *
> > -	 *   The size of single-copy atomic reads made by the SMMU is
> > -	 *   IMPLEMENTATION DEFINED but must be at least 64 bits. Any single
> > -	 *   field within an aligned 64-bit span of a structure can be altered
> > -	 *   without first making the structure invalid.
> > +	 * Since the above is updating the CD entry based on the current value
> > +	 * without zeroing unused bits it needs fixing before being passed to
> > +	 * the programming logic.
> >  	 */
> > -	WRITE_ONCE(cdptr->data[0], cpu_to_le64(val));
> > -	arm_smmu_sync_cd(master, ssid, true);
> > +	arm_smmu_clean_cd_entry(&target);
> 
> I am not sure I understand the logic here, is that only needed for entry[0]
> As I see the other entries are set and not reused.

I'm not sure what you are asking?

The issue is the old logic constructs the new CD by manipulating the
existing CD in various ways "in place" that ends up creating CDs that
don't meet the requirements for the new programmer. For instance EPD0
will be set and the TTB0 will also be left programmed.

> If so, I think it’d be better to make that clear, also as used_bits
> are always 0xff for all cases, I believe the EPD0 logic should be
> integrated in populating the CD so it is correct by construction, as
> this looks like a hack to me.

Yes, this is what happens, in a few more steps. We have to go and
build the missing make functions first.

There is a bit of a circular problem here: the new scheme expects that
the CD is only programmed by the new scheme and follows the rules - eg
no unused bits set. While the old scheme doesn't follow the rules.

So this patch makes the old scheme follow the rules and be compatible
with the new scheme then we go place by place and convert to the new
scheme. Then we remove the old scheme entirely. Look at the "Move the
CD generation for SVA into a function" patch.

Yes, this is a minimal hack to let the next few patches work out
correctly without breaking bisection.

How about a new commit message:

iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry()

CD table entries and STE's have the same essential programming sequence,
just with different types. Use the new ops indirection to link CD
programming to the common writer.

In a few more patches all CD writers will call an appropriate make
function and then directly call arm_smmu_write_cd_entry().
arm_smmu_write_ctx_desc() will be removed.

Until then lightly tweak arm_smmu_write_ctx_desc() to also use the new
programmer by using the same logic as right now to build the target CD on
the stack, sanitizing it to meet the used rules, and then using the
writer.

This is necessary because the writer expects that the currently programmed
CD follows the used rules. Next patches add new make functions and new
direct calls to arm_smmu_write_cd_entry() which will require this.

Jason

Mostafa Saleh April 27, 2024, 10:08 p.m. UTC | #6

On Mon, Apr 22, 2024 at 10:29:54AM -0300, Jason Gunthorpe wrote:
> On Fri, Apr 19, 2024 at 09:07:19PM +0000, Mostafa Saleh wrote:
> > > -	cdptr = arm_smmu_get_cd_ptr(master, ssid);
> > > -	if (!cdptr)
> > > +	cd_table_entry = arm_smmu_get_cd_ptr(master, ssid);
> > > +	if (!cd_table_entry)
> > >  		return -ENOMEM;
> > >  
> > > +	target = *cd_table_entry;
> > 
> > As this changes the logic where all CD manipulation is not on the actual
> > CD, I believe a comment would be helpful here.
> 
> This is all deleted in a few patches, doesn't seem worth it to
> me. These steps exist only for bisection.
> 
> > > @@ -1299,18 +1357,14 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
> > >  		if (cd_table->stall_enabled)
> > >  			val |= CTXDESC_CD_0_S;
> > >  	}
> > > -
> > > +	cdptr->data[0] = cpu_to_le64(val);
> > >  	/*
> > > -	 * The SMMU accesses 64-bit values atomically. See IHI0070Ca 3.21.3
> > > -	 * "Configuration structures and configuration invalidation completion"
> > > -	 *
> > > -	 *   The size of single-copy atomic reads made by the SMMU is
> > > -	 *   IMPLEMENTATION DEFINED but must be at least 64 bits. Any single
> > > -	 *   field within an aligned 64-bit span of a structure can be altered
> > > -	 *   without first making the structure invalid.
> > > +	 * Since the above is updating the CD entry based on the current value
> > > +	 * without zeroing unused bits it needs fixing before being passed to
> > > +	 * the programming logic.
> > >  	 */
> > > -	WRITE_ONCE(cdptr->data[0], cpu_to_le64(val));
> > > -	arm_smmu_sync_cd(master, ssid, true);
> > > +	arm_smmu_clean_cd_entry(&target);
> > 
> > I am not sure I understand the logic here, is that only needed for entry[0]
> > As I see the other entries are set and not reused.
> 
> I'm not sure what you are asking?
> 
> The issue is the old logic constructs the new CD by manipulating the
> existing CD in various ways "in place" that ends up creating CDs that
> don't meet the requirements for the new programmer. For instance EPD0
> will be set and the TTB0 will also be left programmed.
> 

I see, but what I don’t understand is why doesn't the function construct
the CD correctly, as from
	} else if (cd == &quiet_cd) { /* (4) */
		if (!(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
			val &= ~(CTXDESC_CD_0_S | CTXDESC_CD_0_R);
		val |= CTXDESC_CD_0_TCR_EPD0;
		// populate the rest of the CD correctly here.
	}

As I  don’t think the right approach is to populate the CD incorrectly
and then clear the parts not needed for EPD0.
Also, TTB0 is ignored anyway in that case, no?

Thanks,
Mostafa

> > If so, I think it’d be better to make that clear, also as used_bits
> > are always 0xff for all cases, I believe the EPD0 logic should be
> > integrated in populating the CD so it is correct by construction, as
> > this looks like a hack to me.
> 
> Yes, this is what happens, in a few more steps. We have to go and
> build the missing make functions first.
> 
> There is a bit of a circular problem here: the new scheme expects that
> the CD is only programmed by the new scheme and follows the rules - eg
> no unused bits set. While the old scheme doesn't follow the rules.
> 
> So this patch makes the old scheme follow the rules and be compatible
> with the new scheme then we go place by place and convert to the new
> scheme. Then we remove the old scheme entirely. Look at the "Move the
> CD generation for SVA into a function" patch.
> 
> Yes, this is a minimal hack to let the next few patches work out
> correctly without breaking bisection.
> 
> How about a new commit message:
> 
> iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry()
> 
> CD table entries and STE's have the same essential programming sequence,
> just with different types. Use the new ops indirection to link CD
> programming to the common writer.
> 
> In a few more patches all CD writers will call an appropriate make
> function and then directly call arm_smmu_write_cd_entry().
> arm_smmu_write_ctx_desc() will be removed.
> 
> Until then lightly tweak arm_smmu_write_ctx_desc() to also use the new
> programmer by using the same logic as right now to build the target CD on
> the stack, sanitizing it to meet the used rules, and then using the
> writer.
> 
> This is necessary because the writer expects that the currently programmed
> CD follows the used rules. Next patches add new make functions and new
> direct calls to arm_smmu_write_cd_entry() which will require this.
> 
> Jason

Jason Gunthorpe April 29, 2024, 2:29 p.m. UTC | #7

On Sat, Apr 27, 2024 at 10:08:57PM +0000, Mostafa Saleh wrote:
> > The issue is the old logic constructs the new CD by manipulating the
> > existing CD in various ways "in place" that ends up creating CDs that
> > don't meet the requirements for the new programmer. For instance EPD0
> > will be set and the TTB0 will also be left programmed.
> > 
> 
> I see, but what I don’t understand is why doesn't the function construct
> the CD correctly, as from

Why? Because it never had to before. It made minimal edits to minimize
the code.

> 	} else if (cd == &quiet_cd) { /* (4) */
> 		if (!(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
> 			val &= ~(CTXDESC_CD_0_S | CTXDESC_CD_0_R);
> 		val |= CTXDESC_CD_0_TCR_EPD0;
> 		// populate the rest of the CD correctly here.
> 	}

What you are asking for is this:

        cd_live = !!(val & CTXDESC_CD_0_V);
 
        if (!cd) { /* (5) */
+               memset(cdptr, 0, sizeof(*cdptr));
                val = 0;
        } else if (cd == &quiet_cd) { /* (4) */
+               val &= ~(CTXDESC_CD_0_TCR_T0SZ | CTXDESC_CD_0_TCR_TG0 |
+                        CTXDESC_CD_0_TCR_IRGN0 | CTXDESC_CD_0_TCR_ORGN0 |
+                        CTXDESC_CD_0_TCR_SH0);
                if (!(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
                        val &= ~(CTXDESC_CD_0_S | CTXDESC_CD_0_R);
                val |= CTXDESC_CD_0_TCR_EPD0;
+               cdptr->data[1] &= ~cpu_to_le64(CTXDESC_CD_1_TTB0_MASK);
        } else if (cd_live) { /* (3) */
                val &= ~CTXDESC_CD_0_ASID;
                val |= FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid);

I think.. I've been staring at this a while now and I *think* it
covers all the cases and we won't hit the WARN_ON?

So sure, lets do it that way, the code is all deleted anyhow ..

> As I  don’t think the right approach is to populate the CD incorrectly
> and then clear the parts not needed for EPD0.

It is very easy to see that such a simple algorithm will not trigger
the WARN_ON. The above is somewhat trickier.

> Also, TTB0 is ignored anyway in that case, no?

Only by HW, there is a protective WARN_ON that will trigger in the
programmer, that is what this is trying to avoid. For bisection.

Jason

Mostafa Saleh April 29, 2024, 3:30 p.m. UTC | #8

On Mon, Apr 29, 2024 at 11:29:05AM -0300, Jason Gunthorpe wrote:
> On Sat, Apr 27, 2024 at 10:08:57PM +0000, Mostafa Saleh wrote:
> > > The issue is the old logic constructs the new CD by manipulating the
> > > existing CD in various ways "in place" that ends up creating CDs that
> > > don't meet the requirements for the new programmer. For instance EPD0
> > > will be set and the TTB0 will also be left programmed.
> > > 
> > 
> > I see, but what I don’t understand is why doesn't the function construct
> > the CD correctly, as from
> 
> Why? Because it never had to before. It made minimal edits to minimize
> the code.

I understand, my point was why don’t we introduce a new logic to construct it
correctly, instead of hacking the old one, as it is much easier to reason
about (at least from my point of view)

> 
> > 	} else if (cd == &quiet_cd) { /* (4) */
> > 		if (!(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
> > 			val &= ~(CTXDESC_CD_0_S | CTXDESC_CD_0_R);
> > 		val |= CTXDESC_CD_0_TCR_EPD0;
> > 		// populate the rest of the CD correctly here.
> > 	}
> 
> What you are asking for is this:
> 
>         cd_live = !!(val & CTXDESC_CD_0_V);
>  
>         if (!cd) { /* (5) */
> +               memset(cdptr, 0, sizeof(*cdptr));
>                 val = 0;
>         } else if (cd == &quiet_cd) { /* (4) */
> +               val &= ~(CTXDESC_CD_0_TCR_T0SZ | CTXDESC_CD_0_TCR_TG0 |
> +                        CTXDESC_CD_0_TCR_IRGN0 | CTXDESC_CD_0_TCR_ORGN0 |
> +                        CTXDESC_CD_0_TCR_SH0);
>                 if (!(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
>                         val &= ~(CTXDESC_CD_0_S | CTXDESC_CD_0_R);
>                 val |= CTXDESC_CD_0_TCR_EPD0;
> +               cdptr->data[1] &= ~cpu_to_le64(CTXDESC_CD_1_TTB0_MASK);
>         } else if (cd_live) { /* (3) */
>                 val &= ~CTXDESC_CD_0_ASID;
>                 val |= FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid);
> 
> I think.. I've been staring at this a while now and I *think* it
> covers all the cases and we won't hit the WARN_ON?
> 

That’s similar to how I imagined it.

> So sure, lets do it that way, the code is all deleted anyhow ..
> 

I agree, if it's deleted anyway we shouldn't put much time, I haven't
looked at the SVA patch yet.

> > As I  don’t think the right approach is to populate the CD incorrectly
> > and then clear the parts not needed for EPD0.
> 
> It is very easy to see that such a simple algorithm will not trigger
> the WARN_ON. The above is somewhat trickier.
> 
> > Also, TTB0 is ignored anyway in that case, no?
> 
> Only by HW, there is a protective WARN_ON that will trigger in the
> programmer, that is what this is trying to avoid. For bisection.

Makes sense.

Thanks,
Mostafa
> Jason

Patch

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 bf105e914d38b1..3983de90c2fa01 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -56,6 +56,7 @@  struct arm_smmu_entry_writer_ops {
 
 #define NUM_ENTRY_QWORDS 8
 static_assert(sizeof(struct arm_smmu_ste) == NUM_ENTRY_QWORDS * sizeof(u64));
+static_assert(sizeof(struct arm_smmu_cd) == NUM_ENTRY_QWORDS * sizeof(u64));
 
 static phys_addr_t arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = {
 	[EVTQ_MSI_INDEX] = {
@@ -1231,6 +1232,67 @@  static struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
 	return &l1_desc->l2ptr[idx];
 }
 
+struct arm_smmu_cd_writer {
+	struct arm_smmu_entry_writer writer;
+	unsigned int ssid;
+};
+
+static void arm_smmu_get_cd_used(const __le64 *ent, __le64 *used_bits)
+{
+	used_bits[0] = cpu_to_le64(CTXDESC_CD_0_V);
+	if (!(ent[0] & cpu_to_le64(CTXDESC_CD_0_V)))
+		return;
+	memset(used_bits, 0xFF, sizeof(struct arm_smmu_cd));
+
+	/* EPD0 means T0SZ/TG0/IR0/OR0/SH0/TTB0 are IGNORED */
+	if (ent[0] & cpu_to_le64(CTXDESC_CD_0_TCR_EPD0)) {
+		used_bits[0] &= ~cpu_to_le64(
+			CTXDESC_CD_0_TCR_T0SZ | CTXDESC_CD_0_TCR_TG0 |
+			CTXDESC_CD_0_TCR_IRGN0 | CTXDESC_CD_0_TCR_ORGN0 |
+			CTXDESC_CD_0_TCR_SH0);
+		used_bits[1] &= ~cpu_to_le64(CTXDESC_CD_1_TTB0_MASK);
+	}
+}
+
+static void arm_smmu_cd_writer_sync_entry(struct arm_smmu_entry_writer *writer)
+{
+	struct arm_smmu_cd_writer *cd_writer =
+		container_of(writer, struct arm_smmu_cd_writer, writer);
+
+	arm_smmu_sync_cd(writer->master, cd_writer->ssid, true);
+}
+
+static const struct arm_smmu_entry_writer_ops arm_smmu_cd_writer_ops = {
+	.sync = arm_smmu_cd_writer_sync_entry,
+	.get_used = arm_smmu_get_cd_used,
+	.v_bit = cpu_to_le64(CTXDESC_CD_0_V),
+};
+
+static void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
+				    struct arm_smmu_cd *cdptr,
+				    const struct arm_smmu_cd *target)
+{
+	struct arm_smmu_cd_writer cd_writer = {
+		.writer = {
+			.ops = &arm_smmu_cd_writer_ops,
+			.master = master,
+		},
+		.ssid = ssid,
+	};
+
+	arm_smmu_write_entry(&cd_writer.writer, cdptr->data, target->data);
+}
+
+static void arm_smmu_clean_cd_entry(struct arm_smmu_cd *target)
+{
+	struct arm_smmu_cd used = {};
+	int i;
+
+	arm_smmu_get_cd_used(target->data, used.data);
+	for (i = 0; i != ARRAY_SIZE(target->data); i++)
+		target->data[i] &= used.data[i];
+}
+
 int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
 			    struct arm_smmu_ctx_desc *cd)
 {
@@ -1247,17 +1309,20 @@  int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
 	 */
 	u64 val;
 	bool cd_live;
-	struct arm_smmu_cd *cdptr;
+	struct arm_smmu_cd target;
+	struct arm_smmu_cd *cdptr = ⌖
+	struct arm_smmu_cd *cd_table_entry;
 	struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
 	struct arm_smmu_device *smmu = master->smmu;
 
 	if (WARN_ON(ssid >= (1 << cd_table->s1cdmax)))
 		return -E2BIG;
 
-	cdptr = arm_smmu_get_cd_ptr(master, ssid);
-	if (!cdptr)
+	cd_table_entry = arm_smmu_get_cd_ptr(master, ssid);
+	if (!cd_table_entry)
 		return -ENOMEM;
 
+	target = *cd_table_entry;
 	val = le64_to_cpu(cdptr->data[0]);
 	cd_live = !!(val & CTXDESC_CD_0_V);
 
@@ -1279,13 +1344,6 @@  int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
 		cdptr->data[2] = 0;
 		cdptr->data[3] = cpu_to_le64(cd->mair);
 
-		/*
-		 * STE may be live, and the SMMU might read dwords of this CD in any
-		 * order. Ensure that it observes valid values before reading
-		 * V=1.
-		 */
-		arm_smmu_sync_cd(master, ssid, true);
-
 		val = cd->tcr |
 #ifdef __BIG_ENDIAN
 			CTXDESC_CD_0_ENDI |
@@ -1299,18 +1357,14 @@  int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
 		if (cd_table->stall_enabled)
 			val |= CTXDESC_CD_0_S;
 	}
-
+	cdptr->data[0] = cpu_to_le64(val);
 	/*
-	 * The SMMU accesses 64-bit values atomically. See IHI0070Ca 3.21.3
-	 * "Configuration structures and configuration invalidation completion"
-	 *
-	 *   The size of single-copy atomic reads made by the SMMU is
-	 *   IMPLEMENTATION DEFINED but must be at least 64 bits. Any single
-	 *   field within an aligned 64-bit span of a structure can be altered
-	 *   without first making the structure invalid.
+	 * Since the above is updating the CD entry based on the current value
+	 * without zeroing unused bits it needs fixing before being passed to
+	 * the programming logic.
 	 */
-	WRITE_ONCE(cdptr->data[0], cpu_to_le64(val));
-	arm_smmu_sync_cd(master, ssid, true);
+	arm_smmu_clean_cd_entry(&target);
+	arm_smmu_write_cd_entry(master, ssid, cd_table_entry, &target);
 	return 0;
 }