RE: [PATCH v3 4/6] iommu/vt-d: Setup pasid entries for iova over first level

[Liu, Yi L]

Hi Baolu,

> From: Lu Baolu [mailto:baolu.lu@xxxxxxxxxxxxxxx]
> Sent: Saturday, December 14, 2019 11:04 AM
> To: Liu, Yi L <yi.l.liu@xxxxxxxxx>; Joerg Roedel <joro@xxxxxxxxxx>; David
> Woodhouse <dwmw2@xxxxxxxxxxxxx>; Alex Williamson
> <alex.williamson@xxxxxxxxxx>
> Subject: Re: [PATCH v3 4/6] iommu/vt-d: Setup pasid entries for iova over first level
> 
> Hi Liu Yi,
> 
> Thanks for reviewing my patch.
> 
> On 12/13/19 5:23 PM, Liu, Yi L wrote:
> >> From: kvm-owner@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx] On
> >> Behalf Of Lu Baolu
> >> Sent: Wednesday, December 11, 2019 10:12 AM
> >> Subject: [PATCH v3 4/6] iommu/vt-d: Setup pasid entries for iova over
> >> first level
> >>
> >> Intel VT-d in scalable mode supports two types of page tables for IOVA
> translation:
> >> first level and second level. The IOMMU driver can choose one from
> >> both for IOVA translation according to the use case. This sets up the
> >> pasid entry if a domain is selected to use the first-level page table for iova
> translation.
> >>
> >> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> >> ---
> >>   drivers/iommu/intel-iommu.c | 48 +++++++++++++++++++++++++++++++++++--
> >>   include/linux/intel-iommu.h | 10 ++++----
> >>   2 files changed, 52 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/iommu/intel-iommu.c
> >> b/drivers/iommu/intel-iommu.c index
> >> 2b5a47584baf..83a7abf0c4f0 100644
> >> --- a/drivers/iommu/intel-iommu.c
> >> +++ b/drivers/iommu/intel-iommu.c
> >> @@ -571,6 +571,11 @@ static inline int domain_type_is_si(struct
> >> dmar_domain
> >> *domain)
> >>   	return domain->flags & DOMAIN_FLAG_STATIC_IDENTITY;  }
> >>
> >> +static inline bool domain_use_first_level(struct dmar_domain *domain) {
> >> +	return domain->flags & DOMAIN_FLAG_USE_FIRST_LEVEL; }
> >> +
> >>   static inline int domain_pfn_supported(struct dmar_domain *domain,
> >>   				       unsigned long pfn)
> >>   {
> >> @@ -2288,6 +2293,8 @@ static int __domain_mapping(struct dmar_domain
> >> *domain, unsigned long iov_pfn,
> >>   		return -EINVAL;
> >>
> >>   	prot &= DMA_PTE_READ | DMA_PTE_WRITE | DMA_PTE_SNP;
> >> +	if (domain_use_first_level(domain))
> >> +		prot |= DMA_FL_PTE_PRESENT;
> >
> > For DMA_PTE_SNP bit, I think there needs some work. The bit 11 of prot
> > should be cleared when FLPT is used for IOVA.
> 
> SNP (bit 11) is only for second level. This bit is ignored for first level page table walk.
> We should clear this bit for first level anyway.

I think this is what I meant above? This patch somehow misses the operation
to clear the bit 11.

> >
> > Also, we need to set bit 63 "XD" properly. e.g. If bit 11 of prot is
> > set, it means snoop required, then "XD" bit is "0". If bit 11 of prot
> > is "0", it means this domain is not snooping, so you may want to set
> > "XD" bit as "1". With such enhancement, I think IOVA over FLPT would
> > have as less difference with IOVA over SLPT.
> 
> XD (bit 63) is only for the first level, and SNP (bit 11) is only for second level, right? I
> think we need to always set XD bit for IOVA over FL case. thoughts?

Oops, I made a mistake here. Please forget SNP bit, there is no way to control SNP
with first level page table.:-)

Actually, it is execute (bit 1) of second level page table which I wanted to say.
If software sets R/W/X permission to an IOVA, with IOVA over second level
page table, it will set bit 1. However, if IOVA is over first level page table, it
may need to clear XD bit. This is what I want to say here. If IOVA doesnât allow
execute permission, it's ok to always set XD bit for IOVA over FL case. But I
would like to do it just as what we did for R/W permission. R/W permission
relies on the permission configured by the page map caller. right?

Regards,
Yi Liu