Hi Lu,
On Sat, 20 Feb 2021 09:56:26 +0800, Lu Baolu <baolu...@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.ku...@intel.com>
> > Signed-off-by: Jacob Pan <jacob.jun....@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
