| Message ID | 5-v5-9a37e0c884ce+31e3-smmuv3_newapi_p2_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update SMMUv3 to the modern iommu API (part 2/3) | expand |
On Mon, Mar 04, 2024 at 07:43:53PM -0400, Jason Gunthorpe wrote: > CD table entries and STE's have the same essential programming sequence, > just with different types and sizes. Hmm.. I somehow remember that one of them was 4 qwords while the other was 8? Yet now, in the final driver source code on the smmuv3_newapi branch, they are both 8 qwords? Feels like some of the statement and the change doesn't match with the reality so well -- hopefully I am not missing something... Thanks Nicolin
On Mon, Mar 04, 2024 at 07:43:53PM -0400, Jason Gunthorpe wrote: > CD table entries and STE's have the same essential programming sequence, > just with different types and sizes. > 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. > Currently the logic can't ensure that the CD always conforms to the used > requirements until all the CD generation is moved to the new method. Add a > temporary no_used_check to disable the assertions. > Signed-off-by: Michael Shavit <mshavit@google.com> > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 101 ++++++++++++++------ > 1 file changed, 74 insertions(+), 27 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 b7f947e36f596f..237fd6d92c880b 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -57,11 +57,14 @@ struct arm_smmu_entry_writer { > struct arm_smmu_entry_writer_ops { > unsigned int num_entry_qwords; > __le64 v_bit; > + bool no_used_check; > void (*get_used)(const __le64 *entry, __le64 *used); > void (*sync)(struct arm_smmu_entry_writer *writer); > }; > -#define NUM_ENTRY_QWORDS (sizeof(struct arm_smmu_ste) / sizeof(u64)) > +#define NUM_ENTRY_QWORDS \ > + (max(sizeof(struct arm_smmu_ste), sizeof(struct arm_smmu_cd)) / \ > + sizeof(u64)) > static phys_addr_t arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = { > [EVTQ_MSI_INDEX] = { > @@ -1056,7 +1059,8 @@ static u8 arm_smmu_entry_qword_diff(struct > arm_smmu_entry_writer *writer, > * allowed to set a bit to 1 if the used function doesn't say it > * is used. > */ > - WARN_ON_ONCE(target[i] & ~target_used[i]); > + if (!writer->ops->no_used_check) > + WARN_ON_ONCE(target[i] & ~target_used[i]); > /* Bits can change because they are not currently being used */ > unused_update[i] = (entry[i] & cur_used[i]) | > @@ -1065,7 +1069,8 @@ static u8 arm_smmu_entry_qword_diff(struct > arm_smmu_entry_writer *writer, > * Each bit indicates that a used bit in a qword needs to be > * changed after unused_update is applied. > */ > - if ((unused_update[i] & target_used[i]) != target[i]) > + if ((unused_update[i] & target_used[i]) != > + (target[i] & target_used[i])) > used_qword_diff |= 1 << i; > } > return used_qword_diff; > @@ -1161,8 +1166,11 @@ static void arm_smmu_write_entry(struct > arm_smmu_entry_writer *writer, > * in the entry. The target was already sanity checked by > * compute_qword_diff(). > */ > - WARN_ON_ONCE( > - entry_set(writer, entry, target, 0, num_entry_qwords)); > + if (writer->ops->no_used_check) > + entry_set(writer, entry, target, 0, num_entry_qwords); > + else > + WARN_ON_ONCE(entry_set(writer, entry, target, 0, > + num_entry_qwords)); > } > } > @@ -1242,6 +1250,59 @@ 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), > + .no_used_check = true, > + .num_entry_qwords = sizeof(struct arm_smmu_cd) / sizeof(u64), > +}; > + > +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); > +} > + > int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid, > struct arm_smmu_ctx_desc *cd) > { > @@ -1258,17 +1319,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); > @@ -1290,13 +1354,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 | > @@ -1310,18 +1367,8 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master > *master, int ssid, > if (cd_table->stall_enabled) > val |= CTXDESC_CD_0_S; > } > - > - /* > - * 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. > - */ > - WRITE_ONCE(cdptr->data[0], cpu_to_le64(val)); > - arm_smmu_sync_cd(master, ssid, true); > + cdptr->data[0] = cpu_to_le64(val); > + arm_smmu_write_cd_entry(master, ssid, cd_table_entry, &target); > return 0; > } > -- > 2.43.2 Reviewed-by: Moritz Fischer <moritzf@google.com>
On Fri, Mar 15, 2024 at 12:52:43AM -0700, Nicolin Chen wrote: > On Mon, Mar 04, 2024 at 07:43:53PM -0400, Jason Gunthorpe wrote: > > CD table entries and STE's have the same essential programming sequence, > > just with different types and sizes. > > Hmm.. I somehow remember that one of them was 4 qwords while > the other was 8? Yet now, in the final driver source code on > the smmuv3_newapi branch, they are both 8 qwords? Yeah they are both 8, it is a mistake in the commit message. I fixed it. Thanks, Jason
Hi Jason, On Mon, Mar 04, 2024 at 07:43:53PM -0400, Jason Gunthorpe wrote: > CD table entries and STE's have the same essential programming sequence, > just with different types and sizes. > > 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. > > Currently the logic can't ensure that the CD always conforms to the used > requirements until all the CD generation is moved to the new method. Add a > temporary no_used_check to disable the assertions. > I am still going through the patches, but is it possible to reorder/squash to avoid that, so it is easier to review? > Signed-off-by: Michael Shavit <mshavit@google.com> > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 101 ++++++++++++++------ > 1 file changed, 74 insertions(+), 27 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 b7f947e36f596f..237fd6d92c880b 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -57,11 +57,14 @@ struct arm_smmu_entry_writer { > struct arm_smmu_entry_writer_ops { > unsigned int num_entry_qwords; > __le64 v_bit; > + bool no_used_check; > void (*get_used)(const __le64 *entry, __le64 *used); > void (*sync)(struct arm_smmu_entry_writer *writer); > }; > > -#define NUM_ENTRY_QWORDS (sizeof(struct arm_smmu_ste) / sizeof(u64)) > +#define NUM_ENTRY_QWORDS \ > + (max(sizeof(struct arm_smmu_ste), sizeof(struct arm_smmu_cd)) / \ > + sizeof(u64)) > > static phys_addr_t arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = { > [EVTQ_MSI_INDEX] = { > @@ -1056,7 +1059,8 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer, > * allowed to set a bit to 1 if the used function doesn't say it > * is used. > */ > - WARN_ON_ONCE(target[i] & ~target_used[i]); > + if (!writer->ops->no_used_check) > + WARN_ON_ONCE(target[i] & ~target_used[i]); > > /* Bits can change because they are not currently being used */ > unused_update[i] = (entry[i] & cur_used[i]) | > @@ -1065,7 +1069,8 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer, > * Each bit indicates that a used bit in a qword needs to be > * changed after unused_update is applied. > */ > - if ((unused_update[i] & target_used[i]) != target[i]) > + if ((unused_update[i] & target_used[i]) != > + (target[i] & target_used[i])) > used_qword_diff |= 1 << i; > } > return used_qword_diff; > @@ -1161,8 +1166,11 @@ static void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, > * in the entry. The target was already sanity checked by > * compute_qword_diff(). > */ > - WARN_ON_ONCE( > - entry_set(writer, entry, target, 0, num_entry_qwords)); > + if (writer->ops->no_used_check) > + entry_set(writer, entry, target, 0, num_entry_qwords); > + else > + WARN_ON_ONCE(entry_set(writer, entry, target, 0, > + num_entry_qwords)); > } > } > > @@ -1242,6 +1250,59 @@ 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)); This is a slightly different approach than what the driver does for STEs, where it explicitly sets the used bits. Is there a reason for that? > + > + /* 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); > + } > +} We should add a comment about EPD1 maybe? > + > +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), > + .no_used_check = true, > + .num_entry_qwords = sizeof(struct arm_smmu_cd) / sizeof(u64), > +}; > + > +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); > +} > + > int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid, > struct arm_smmu_ctx_desc *cd) > { > @@ -1258,17 +1319,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); > > @@ -1290,13 +1354,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 | > @@ -1310,18 +1367,8 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid, > if (cd_table->stall_enabled) > val |= CTXDESC_CD_0_S; > } > - > - /* > - * 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. > - */ > - WRITE_ONCE(cdptr->data[0], cpu_to_le64(val)); > - arm_smmu_sync_cd(master, ssid, true); > + cdptr->data[0] = cpu_to_le64(val); > + arm_smmu_write_cd_entry(master, ssid, cd_table_entry, &target); > return 0; > } > > -- > 2.43.2 Thanks, Mostafa
On Sat, Mar 23, 2024 at 01:02:15PM +0000, Mostafa Saleh wrote: > Hi Jason, > > On Mon, Mar 04, 2024 at 07:43:53PM -0400, Jason Gunthorpe wrote: > > CD table entries and STE's have the same essential programming sequence, > > just with different types and sizes. > > > > 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. > > > > Currently the logic can't ensure that the CD always conforms to the used > > requirements until all the CD generation is moved to the new method. Add a > > temporary no_used_check to disable the assertions. > > > > I am still going through the patches, but is it possible to > reorder/squash to avoid that, so it is easier to review? After Nicolin's remark I changed this one use a temporary helper to 0 the unused bits then we delete the helper instead of touching the machinery itself. It is much clearer. We can't avoid this with patch ordering because the progression is to move CD generation out of arm_smmu_write_ctx_desc() type-by-type then delete it. In the mean time we need to use the new write logic in all cases because I'm not sure the old/new schemes have compatible assumptions for the existing arm_smmu_write_ctx_desc() to be safe. Thanks, Jason
On Sat, Mar 23, 2024 at 01:02:15PM +0000, Mostafa Saleh wrote: > > +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)); > > This is a slightly different approach than what the driver does for STEs, > where it explicitly sets the used bits. Is there a reason for that? It is just more compact this way > > + /* 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); > > + } > > +} > > We should add a comment about EPD1 maybe? Driver doesn't use TTB1? Jason
On Tue, Mar 26, 2024 at 03:30:55PM -0300, Jason Gunthorpe wrote: > On Sat, Mar 23, 2024 at 01:02:15PM +0000, Mostafa Saleh wrote: > > > +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)); > > > > This is a slightly different approach than what the driver does for STEs, > > where it explicitly sets the used bits. Is there a reason for that? > > It is just more compact this way IMHO, it seems too much to have this mechanism for CDs for just one SVA case, but I'll need to go through the whole seires first to make sure I am not missing anything. > > > + /* 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); > > > + } > > > +} > > > > We should add a comment about EPD1 maybe? > > Driver doesn't use TTB1? Yes, it's not immediately obvious why we ignore EPD1, so maybe it's worth a comment to highlight that, but no strong opinion. Thanks, Mostafa
On Tue, Mar 26, 2024 at 07:12:53PM +0000, Mostafa Saleh wrote: > On Tue, Mar 26, 2024 at 03:30:55PM -0300, Jason Gunthorpe wrote: > > On Sat, Mar 23, 2024 at 01:02:15PM +0000, Mostafa Saleh wrote: > > > > +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)); > > > > > > This is a slightly different approach than what the driver does for STEs, > > > where it explicitly sets the used bits. Is there a reason for that? > > > > It is just more compact this way > > IMHO, it seems too much to have this mechanism for CDs for just one > SVA case, but I'll need to go through the whole seires first to make > sure I am not missing anything. It is pretty ugly if you try to do it that way. You still need to create some ops because the entry_set should be re-used (I mean I guess you could copy it as well). Then you have to open code the logic. And then the EPD0 path is somewhat fragile. Something sort of like this: void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, struct arm_smmu_cd *cdptr, const struct arm_smmu_cd *target) { bool target_valid = target->data[0] & cpu_to_le64(CTXDESC_CD_0_V); bool cur_valid = cdptr->data[0] & cpu_to_le64(CTXDESC_CD_0_V); struct arm_smmu_cd_writer cd_writer = { .writer = { .ops = &arm_smmu_cd_writer_ops, .master = master, }, .ssid = ssid, }; if (ssid != IOMMU_NO_PASID && cur_valid != target_valid) { if (cur_valid) master->cd_table.used_ssids--; else master->cd_table.used_ssids++; } /* Force a V=0/V=1 update*/ __le64 update = target[0] & ~cpu_to_le64(CTXDESC_CD_0_V); entry_set(&cd_writer.writer, cdptr->data, &update, 0, 1); entry_set(&cd_writer.writer, cdptr->data, target->data, 1, NUM_ENTRY_QWORDS - 1); entry_set(&cd_writer.writer, cdptr->data, target->data, 0, 1); } void arm_smmu_write_cd_entry_epd0(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, }; /* * Target must the EPD0 = 1 version of the existing CD entry, caller * must enforce it. Assume used_ssids doesn't need updating * for this reason. */ /* Update EPD0 */ entry_set(&cd_writer.writer, cdptr->data, target->data, 0, 1); /* Update everthing else */ entry_set(&cd_writer.writer, cdptr->data, target->data, 0, NUM_ENTRY_QWORDS - 1); } IMOH, at this point it is saner to have just implemented the used function and use the mechanism robustly. Less special cases, less fragility, less duplication. Jason
On Tue, Mar 26, 2024 at 07:27:58PM -0300, Jason Gunthorpe wrote: > On Tue, Mar 26, 2024 at 07:12:53PM +0000, Mostafa Saleh wrote: > > On Tue, Mar 26, 2024 at 03:30:55PM -0300, Jason Gunthorpe wrote: > > > On Sat, Mar 23, 2024 at 01:02:15PM +0000, Mostafa Saleh wrote: > > > > > +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)); > > > > > > > > This is a slightly different approach than what the driver does for STEs, > > > > where it explicitly sets the used bits. Is there a reason for that? > > > > > > It is just more compact this way > > > > IMHO, it seems too much to have this mechanism for CDs for just one > > SVA case, but I'll need to go through the whole seires first to make > > sure I am not missing anything. > > It is pretty ugly if you try to do it that way. You still need to > create some ops because the entry_set should be re-used (I mean I > guess you could copy it as well). Then you have to open code the > logic. And then the EPD0 path is somewhat fragile. Something sort of > like this: > > void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, > struct arm_smmu_cd *cdptr, > const struct arm_smmu_cd *target) > { > bool target_valid = target->data[0] & cpu_to_le64(CTXDESC_CD_0_V); > bool cur_valid = cdptr->data[0] & cpu_to_le64(CTXDESC_CD_0_V); > struct arm_smmu_cd_writer cd_writer = { > .writer = { > .ops = &arm_smmu_cd_writer_ops, > .master = master, > }, > .ssid = ssid, > }; > > if (ssid != IOMMU_NO_PASID && cur_valid != target_valid) { > if (cur_valid) > master->cd_table.used_ssids--; > else > master->cd_table.used_ssids++; > } > > /* Force a V=0/V=1 update*/ > __le64 update = target[0] & ~cpu_to_le64(CTXDESC_CD_0_V); > entry_set(&cd_writer.writer, cdptr->data, &update, 0, 1); > entry_set(&cd_writer.writer, cdptr->data, target->data, 1, NUM_ENTRY_QWORDS - 1); > entry_set(&cd_writer.writer, cdptr->data, target->data, 0, 1); > } > > void arm_smmu_write_cd_entry_epd0(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, > }; > > /* > * Target must the EPD0 = 1 version of the existing CD entry, caller > * must enforce it. Assume used_ssids doesn't need updating > * for this reason. > */ > /* Update EPD0 */ > entry_set(&cd_writer.writer, cdptr->data, target->data, 0, 1); > /* Update everthing else */ > entry_set(&cd_writer.writer, cdptr->data, target->data, 0, NUM_ENTRY_QWORDS - 1); > } > > IMOH, at this point it is saner to have just implemented the used > function and use the mechanism robustly. Less special cases, less > fragility, less duplication. > But that adds extra cost of adding ops, indirection, modifying STE code..., for a case that is not common, so I think special casing it is actually better for readability and maintainability. But again, I need to finish going through the series to get the full context. Thanks, Mostafa
On Wed, Mar 27, 2024 at 09:45:03AM +0000, Mostafa Saleh wrote: > On Tue, Mar 26, 2024 at 07:27:58PM -0300, Jason Gunthorpe wrote: > > On Tue, Mar 26, 2024 at 07:12:53PM +0000, Mostafa Saleh wrote: > > > On Tue, Mar 26, 2024 at 03:30:55PM -0300, Jason Gunthorpe wrote: > > > > On Sat, Mar 23, 2024 at 01:02:15PM +0000, Mostafa Saleh wrote: > > > > > > +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)); > > > > > > > > > > This is a slightly different approach than what the driver does for STEs, > > > > > where it explicitly sets the used bits. Is there a reason for that? > > > > > > > > It is just more compact this way > > > > > > IMHO, it seems too much to have this mechanism for CDs for just one > > > SVA case, but I'll need to go through the whole seires first to make > > > sure I am not missing anything. > > > > It is pretty ugly if you try to do it that way. You still need to > > create some ops because the entry_set should be re-used (I mean I > > guess you could copy it as well). Then you have to open code the > > logic. And then the EPD0 path is somewhat fragile. Something sort of > > like this: > > > > void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, > > struct arm_smmu_cd *cdptr, > > const struct arm_smmu_cd *target) > > { > > bool target_valid = target->data[0] & cpu_to_le64(CTXDESC_CD_0_V); > > bool cur_valid = cdptr->data[0] & cpu_to_le64(CTXDESC_CD_0_V); > > struct arm_smmu_cd_writer cd_writer = { > > .writer = { > > .ops = &arm_smmu_cd_writer_ops, > > .master = master, > > }, > > .ssid = ssid, > > }; > > > > if (ssid != IOMMU_NO_PASID && cur_valid != target_valid) { > > if (cur_valid) > > master->cd_table.used_ssids--; > > else > > master->cd_table.used_ssids++; > > } > > > > /* Force a V=0/V=1 update*/ > > __le64 update = target[0] & ~cpu_to_le64(CTXDESC_CD_0_V); > > entry_set(&cd_writer.writer, cdptr->data, &update, 0, 1); > > entry_set(&cd_writer.writer, cdptr->data, target->data, 1, NUM_ENTRY_QWORDS - 1); > > entry_set(&cd_writer.writer, cdptr->data, target->data, 0, 1); > > } > > > > void arm_smmu_write_cd_entry_epd0(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, > > }; > > > > /* > > * Target must the EPD0 = 1 version of the existing CD entry, caller > > * must enforce it. Assume used_ssids doesn't need updating > > * for this reason. > > */ > > /* Update EPD0 */ > > entry_set(&cd_writer.writer, cdptr->data, target->data, 0, 1); > > /* Update everthing else */ > > entry_set(&cd_writer.writer, cdptr->data, target->data, 0, NUM_ENTRY_QWORDS - 1); > > } > > > > IMOH, at this point it is saner to have just implemented the used > > function and use the mechanism robustly. Less special cases, less > > fragility, less duplication. > > > > But that adds extra cost of adding ops, indirection, modifying STE > code..., for a case that is not common, so I think special casing it > is actually better for readability and maintainability. The above is what the special case would have to look like. This programming work is not trivial. The programmer code was always intended to be re-used for the CD and STE together so there is only one logic, not two or three copies. Reducing duplicated logic in such a tricky area is worth it, IMHO. Jason
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 b7f947e36f596f..237fd6d92c880b 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -57,11 +57,14 @@ struct arm_smmu_entry_writer { struct arm_smmu_entry_writer_ops { unsigned int num_entry_qwords; __le64 v_bit; + bool no_used_check; void (*get_used)(const __le64 *entry, __le64 *used); void (*sync)(struct arm_smmu_entry_writer *writer); }; -#define NUM_ENTRY_QWORDS (sizeof(struct arm_smmu_ste) / sizeof(u64)) +#define NUM_ENTRY_QWORDS \ + (max(sizeof(struct arm_smmu_ste), sizeof(struct arm_smmu_cd)) / \ + sizeof(u64)) static phys_addr_t arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = { [EVTQ_MSI_INDEX] = { @@ -1056,7 +1059,8 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer, * allowed to set a bit to 1 if the used function doesn't say it * is used. */ - WARN_ON_ONCE(target[i] & ~target_used[i]); + if (!writer->ops->no_used_check) + WARN_ON_ONCE(target[i] & ~target_used[i]); /* Bits can change because they are not currently being used */ unused_update[i] = (entry[i] & cur_used[i]) | @@ -1065,7 +1069,8 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer, * Each bit indicates that a used bit in a qword needs to be * changed after unused_update is applied. */ - if ((unused_update[i] & target_used[i]) != target[i]) + if ((unused_update[i] & target_used[i]) != + (target[i] & target_used[i])) used_qword_diff |= 1 << i; } return used_qword_diff; @@ -1161,8 +1166,11 @@ static void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, * in the entry. The target was already sanity checked by * compute_qword_diff(). */ - WARN_ON_ONCE( - entry_set(writer, entry, target, 0, num_entry_qwords)); + if (writer->ops->no_used_check) + entry_set(writer, entry, target, 0, num_entry_qwords); + else + WARN_ON_ONCE(entry_set(writer, entry, target, 0, + num_entry_qwords)); } } @@ -1242,6 +1250,59 @@ 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), + .no_used_check = true, + .num_entry_qwords = sizeof(struct arm_smmu_cd) / sizeof(u64), +}; + +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); +} + int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid, struct arm_smmu_ctx_desc *cd) { @@ -1258,17 +1319,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); @@ -1290,13 +1354,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 | @@ -1310,18 +1367,8 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid, if (cd_table->stall_enabled) val |= CTXDESC_CD_0_S; } - - /* - * 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. - */ - WRITE_ONCE(cdptr->data[0], cpu_to_le64(val)); - arm_smmu_sync_cd(master, ssid, true); + cdptr->data[0] = cpu_to_le64(val); + arm_smmu_write_cd_entry(master, ssid, cd_table_entry, &target); return 0; }