Re: [PATCH v4 2/6] iommu/amd: Introduce helper function to update 256-bit DTE

[Jason Gunthorpe]

On Mon, Sep 16, 2024 at 05:18:01PM +0000, Suravee Suthikulpanit wrote:

> +static void write_lower(struct dev_table_entry *ptr, struct dev_table_entry *new)
> +{
> +	struct dev_table_entry old = {};
> +
> +	do {
> +		old.data128[0] = ptr->data128[0];
> +	} while (!try_cmpxchg128(&ptr->data128[0], &old.data128[0], new->data128[0]));
> +}
> +
> +/*
> + * Note:
> + * IOMMU reads the entire Device Table entry in a single 256-bit transaction
> + * but the driver is programming DTE using 2 128-bit cmpxchg. So, the driver
> + * need to ensure the following:

I wonder if the intention here was to use a SSE operation to do the
256 bit store from the CPU side too? Just thinking aloud

> +	if (!(ptr->data[0] & DTE_FLAG_V)) {
> +		/* Existing DTE is not valid. */
> +		write_upper(ptr, new);
> +		write_lower(ptr, new);
> +		iommu_flush_sync_dte(iommu, dev_data->devid);
> +	} else if (!(new->data[0] & DTE_FLAG_V)) {
> +		/* Existing DTE is valid. New DTE is not valid.  */
> +		write_lower(ptr, new);
> +		write_upper(ptr, new);
> +		iommu_flush_sync_dte(iommu, dev_data->devid);
> +	} else {
> +		/* Existing & new DTEs are valid. */
> +		if (!FIELD_GET(DTE_FLAG_GV, ptr->data[0])) {
> +			/* Existing DTE has no guest page table. */
> +			write_upper(ptr, new);
> +			write_lower(ptr, new);
> +			iommu_flush_sync_dte(iommu, dev_data->devid);
> +		} else if (!FIELD_GET(DTE_FLAG_GV, new->data[0])) {
> +			/*
> +			 * Existing DTE has guest page table,
> +			 * new DTE has no guest page table,
> +			 */
> +			write_lower(ptr, new);
> +			write_upper(ptr, new);
> +			iommu_flush_sync_dte(iommu, dev_data->devid);
> +		} else {
> +			/*
> +			 * Existing DTE has guest page table,
> +			 * new DTE has guest page table.
> +			 */
> +			struct dev_table_entry clear = {};
> +
> +			/* First disable DTE */
> +			write_lower(ptr, &clear);
> +			iommu_flush_sync_dte(iommu, dev_data->devid);
> +
> +			/* Then update DTE */
> +			write_upper(ptr, new);
> +			write_lower(ptr, new);
> +			iommu_flush_sync_dte(iommu, dev_data->devid);
> +		}

There is one branch missing where GV is valid in both and the [1]
doesn't change. Ie atomic replace of a GCR3 table.

And maybe this will need more branches later for the viommu stuff?

But otherwise yes this captures what is needed just fine.

Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>

> @@ -1256,6 +1342,16 @@ static int iommu_flush_dte(struct amd_iommu *iommu, u16 devid)
> +int iommu_flush_sync_dte(struct amd_iommu *iommu, u16 devid)
> +{
> +	int ret;
> +
> +	ret = iommu_flush_dte(iommu, devid);
> +	if (!ret)
> +		iommu_completion_wait(iommu);
> +	return ret;
> +}

Maybe this doesn't need to return an error since we can't handle
failure to flush DTE tables..

Jason