Received-SPF: pass (google.com: domain of linux-kernel+bounces-97136-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 8 Mar 2024 10:04:40 -0400
From: Jason Gunthorpe <jgg@nvidia.com>
To: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: linux-kernel@vger.kernel.org, iommu@lists.linux.dev, joro@8bytes.org,
	yi.l.liu@intel.com, kevin.tian@intel.com, nicolinc@nvidia.com,
	eric.auger@redhat.com, vasant.hegde@amd.com, jon.grimm@amd.com,
	santosh.shukla@amd.com, Dhaval.Giani@amd.com, pandoh@google.com,
	loganodell@google.com
Subject: Re: [RFCv2 PATCH 6/7] iommu/amd: Add nested domain allocation support
Message-ID: <20240308140440.GN9179@nvidia.com>
References: <20240112000646.98001-1-suravee.suthikulpanit@amd.com>
 <20240112000646.98001-7-suravee.suthikulpanit@amd.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240112000646.98001-7-suravee.suthikulpanit@amd.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?v2BsVG52RObY/MVT8Eu5Q7o3CgMOAxhtiVBLTe+PCYOur0XEURusoS8r64F1?=
 =?us-ascii?Q?5pJk/BVArg0ffXBsZRBYSbXSvG1HCwFUbUADBRoz9/Jx6pkOzV0Yhlkr7Q1y?=
 =?us-ascii?Q?EyytpKyEHqVI7l1TT/1kHwxNu7oR2xb6/jXSENZq2q8+hLpvuupaPxG8Wnif?=
 =?us-ascii?Q?ZVEthDLMRpiXcPGIpGflDElux77yJ8j0CEIIW/Uj0S9iJhkMvZcfBDffM2sz?=
 =?us-ascii?Q?0iawygUVEoIZLYdmWYWY9vyh1V+5jgHqSmNXvFpbLzu6FJEMt7lnEjWaBfAY?=
 =?us-ascii?Q?l6+TpZ5pLpX/JAAhe/oa0dWLq/KFsgRV//fBpog75EFIzZNJnuAhqtCD+Lof?=
 =?us-ascii?Q?6+ugARC3YXBxgKvVjH6n4G9MW0CUEsgekCCqFLTwX3w3elgHZkdggB/4Jy+N?=
 =?us-ascii?Q?ya4jYb09Wm2/n39R6jWPZST4yZI8RxJBJnAdroGbDH0DTEiJx7rkFBvvxpjc?=
 =?us-ascii?Q?UcsLrpTNz85He92FZIZUN4b7evwLznLk8BKifwreqiPK1DqoabEArgsilaRg?=
 =?us-ascii?Q?FuJkmghgAUYJL/7ty9P0KQScgqMYu9hWKJBa0TaVAQx8seNenxXKnTe0gjmv?=
 =?us-ascii?Q?tY3MSwk6IyI9YNUrVbB4o4EbckiC9qpcQAvW40dkDGCbSorV/6LMD4WjShL2?=
 =?us-ascii?Q?t61b/NlxoYWLmkSAn56AP8PBCp0fWG8AKUtJ1/GdnAT3JSv0Mnp0H88UbzXg?=
 =?us-ascii?Q?qtzNEwVT0RQubywiB43k9fH6uGV3pJr1MbeHwXSr2bcb7iTTmA3tD97s44gk?=
 =?us-ascii?Q?7AHCJDTIY8asOe/TUbir5cYl4P9JKuDaY0ozJqM+UYihv0VaILeQmY91ZveZ?=
 =?us-ascii?Q?D3MoUtrR34ZcBq0SUg2hU2oTxsy39xDsUszMMQXMFdal8RFipxXIKANFld/7?=
 =?us-ascii?Q?GemYlNJ7wtk1iT3eXWY99NfolW3Mx9xdzXCrRYe4owa/wB9VHEpK1gmLRxoc?=
 =?us-ascii?Q?sAT0c41bwKriD34W+MJ23Mj3L59kiqVAosOrnDQLm6er9Av9b/p/Dt/Dll9y?=
 =?us-ascii?Q?TBL8BqXfFcsHrHJwF6kBcP5fIt/0uXnBJnzX8LHFYgJjXye3Puk5yQdwA4a3?=
 =?us-ascii?Q?RiTnp1E5t8xb0/zD54uk4KtGZ5tMQBa1EbbkA/V+I6rSo9bUSarC25oj9Unh?=
 =?us-ascii?Q?NCcylzKgSkIOQ8fMsPWl8Xogbw1iFAJlRbmHAB1rCH+p2Xz8OnwHSep7tbRf?=
 =?us-ascii?Q?F4pGau8tSJRJQEqO/hihL9ncOZ5jmSWnu/LauiRK7JE+Pj27KNw3BbiHLn41?=
 =?us-ascii?Q?FJ5WsWtJDKsrS+nXOGW8+G4WdgFcwczanq6WYNETzLB5fvAUWTwxjf8kg3wA?=
 =?us-ascii?Q?wv1St1zMza0eVAUAzcVGB+geEgY0/H1x0QbVRSmd8v1EBH+1TstTWnjfD8oR?=
 =?us-ascii?Q?AfK2pT/rCv4xIXvRAwjwhwtMBTFGSrx4QYL7blfvq1TkfddIIS6Mnd2IodaZ?=
 =?us-ascii?Q?J2mis03J9l0nKwS6eArzrMhkFHjCTo30fuCM/Vr/bFllNCuVP+mtRFxmHR+M?=
 =?us-ascii?Q?gzObd6tjs2xjhG9E99X88r/SIU0qXeBrBldqo1Wvee2f+UOcZSqYKGFxJatk?=
 =?us-ascii?Q?vXPQWL9lrC6Uvkg8PMM=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b250b6de-3d42-477c-860c-08dc3f78b3cd
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3849.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Mar 2024 14:04:42.6668
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WOUZkjP9XEn2JDjg1WKufaZokUJ0lVjV3UWKwURs/ithiAhUuVcKLBtqmGixWZcu
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN6PR12MB8566

On Thu, Jan 11, 2024 at 06:06:45PM -0600, Suravee Suthikulpanit wrote:
> @@ -2367,8 +2372,9 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
>  	domain->nid = NUMA_NO_NODE;
>  
>  	switch (type) {
> -	/* No need to allocate io pgtable ops in passthrough mode */
> +	/* No need to allocate io pgtable ops in passthrough and nested mode */
>  	case IOMMU_DOMAIN_IDENTITY:
> +	case IOMMU_DOMAIN_NESTED:

These constants should not show up in the driver, it needs to be
reorganized to use the new allocation APIs to avoid this.

> -static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
> +static const struct iommu_domain_ops nested_domain_ops = {
> +	.attach_dev = amd_iommu_attach_device,
> +	.free = amd_iommu_domain_free,
> +};

I would expect nesting to have its own attach function too, because it
should be quite different.

>  static struct iommu_domain *
>  amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
>  			    struct iommu_domain *parent,
>  			    const struct iommu_user_data *user_data)
> -
>  {
> +	struct iommu_domain *dom;
> +	struct iommu_dev_data *dev_data;
>  	unsigned int type = IOMMU_DOMAIN_UNMANAGED;
> +	bool nested_parent = flags & IOMMU_HWPT_ALLOC_NEST_PARENT;
> +
> +	if (parent) {
> +		int ret;
> +		struct iommu_hwpt_amd_v2 hwpt;
> +
> +		if (parent->ops != amd_iommu_ops.default_domain_ops)
> +			return ERR_PTR(-EINVAL);
> +
> +		ret = udata_to_iommu_hwpt_amd_v2(user_data, &hwpt);
> +		if (ret)
> +			return ERR_PTR(ret);
>  
> -	if ((flags & ~IOMMU_HWPT_ALLOC_DIRTY_TRACKING) || parent || user_data)
> +		return amd_iommu_nested_domain_alloc(dev, type, flags,
> +						     &hwpt, parent);
> +	}
> +
> +	/* Check supported flags */
> +	if ((flags & ~amd_iommu_hwpt_supported_flags) ||
> +	    !check_nested_support(flags))
>  		return ERR_PTR(-EOPNOTSUPP);
>  
> -	return do_iommu_domain_alloc(type, dev, flags);
> +	dev_data = dev_iommu_priv_get(dev);
> +
> +	/*
> +	 * When allocated nested parent domain, the device may already
> +	 * have been attached to a domain. For example, a device is already
> +	 * attached to the domain allocated by VFIO, which contains GPA->SPA mapping.
> +	 * In such case, return reference to the same domain.
> +	 */

What? No! This would break all the lifecycle model. Domain allocation must
always allocate. qemu needs to allocate the nested partent domain type
from the very start.

> +static int nested_gcr3_update(struct iommu_hwpt_amd_v2 *hwpt,
> +			      struct protection_domain *pdom,
> +			      struct protection_domain *ppdom,
> +			      struct device *dev)
> +{
> +	struct pci_dev *pdev;
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +
> +	pdev = to_pci_dev(dev);
> +	if (!pdev)
> +		return -EINVAL;
> +
> +	/* Note: Currently only support GCR3TRPMode with nested translation */
> +	if (!check_feature2(FEATURE_GCR3TRPMODE))
> +		return -EOPNOTSUPP;
> +
> +	pdom->parent = ppdom;
> +	pdom->guest_domain_id = hwpt->gdom_id;
> +	pdom->guest_paging_mode = hwpt->flags.guest_paging_mode;
> +
> +	dev_data->gcr3_info.trp_gpa = hwpt->gcr3;
> +	dev_data->gcr3_info.glx = hwpt->flags.glx;
> +	dev_data->gcr3_info.giov = hwpt->flags.giov;

Here again, this is just data copied from the vDTE to the pDTE - the
guest's gcr3 related parameters are just bits being flowed through

And as before "alloc" shouldn't touch anything outside the
iommu_domain being allocated, touching dev_data here is completely
wrong.

Jason