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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240926194607.GP9417@nvidia.com>
Date: Thu, 26 Sep 2024 16:46:07 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
Cc: linux-kernel@...r.kernel.org, iommu@...ts.linux.dev, joro@...tes.org,
	robin.murphy@....com, vasant.hegde@....com, kevin.tian@...el.com,
	jon.grimm@....com, santosh.shukla@....com, pandoh@...gle.com,
	kumaranand@...gle.com
Subject: Re: [PATCH v4 2/6] iommu/amd: Introduce helper function to update
 256-bit DTE

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@...dia.com>

> @@ -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