iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
From: "Raj, Ashok" <ashok.raj@intel.com>
To: Andy Lutomirski <luto@kernel.org>
Cc: Ravi V Shankar <ravi.v.shankar@intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Dave Hansen <dave.hansen@intel.com>,
	H Peter Anvin <hpa@zytor.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Dave Jiang <dave.jiang@intel.com>,
	Ashok Raj <ashok.raj@intel.com>, x86 <x86@kernel.org>,
	amd-gfx <amd-gfx@lists.freedesktop.org>,
	Christoph Hellwig <hch@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, Fenghua Yu <fenghua.yu@intel.com>,
	Borislav Petkov <bp@alien8.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Tony Luck <tony.luck@intel.com>,
	Felix Kuehling <Felix.Kuehling@amd.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	iommu <iommu@lists.linux-foundation.org>,
	Jacob Jun Pan <jacob.jun.pan@intel.com>,
	David Woodhouse <dwmw2@infradead.org>
Subject: Re: [PATCH v6 12/12] x86/traps: Fix up invalid PASID
Date: Mon, 3 Aug 2020 08:19:21 -0700	[thread overview]
Message-ID: <20200803151921.GA387967@otc-nc-03> (raw)
In-Reply-To: <CALCETrXLFwzCzoE8ZjciBO_WSK8StyTfO1yXVm4v2qFQZpfasg@mail.gmail.com>

On Mon, Aug 03, 2020 at 08:12:18AM -0700, Andy Lutomirski wrote:
> On Mon, Aug 3, 2020 at 8:03 AM Dave Hansen <dave.hansen@intel.com> wrote:
> >
> > On 7/31/20 4:34 PM, Andy Lutomirski wrote:
> > >> Thomas suggested to provide a reason for the #GP caused by executing ENQCMD
> > >> without a valid PASID value programmed. #GP error codes are 16 bits and all
> > >> 16 bits are taken. Refer to SDM Vol 3, Chapter 16.13 for details. The other
> > >> choice was to reflect the error code in an MSR. ENQCMD can also cause #GP
> > >> when loading from the source operand, so its not fully comprehending all
> > >> the reasons. Rather than special case the ENQCMD, in future Intel may
> > >> choose a different fault mechanism for such cases if recovery is needed on
> > >> #GP.
> > > Decoding the user instruction is ugly and sets a bad architecture
> > > precedent, but we already do it in #GP for UMIP.  So I'm unconvinced.
> >
> > I'll try to do one more bit of convincing. :)
> >
> > In the end, we need a way to figure out if the #GP was from a known "OK"
> > source that we can fix up.  You're right that we could fire up the
> > instruction decoder to help answer that question.  But, it (also)
> > doesn't easily yield a perfect answer as to the source of the #GP, it
> > always involves a user copy, and it's a larger code impact than what
> > we've got.
> >
> > I think I went and looked at fixup_umip_exception(), and compared it to
> > the alternative which is essentially just these three lines of code:
> >
> > > +     /*
> > > +      * If the current task already has a valid PASID in the MSR,
> > > +      * the #GP must be for some other reason.
> > > +      */
> > > +     if (current->has_valid_pasid)
> > > +             return false;
> > ...> +  /* Now the current task has a valid PASID in the MSR. */
> > > +     current->has_valid_pasid = 1;
> >
> > and *I* was convinced that instruction decoding wasn't worth it.
> >
> > There's a lot of stuff that fixup_umip_exception() does which we don't
> > have to duplicate, but it's going to be really hard to get it anywhere
> > near as compact as what we've got.
> >
> 
> I could easily be convinced that the PASID fixup is so trivial and so
> obviously free of misfiring in a way that causes an infinite loop that
> this code is fine.  But I think we first need to answer the bigger
> question of why we're doing a lazy fixup in the first place.

We choose lazy fixup for 2 reasons. 

- If some threads were already created before the MSR is programmed, then
  we need to fixup those in a race free way. scheduling some task-work etc.
  We did do that early on, but decided it was ugly. 
