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

[Lu Baolu]

Hi,

On 10/24/19 1:55 AM, Jacob Pan wrote:
> On Wed, 23 Oct 2019 09:53:04 +0800
> Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxx>
> > > > Cc: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> > > > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> > > > Signed-off-by: Liu Yi L <yi.l.liu@xxxxxxxxx>
> > > > Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> > > > Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
> > > > ---
> > > >    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