Re: [PATCH v6 01/10] iommu/vt-d: Enlightened PASID allocation

[Lu Baolu <baolu.lu@linux.intel.com>]

Hi,

On 10/24/19 1:55 AM, Jacob Pan wrote:
> On Wed, 23 Oct 2019 09:53:04 +0800
> Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
>> Hi Ashok,
>>
>> Thanks for reviewing the patch.
>>
>> On 10/23/19 8:45 AM, Raj, Ashok wrote:
>>> On Tue, Oct 22, 2019 at 04:53:14PM -0700, Jacob Pan wrote:
>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>>
>>>> If Intel IOMMU runs in caching mode, a.k.a. virtual IOMMU, the
>>>> IOMMU driver should rely on the emulation software to allocate
>>>> and free PASID IDs. The Intel vt-d spec revision 3.0 defines a
>>>> register set to support this. This includes a capability register,
>>>> a virtual command register and a virtual response register. Refer
>>>> to section 10.4.42, 10.4.43, 10.4.44 for more information.
>>>
>>> The above paragraph seems a bit confusing, there is no reference
>>> to caching mode for for VCMD... some suggestion below.
>>>
>>> Enabling IOMMU in a guest requires communication with the host
>>> driver for certain aspects. Use of PASID ID to enable Shared Virtual
>>> Addressing (SVA) requires managing PASID's in the host. VT-d 3.0
>>> spec provides a Virtual Command Register (VCMD) to facilitate this.
>>> Writes to this register in the guest are trapped by Qemu and
>>> proxies the call to the host driver....
>>
>> Yours is better. Will use it.
>>
> I will roll that in to v7
>>>
>>>    
>>>>
>>>> This patch adds the enlightened PASID allocation/free interfaces
>>>> via the virtual command register.
>>>>
>>>> Cc: Ashok Raj <ashok.raj@intel.com>
>>>> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
>>>> Cc: Kevin Tian <kevin.tian@intel.com>
>>>> Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
>>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
>>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>    drivers/iommu/intel-pasid.c | 83
>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>> drivers/iommu/intel-pasid.h | 13 ++++++-
>>>> include/linux/intel-iommu.h |  2 ++ 3 files changed, 97
>>>> insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/iommu/intel-pasid.c
>>>> b/drivers/iommu/intel-pasid.c index 040a445be300..76bcbb21e112
>>>> 100644 --- a/drivers/iommu/intel-pasid.c
>>>> +++ b/drivers/iommu/intel-pasid.c
>>>> @@ -63,6 +63,89 @@ void *intel_pasid_lookup_id(int pasid)
>>>>    	return p;
>>>>    }
>>>>    
>>>> +static int check_vcmd_pasid(struct intel_iommu *iommu)
>>>> +{
>>>> +	u64 cap;
>>>> +
>>>> +	if (!ecap_vcs(iommu->ecap)) {
>>>> +		pr_warn("IOMMU: %s: Hardware doesn't support
>>>> virtual command\n",
>>>> +			iommu->name);
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	cap = dmar_readq(iommu->reg + DMAR_VCCAP_REG);
>>>> +	if (!(cap & DMA_VCS_PAS)) {
>>>> +		pr_warn("IOMMU: %s: Emulation software doesn't
>>>> support PASID allocation\n",
>>>> +			iommu->name);
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +int vcmd_alloc_pasid(struct intel_iommu *iommu, unsigned int
>>>> *pasid) +{
>>>> +	u64 res;
>>>> +	u8 status_code;
>>>> +	unsigned long flags;
>>>> +	int ret = 0;
>>>> +
>>>> +	ret = check_vcmd_pasid(iommu);
>>>
>>> Do you have to check this everytime? every dmar_readq is going to
>>> trap to the other side ...
>>
>> Yes. We don't need to check it every time. Check once and save the
>> result during boot is enough.
>>
>> How about below incremental change?
>>
> Below is good but I was thinking to include vccap in struct
> intel_iommu{} where cap and ecaps reside. i.e.
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 14b87ae2916a..e2cf25c9c956 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -528,6 +528,7 @@ struct intel_iommu {
>          u64             reg_size; /* size of hw register set */
>          u64             cap;
>          u64             ecap;
> +       u64             vccap;
> 
> Also, we can use a static branch here.

Yeah! Good idea.

Best regards,
baolu