[PATCH v5 04/27] iommu/arm-smmu-v3: Add an ops indirection to the STE code
Mostafa Saleh
smostafa at google.com
Mon Mar 25 14:01:13 PDT 2024
On Mon, Mar 25, 2024 at 11:11:32AM -0300, Jason Gunthorpe wrote:
> On Fri, Mar 22, 2024 at 06:14:24PM +0000, Mostafa Saleh wrote:
> > > @@ -1027,57 +1038,55 @@ static void arm_smmu_get_ste_used(const struct arm_smmu_ste *ent,
> > > * unused_update is an intermediate value of entry that has unused bits set to
> > > * their new values.
> > > */
> > > -static u8 arm_smmu_entry_qword_diff(const struct arm_smmu_ste *entry,
> > > - const struct arm_smmu_ste *target,
> > > - struct arm_smmu_ste *unused_update)
> > > +static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
> > > + const __le64 *entry, const __le64 *target,
> > > + __le64 *unused_update)
> > > {
> > > - struct arm_smmu_ste target_used = {};
> > > - struct arm_smmu_ste cur_used = {};
> > > + __le64 target_used[NUM_ENTRY_QWORDS] = {};
> > > + __le64 cur_used[NUM_ENTRY_QWORDS] = {};
> > This is confusing to me, the function was modified to be generic, so its has
> > args are __le64 * instead of struct arm_smmu_ste *.
>
> Right
>
> > But NUM_ENTRY_QWORDS is defined as “(sizeof(struct arm_smmu_ste) / sizeof(u64))”
> > and in the same function writer->ops->num_entry_qwords is used
> > nterchangeably,
>
> Right
>
> > I understand that this not a constant and the compiler would complain.
> > But since for any other num_entry_qwords larger than NUM_ENTRY_QWORDS it fails,
> > and we know STEs and CDs both have the same size, we simplify the code and make
> > it a constant everywhere.
>
> So you say to get rid of num_entry_qwords and just use the constant?
In my opinion, yes, that looks easier to understand, and avoids the MAX
stuff as there is no reason for the extra generalisation.
> > I see in the next patch, that this is redefined to be the max between STE and
> > CD, but again, this hardware and it never changes, so my opinion is to simplify
> > the code, as there is no need to generalize this part.
>
> Yes, we need a constant.
>
> It would look like this, it is a little bit simpler:
>
> 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 a54062faccde38..d015f41900d802 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -63,9 +63,9 @@ enum arm_smmu_msi_index {
> ARM_SMMU_MAX_MSIS,
> };
>
> -#define NUM_ENTRY_QWORDS \
> - (max(sizeof(struct arm_smmu_ste), sizeof(struct arm_smmu_cd)) / \
> - sizeof(u64))
> +#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] = {
> @@ -1045,7 +1045,7 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
> writer->ops->get_used(entry, cur_used);
> writer->ops->get_used(target, target_used);
>
> - for (i = 0; i != writer->ops->num_entry_qwords; i++) {
> + for (i = 0; i != NUM_ENTRY_QWORDS; i++) {
> /*
> * Check that masks are up to date, the make functions are not
> * allowed to set a bit to 1 if the used function doesn't say it
> @@ -1114,7 +1114,6 @@ static bool entry_set(struct arm_smmu_entry_writer *writer, __le64 *entry,
> void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *entry,
> const __le64 *target)
> {
> - unsigned int num_entry_qwords = writer->ops->num_entry_qwords;
> __le64 unused_update[NUM_ENTRY_QWORDS];
> u8 used_qword_diff;
>
> @@ -1137,9 +1136,9 @@ void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *entry,
> */
> unused_update[critical_qword_index] =
> entry[critical_qword_index];
> - entry_set(writer, entry, unused_update, 0, num_entry_qwords);
> + entry_set(writer, entry, unused_update, 0, NUM_ENTRY_QWORDS);
> entry_set(writer, entry, target, critical_qword_index, 1);
> - entry_set(writer, entry, target, 0, num_entry_qwords);
> + entry_set(writer, entry, target, 0, NUM_ENTRY_QWORDS);
> } else if (used_qword_diff) {
> /*
> * At least two qwords need their inuse bits to be changed. This
> @@ -1148,7 +1147,7 @@ void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *entry,
> */
> unused_update[0] = entry[0] & (~writer->ops->v_bit);
> entry_set(writer, entry, unused_update, 0, 1);
> - entry_set(writer, entry, target, 1, num_entry_qwords - 1);
> + entry_set(writer, entry, target, 1, NUM_ENTRY_QWORDS - 1);
> entry_set(writer, entry, target, 0, 1);
> } else {
> /*
> @@ -1157,7 +1156,7 @@ void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *entry,
> * compute_qword_diff().
> */
> WARN_ON_ONCE(
> - entry_set(writer, entry, target, 0, num_entry_qwords));
> + entry_set(writer, entry, target, 0, NUM_ENTRY_QWORDS));
> }
> }
>
> @@ -1272,7 +1271,6 @@ 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),
> - .num_entry_qwords = sizeof(struct arm_smmu_cd) / sizeof(u64),
> };
>
> void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
> @@ -1460,7 +1458,6 @@ static const struct arm_smmu_entry_writer_ops arm_smmu_ste_writer_ops = {
> .sync = arm_smmu_ste_writer_sync_entry,
> .get_used = arm_smmu_get_ste_used,
> .v_bit = cpu_to_le64(STRTAB_STE_0_V),
> - .num_entry_qwords = sizeof(struct arm_smmu_ste) / sizeof(u64),
> };
>
> static void arm_smmu_write_ste(struct arm_smmu_master *master, u32 sid,
> 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 8ba07b00bf6056..5936dc5f76786a 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -779,7 +779,6 @@ struct arm_smmu_entry_writer {
> };
>
> struct arm_smmu_entry_writer_ops {
> - unsigned int num_entry_qwords;
> __le64 v_bit;
> void (*get_used)(const __le64 *entry, __le64 *used);
> void (*sync)(struct arm_smmu_entry_writer *writer);
>
Thanks,
Mostafa
More information about the linux-arm-kernel
mailing list