[PATCH v5 05/27] iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry()

[Jason Gunthorpe]

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