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

From: Suthikulpanit, Suravee
Date: Thu Oct 03 2024 - 12:18:28 EST


On 9/27/2024 2:46 AM, Jason Gunthorpe wrote:
On Mon, Sep 16, 2024 at 05:18:01PM +0000, Suravee Suthikulpanit wrote:

....

+ 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.

Not sure if I follow this.

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

I will take care of this later once we introduce the change for 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..

Okey.

Thanks,
Suravee