> I would expect real applications will try to use the same PASID for
> the same IOVA map to optimize IOTLB caching.
On Intel a ring3 application only gets to use one PASID.
The ENQCMD instruction pick up the PASID for the process
from the IA32_PASID MSR (set by OS when first access,
and on
On Thu, Feb 10, 2022 at 08:27:50AM -0800, Fenghua Yu wrote:
> Hi, Jacob,
>
> On Wed, Feb 09, 2022 at 07:16:14PM -0800, Jacob Pan wrote:
> > Hi Fenghua,
> >
> > On Mon, 7 Feb 2022 15:02:48 -0800, Fenghua Yu wrote:
> >
> > > @@ -1047,8 +1040,6 @@ struct iommu_sva *intel_svm_bind(struct device
>
On Wed, Sep 29, 2021 at 06:07:22PM +, Fenghua Yu wrote:
> Add
> +#ifdef CONFIG_IOMMU_SUPPORT
> because mm->pasid and current-pasid_activated are defined only if
> CONFIG_IOMMU_SUPPORT is defined.
>
> > + if (user_mode(regs) && current->mm->pasid && !current->pasid_activated)
> > {
>
>
On Wed, Sep 29, 2021 at 07:15:53PM +0200, Thomas Gleixner wrote:
> On Wed, Sep 29 2021 at 09:59, Andy Lutomirski wrote:
> > On 9/29/21 05:28, Thomas Gleixner wrote:
> >> Looking at that patch again, none of this muck in fpu__pasid_write() is
> >> required at all. The whole exception fixup is:
> >>
On Wed, Sep 29, 2021 at 09:58:22AM -0700, Andy Lutomirski wrote:
> On 9/28/21 16:10, Luck, Tony wrote:
> > Moving beyond pseudo-code and into compiles-but-probably-broken-code.
> >
> >
> > The intent of the functions below is that Fenghua should be able to
> > d
> There is zero requirement to look at TIF_NEED_FPU_LOAD or
> fpregs_state_valid() simply because the #GP comes straight from user
> space which means the FPU registers contain the current tasks user space
> state.
Just to double confirm ... there is no point in the #GP handler up to this point
>> if (!(xsave->header.xfeatures & fmask)) {
>> xsave->header.xfeatures |= fmask; //<
>> xsaves(xsave, fmask);
>> }
>
> I'm not sure why the FPU state is initialized here.
>
> For updating the PASID state, it's unnecessary to init the PASID state.
>
>
> But the helpers seem to be generic. They take "task" as a parameter and
> handle the task as non-current case. So the helpers are not for PASID only.
> They may be used by others to modify a running task's FPU state. But
> It's not safe to do so.
>
> At least need some comments/restriction for
>> fpregs_lock();
>
> I'm afraid we may hit the same locking issue when we send IPI to notify
> another task to modify its
> PASID state. Here the API is called to modify another running task's PASID
> state as well without a right lock.
> fpregs_lock() is not enough to deal with this, I'm
On Tue, Sep 28, 2021 at 11:50:37PM +, Fenghua Yu wrote:
> If xfeatures's feature bit is 0, xsaves will not write its init value to the
> memory due to init optimization. So the xsaves will do nothing and the
> state is not initialized and may have random data.
> Setting TIF_NEED_FPU_LOAD
Moving beyond pseudo-code and into compiles-but-probably-broken-code.
The intent of the functions below is that Fenghua should be able to
do:
void fpu__pasid_write(u32 pasid)
{
u64 msr_val = pasid | MSR_IA32_PASID_VALID;
struct ia32_pasid_state *addr;
addr =
On Tue, Sep 28, 2021 at 12:19:22PM -0700, Dave Hansen wrote:
> On 9/28/21 11:50 AM, Luck, Tony wrote:
> > On Mon, Sep 27, 2021 at 04:51:25PM -0700, Dave Hansen wrote:
> ...
> >> 1. Hide whether we need to write to real registers
> >> 2. Hide whether we need to upd
On Mon, Sep 27, 2021 at 04:51:25PM -0700, Dave Hansen wrote:
> That's close to what we want.
>
> The size should probably be implicit. If it isn't implicit, it needs to
> at least be double-checked against the state sizes.
>
> Not to get too fancy, but I think we also want to have a "replace"
>
On Thu, Sep 23, 2021 at 04:17:05PM -0700, Andy Lutomirski wrote:
> > + fpregs_lock();
> > +
> > + /*
> > +* If the task's FPU doesn't need to be loaded or is valid, directly
> > +* write the IA32_PASID MSR. Otherwise, write the PASID state and
> > +* the MSR will be loaded from the
On Fri, Sep 24, 2021 at 04:03:53PM -0700, Andy Lutomirski wrote:
> 1. The context switch code needs to resync PASID. Unfortunately, this adds
> some overhead to every context switch, although a static_branch could
> minimize it for non-PASID users.
Any solution that adds to context switch time
On Fri, Sep 24, 2021 at 03:18:12PM +0200, Thomas Gleixner wrote:
> On Thu, Sep 23 2021 at 19:48, Thomas Gleixner wrote:
> > On Thu, Sep 23 2021 at 09:40, Tony Luck wrote:
> >
> > fpu_write_task_pasid() can just grab the pasid from current->mm->pasid
> > and be done with it.
> >
> > The task exit
On Fri, Sep 24, 2021 at 03:37:22PM +0200, Peter Zijlstra wrote:
> On Thu, Sep 23, 2021 at 10:14:42AM -0700, Luck, Tony wrote:
> > On Wed, Sep 22, 2021 at 11:07:22PM +0200, Peter Zijlstra wrote:
> > > On Mon, Sep 20, 2021 at 07:23:45PM +, Fenghua Yu wrote:
> &
On Thu, Sep 23, 2021 at 04:09:18PM -0700, Andy Lutomirski wrote:
> On Mon, Sep 20, 2021, at 12:23 PM, Fenghua Yu wrote:
> I think this is unnecessarily complicated because it's buying in to the
> existing ISA misconception that PASID has anything to do with a task.
> A PASID belongs to an mm,
On Wed, Sep 22, 2021 at 11:07:22PM +0200, Peter Zijlstra wrote:
> On Mon, Sep 20, 2021 at 07:23:45PM +, Fenghua Yu wrote:
> > @@ -538,6 +547,9 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
> >
> > cond_local_irq_enable(regs);
> >
> > + if (user_mode(regs) &&
On Thu, Sep 23, 2021 at 04:36:50PM +0200, Thomas Gleixner wrote:
> On Mon, Sep 20 2021 at 19:23, Fenghua Yu wrote:
> >
> > +#ifdef CONFIG_INTEL_IOMMU_SVM
> > +void pasid_put(struct task_struct *tsk, struct mm_struct *mm);
> > +#else
> > +static inline void pasid_put(struct task_struct *tsk,
>> > +static bool fixup_pasid_exception(void)
>> > +{
>> > + if (!cpu_feature_enabled(X86_FEATURE_ENQCMD))
>> > + return false;
>> > +
>> > + return __fixup_pasid_exception();
>> > +}
>
> That is, shouldn't the above at the very least decode the instruction
> causing the #GP and check
On Wed, Jun 09, 2021 at 04:34:31PM -0700, Andy Lutomirski wrote:
> On 6/9/21 10:32 AM, Luck, Tony wrote:
> >>> I've told Fenghua to dig out the previous iteration of this patch where
> >>> the plan was to lazily fix the PASID_MSR in other threads in the #GP
> >&
>> I've told Fenghua to dig out the previous iteration of this patch where
>> the plan was to lazily fix the PASID_MSR in other threads in the #GP
>> handler.
>
> Blech. Also this won't work for other PASID-like features.
>
> I have a half-written patch to fix this up for real. Stay tuned.
>> ... so on a PASID system, your trivial reproducer would theoretically
>> fire the same way and corrupt FPU state just as well.
>
> This is worse and you can't selftest it because the IPI can just hit in
> the middle of _any_ FPU state operation and corrupt state.
That sounds like we should
On Thu, May 13, 2021 at 12:46:21PM -0700, Jacob Pan wrote:
> It seems there are two options:
> 1. Add a new IOMMU API to set up a system PASID with a *separate* IOMMU page
> table/domain, mark the device is PASID only with a flag. Use DMA APIs
> to explicit map/unmap. Based on this PASID-only
> If you want this then be explicit about what it is you are making when
> building the API. Don't try to hide it under some generic idea of
> "kernel PCI DMA SVA"
So, a special API call (that Dave can call from IDXD) to set up this
kernel PASID. With suitable documentation to explain the scope.
On Thu, May 13, 2021 at 02:33:03PM -0300, Jason Gunthorpe wrote:
> The page table under the kernel PASID should behave the same way that
> the kernel would operate the page table assigned to a kernel RID.
>
> If the kernel has security off then the PASID should map to all
> physical memory, just
> For shared workqueue, it can only generate DMA request with PASID. The
> submission is done by ENQCMDS (S for supervisor) instruction.
>
> If we were not to share page tables with init_mm, we need a system PASID
> that doing the same direct mapping in IOMMU page tables.
Note that for the
On Fri, Jun 26, 2020 at 11:44:50AM +0200, Peter Zijlstra wrote:
> On Thu, Jun 25, 2020 at 01:17:22PM -0700, Fenghua Yu wrote:
>
> > +static bool fixup_pasid_exception(void)
> > +{
> > + if (!IS_ENABLED(CONFIG_INTEL_IOMMU_SVM))
> > + return false;
> > + if
> Is PASID in the uapi at all?
$ grep pasid include/uapi/linux/iommu.h
* @pasid: Process Address Space ID
__u32 pasid;
* @pasid: Process Address Space ID
__u32 pasid;
* @pasid: Process Address Space ID
__u32 pasid;
* - If the PASID bit is set, the @pasid field is
> So what’s the RDMSR for? Surely you
> have some state somewhere that says “this task has a PASID.”
> Can’t you just make sure that stays in sync with the MSR? Then, on #GP, if
> the task already has a PASID, you know the MSR is set.
We have state that says the process ("mm") has been
> Are we planning to keep PASID live once a task has used it once or are we
> going to swap it lazily? If the latter, a percpu variable might be better.
Current plan is "touch it once and the task owns it until exit(2)"
Maybe someday in the future when we have data on how applications
actually
>> The heuristic always initializes the MSR with the per mm PASID IIF the
>> mm has a valid PASID but the MSR doesn't have one. This heuristic usually
>> happens only once on the first #GP in a thread.
>
> But it doesn't guarantee the PASID is the right one. Suppose both the mm
> has a PASID and
> There are two users of a PASID, mm and device driver(FD). If
> either one is not done with the PASID, it cannot be reclaimed. As you
> mentioned, it could take a long time for the driver to abort. If the
> abort ends *after* mmdrop, we are in trouble.
> If driver drops reference after
>> So the driver needs to use flush/drain operations to make sure all
>> the in-flight work has completed before releasing/re-using the PASID.
>>
> Are you suggesting we should let driver also hold a reference of the
> PASID?
The sequence for bare metal is:
process is queuing requests
> If fd release cleans up then how should there be something in flight at
> the final mmdrop?
ENQCMD from the user is only synchronous in that it lets the user know their
request has been added to a queue (or not). Execution of the request may happen
later (if the device is busy working on
> Just for the record I also suggested to have a proper errorcode in the
> #GP for ENQCMD and I surely did not suggest to avoid decoding the user
> instructions.
Thomas,
Is the heuristic to avoid decoding the user instructions OK (you are just
pointing
out that you should not be given credit
> But that might not be your fault. My ancient system is getting flaky. A v4.19
> build that
> has booted before is also resetting :-(
After a power-cycle (and some time to let the machine cool off). System now
boots
with your patch series plus the __phys_to_pfn() #define
So if you can figure
> This should fix it:
...
> +#include
Not quite. Still have an issue with __phys_to_pfn(paddr)
Trying ti #include gave we a raft of redefined
macros. So I just added
#define __phys_to_pfn(paddr)PHYS_PFN(paddr)
to arch/ia64/mm/init.c
That made the build work. But boot spontaneously
On Fri, Dec 07, 2018 at 11:07:05AM -0800, Christoph Hellwig wrote:
> This works is based on the dma-mapping tree, so you probably want to
> want this git tree for testing:
>
> git://git.infradead.org/users/hch/misc.git dma-direct-calls.2
Pulled this tree. Got HEAD
33b9fc015171
> A couple random cleanups I stumbled upon when doing dma related work.
Christoph,
Thanks. Applied. Will send pull request for next merge window.
-Tony
___
iommu mailing list
iommu@lists.linux-foundation.org
41 matches
Mail list logo
