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

From: Jason Gunthorpe
Date: Thu Sep 26 2024 - 15:46:24 EST


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