Re: [PATCH v3 10/13] iommu/amd: Refactor helper function for attaching / detaching device

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Oct 13, 2023 at 03:16:49PM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> To use the new helper function for setting up GCR3 table.
> 
> If system is booted with V2 page table then setup default GCR3 with
> domain GCR3 pointer. 

Lets stop talking about "booted with V2 page table" - "If the domain
uses the V2 format then setup a GCR3 table in the device to point to it"

> +static int _init_gcr3_tbl(struct iommu_dev_data *dev_data)
> +{
> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> +
> +	/* By default, GCR3 is set to support non-PASID devices. */
> +	gcr3_info->giov = true;

I admit I find it really hard to read the AMD spec here.. In terms of
the SW model what modes are actually supportable by HW?

No PASIDs being used:
 RID domain=IDENTITY - yes [V=0]
 RID domain=BLOCKED - yes [V=1, TV=1, GV=0, mode == 0]
 RID domain=v1 - Yes [V=1, TV=1, GV=0, mode != 0]
 RID domain=v2 - Yes [V=1, TV=0, GV=1, GIOV=1]

Some kind of PASID in use:
 RID domain=v2 & PASID - Yes [V=1, TV=0, GV=1, GIOV=1]
 RID domain=V1 & PASID - No??
 RID domain=IDENTITY & PASID - ?? [V=1, TV=1 GV=1, mode=0, GIOV=0] (Section 2.2.7.1?)
 RID domain=BLOCKED & PASID - ?? [V=1, TV=0, GV=1, GIOV=1 with GCR3 entry 0 being non-valid] 

Did I get it right? If so GIOV should ultimately be deduced based on
what domain the RID has?

Look at how the SMMUv3 stuff ended up. Their STE is the same purpose
as the AMD DTE. There are alot of combinations here, it was hard to
make a code flow that was clean. It turned out pretty good when the
DTE was generated in the ops->attach based on a calculation of exactly
what the current configuration is, because we already know what we are
in alot of detail at that point.

eg we know if we are attaching an identity domain and PASIDs are in
use that a single specific DTE should be created. So just call a
function directly to get the required DTE.

IOW - I'm not sure it really makes logical sense to store giov in
gcr3_info.

> +static int do_attach(struct iommu_dev_data *dev_data,
> +		     struct protection_domain *domain)
>  {
>  	struct amd_iommu *iommu;
> +	int ret = 0;
>  
>  	iommu = get_amd_iommu_from_dev(dev_data->dev);
>  	if (!iommu)
> -		return;
> +		return -EINVAL;

iommu can't be null here, have a dev_data.

>  	dev_data->domain = domain;
> @@ -2080,11 +2103,27 @@ static void do_attach(struct iommu_dev_data *dev_data,
>  	if (domain_id_is_per_dev(domain))
>  		dev_data->domid = domain_id_alloc();

At some point this is the wrong place to put this, the domain ID is
logically associated with the gcr3 table, it should never be used if
there is no gcr table allocated, and it should be freed once the gcr3
table is freed.

> +	/* Init GCR3 table */
> +	if (domain->pd_mode == PD_MODE_V2) {

Is there a case where domain_id_is_per_dev() but we are attaching a v1
table?

Jason