Re: [PATCH 1/4] iommu/vt-d: Enable write protect for supervisor SVM

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

Hi Lu,

On Sat, 20 Feb 2021 09:56:26 +0800, Lu Baolu <baolu.lu@linux.intel.com>
wrote:

> Hi Jacob and Sanjay,
> 
> On 2/19/21 5:31 AM, Jacob Pan wrote:
> > Write protect bit, when set, inhibits supervisor writes to the read-only
> > pages. In supervisor shared virtual addressing (SVA), where page tables
> > are shared between CPU and DMA, IOMMU PASID entry WPE bit should match
> > CR0.WP bit in the CPU.
> > This patch sets WPE bit for supervisor PASIDs if CR0.WP is set.  
> 
>  From reading the commit message, the intention of this patch is to match
> PASID entry WPE bith with CPU CR0.WP if 1) SRE is set (supervisor
> pasid); 2) page table is shared between CPU and IOMMU. Do I understand
> it right?
> 
yes. that is my intention.

> But what the real code doing is failing pasid entry setup for first
> level translation if CPU CR0.WP is not set. It's not consistent with
> what described above.
> 
> What I am thinking is that, as long as SRE is set, we should always set
> WPE in intel_pasid_setup_first_level(). For supervisor SVA case, we
> should check CPU CR0.WP in intel_svm_bind_mm() and abort binding if
> CR0.WP is not set.
> 
> Thought?
> 
This code only affects supervisor SVA, since PASID_FLAG_SUPERVISOR_MODE
flag is not set for FL IOVA.

> Best regards,
> baolu
> 
> > 
> > Signed-off-by: Sanjay Kumar <sanjay.k.kumar@intel.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> >   drivers/iommu/intel/pasid.c | 26 ++++++++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> > 
> > diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> > index 0cceaabc3ce6..0b7e0e726ade 100644
> > --- a/drivers/iommu/intel/pasid.c
> > +++ b/drivers/iommu/intel/pasid.c
> > @@ -410,6 +410,15 @@ static inline void pasid_set_sre(struct
> > pasid_entry *pe) pasid_set_bits(&pe->val[2], 1 << 0, 1);
> >   }
> >   
> > +/*
> > + * Setup the WPE(Write Protect Enable) field (Bit 132) of a
> > + * scalable mode PASID entry.
> > + */
> > +static inline void pasid_set_wpe(struct pasid_entry *pe)
> > +{
> > +	pasid_set_bits(&pe->val[2], 1 << 4, 1 << 4);
> > +}
> > +
> >   /*
> >    * Setup the P(Present) field (Bit 0) of a scalable mode PASID
> >    * entry.
> > @@ -553,6 +562,20 @@ static void pasid_flush_caches(struct intel_iommu
> > *iommu, }
> >   }
> >   
> > +static inline int pasid_enable_wpe(struct pasid_entry *pte)
> > +{
> > +	unsigned long cr0 = read_cr0();
> > +
> > +	/* CR0.WP is normally set but just to be sure */
> > +	if (unlikely(!(cr0 & X86_CR0_WP))) {
> > +		pr_err_ratelimited("No CPU write protect!\n");
> > +		return -EINVAL;
> > +	}
> > +	pasid_set_wpe(pte);
> > +
> > +	return 0;
> > +};
> > +
> >   /*
> >    * Set up the scalable mode pasid table entry for first only
> >    * translation type.
> > @@ -584,6 +607,9 @@ int intel_pasid_setup_first_level(struct
> > intel_iommu *iommu, return -EINVAL;
> >   		}
> >   		pasid_set_sre(pte);
> > +		if (pasid_enable_wpe(pte))
> > +			return -EINVAL;
> > +
> >   	}
> >   
> >   	if (flags & PASID_FLAG_FL5LP) {
> >   


Thanks,

Jacob