- Not all threads need to submit ENQCMD, force feeding the MSR probably
  isn't even required for all. Yes the overhead isn't probably big, but
  might not even be required for all threads.

We needed to fixup MSR in two different way. To keep the code simple, the
choice was to only fixup on #GP, that eliminated the extra code we need to
support case1.

Cheers,
Ashok
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  reply	other threads:[~2020-08-03 15:19 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-13 23:47 [PATCH v6 00/12] x86: tag application address space for devices Fenghua Yu
2020-07-13 23:47 ` [PATCH v6 01/12] iommu: Change type of pasid to u32 Fenghua Yu
2020-07-14  2:45   ` Liu, Yi L
2020-07-14 13:54     ` Fenghua Yu
2020-07-14 13:56       ` Liu, Yi L
2020-07-22 14:03   ` Joerg Roedel
2020-07-22 17:21     ` Fenghua Yu
2020-07-13 23:47 ` [PATCH v6 02/12] iommu/vt-d: Change flags type to unsigned int in binding mm Fenghua Yu
2020-07-13 23:47 ` [PATCH v6 03/12] docs: x86: Add documentation for SVA (Shared Virtual Addressing) Fenghua Yu
2020-07-14  3:25   ` Liu, Yi L
2020-07-15 23:32     ` Fenghua Yu
2020-07-13 23:47 ` [PATCH v6 04/12] x86/cpufeatures: Enumerate ENQCMD and ENQCMDS instructions Fenghua Yu
2020-07-13 23:48 ` [PATCH v6 05/12] x86/fpu/xstate: Add supervisor PASID state for ENQCMD feature Fenghua Yu
2020-07-13 23:48 ` [PATCH v6 06/12] x86/msr-index: Define IA32_PASID MSR Fenghua Yu
2020-07-13 23:48 ` [PATCH v6 07/12] mm: Define pasid in mm Fenghua Yu
2020-07-13 23:48 ` [PATCH v6 08/12] fork: Clear PASID for new mm Fenghua Yu
2021-02-24 10:19   ` Jean-Philippe Brucker
2021-02-25 22:17     ` Fenghua Yu
2021-03-01 23:00       ` Jacob Pan
2021-03-02 10:43         ` Jean-Philippe Brucker
2020-07-13 23:48 ` [PATCH v6 09/12] x86/process: Clear PASID state for a newly forked/cloned thread Fenghua Yu
2020-08-01  1:44   ` Andy Lutomirski
2020-07-13 23:48 ` [PATCH v6 10/12] x86/mmu: Allocate/free PASID Fenghua Yu
2020-07-13 23:48 ` [PATCH v6 11/12] sched: Define and initialize a flag to identify valid PASID in the task Fenghua Yu
2020-07-13 23:48 ` [PATCH v6 12/12] x86/traps: Fix up invalid PASID Fenghua Yu
2020-07-31 23:34   ` Andy Lutomirski
2020-08-01  0:42     ` Fenghua Yu
2020-08-03 15:03     ` Dave Hansen
2020-08-03 15:12       ` Andy Lutomirski
2020-08-03 15:19         ` Raj, Ashok [this message]
2020-08-03 16:36         ` Dave Hansen
2020-08-03 17:16           ` Andy Lutomirski
2020-08-03 17:34             ` Dave Hansen
2020-08-03 19:24               ` Andy Lutomirski
2020-08-01  1:28   ` Andy Lutomirski
2020-08-03 17:19     ` Fenghua Yu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200803151921.GA387967@otc-nc-03 \
    --to=ashok.raj@intel.com \
    --cc=Felix.Kuehling@amd.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=bp@alien8.de \
    --cc=dave.hansen@intel.com \
    --cc=dave.jiang@intel.com \
    --cc=dwmw2@infradead.org \
    --cc=fenghua.yu@intel.com \
    --cc=hch@infradead.org \
    --cc=hpa@zytor.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jacob.jun.pan@intel.com \
    --cc=jean-philippe@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=ravi.v.shankar@intel.com \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH v6 12/12] x86/traps: Fix up invalid PASID' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).