Date:   Mon, 20 Mar 2023 09:00:06 -0700
From:   Jacob Pan <jacob.jun.pan@linux.intel.com>
To:     Lu Baolu <baolu.lu@linux.intel.com>
Cc:     iommu@lists.linux.dev, dmaengine@vger.kernel.org,
        Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        Kevin Tian <kevin.tian@intel.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Dave Jiang <dave.jiang@intel.com>,
        Vinod Koul <vkoul@kernel.org>, linux-kernel@vger.kernel.org,
        jacob.jun.pan@linux.intel.com
Subject: Re: [PATCH v2 2/5] iommu/vt-d: Allow SVA with device-specific IOPF
Message-ID: <20230320090006.180efbed@jacob-builder>
In-Reply-To: <20230309025639.26109-3-baolu.lu@linux.intel.com>
References: <20230309025639.26109-1-baolu.lu@linux.intel.com>
        <20230309025639.26109-3-baolu.lu@linux.intel.com>
Organization: OTC
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi BaoLu,

On Thu,  9 Mar 2023 10:56:36 +0800, Lu Baolu <baolu.lu@linux.intel.com>
wrote:

> Currently enabling SVA requires IOPF support from the IOMMU and device
> PCI PRI. However, some devices can handle IOPF by itself without ever
> sending PCI page requests nor advertising PRI capability.
> 
> Allow SVA support with IOPF handled either by IOMMU (PCI PRI) or device
> driver (device-specific IOPF). As long as IOPF could be handled, SVA
> should continue to work.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 7c2f4bd33582..d2fcab9d8f61 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4650,7 +4650,18 @@ static int intel_iommu_enable_sva(struct device
> *dev) if (!(iommu->flags & VTD_FLAG_SVM_CAPABLE))
>  		return -ENODEV;
>  
> -	if (!info->pasid_enabled || !info->pri_enabled ||
> !info->ats_enabled)
> +	if (!info->pasid_enabled)
> +		return -EINVAL;
> +
> +	/*
> +	 * Devices having device-specific I/O fault handling should not
> +	 * support PCI/PRI.
> +	 */
> +	if (!info->pri_supported)
> +		return 0;
If you put this check at the very beginning, everything else should it be
the same, right?

Still feel a little weird that, SVA is tied to PRI for PCI PRI but not for
device specific IOPF.

> +	/* Devices supporting ATS/PRI should have it enabled. */
> +	if (!info->pri_enabled || !info->ats_enabled)
>  		return -EINVAL;
>  
>  	ret = iopf_queue_add_device(iommu->iopf_queue, dev);


Thanks,

Jacob