Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20171128190505.pqip2v2ewb3isjhd@hirez.programming.kicks-ass.net>
References: <20171127104923.14378-1-mingo@kernel.org> <20171127104923.14378-16-mingo@kernel.org>
 <CALCETrUFfpDLEP78K9V4GsbHWS5=M6k8ndv0p+R0ud0=xxbaMg@mail.gmail.com>
 <20171128163908.e3gj6zgq6kcbdlxe@hirez.programming.kicks-ass.net>
 <1c9a32ed-e754-9d91-98f7-b72c07b2c0dc@linux.intel.com> <20171128190505.pqip2v2ewb3isjhd@hirez.programming.kicks-ass.net>
From:   Andy Lutomirski <luto@amacapital.net>
Date:   Tue, 28 Nov 2017 12:34:17 -0800
Message-ID: <CALCETrW3hgNJoP9sMWXBmYiRKten1CJNY6AgCxv1wQdcBfd8pQ@mail.gmail.com>
Subject: Re: [PATCH 15/24] x86/mm: Allow flushing for future ASID switches
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Dave Hansen <dave.hansen@linux.intel.com>,
        Ingo Molnar <mingo@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        "H . Peter Anvin" <hpa@zytor.com>, Borislav Petkov <bp@alien8.de>,
        Linus Torvalds <torvalds@linux-foundation.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 28, 2017 at 11:05 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Nov 28, 2017 at 10:13:30AM -0800, Dave Hansen wrote:
>> Thanks for looking at this, Peter.  I've been resisting doing this for a
>> bit and it's an embarrassingly small amount of code.
>
> Right, well, its not complete yet, and it might be complete crap :-)
>
>> On 11/28/2017 08:39 AM, Peter Zijlstra wrote:
>> > @@ -220,7 +221,21 @@ For 32-bit we have the following conventions - kernel is built with
>> >  .macro SWITCH_TO_USER_CR3 scratch_reg:req
>> >     STATIC_JUMP_IF_FALSE .Lend_\@, kaiser_enabled_key, def=1
>> >     mov     %cr3, \scratch_reg
>> > -   ADJUST_USER_CR3 \scratch_reg
>> > +   push    \scratch_reg
>>
>> Do we have a good stack in all the spots that we need to do this?  It
>> may have changed with the trampoline stack, but I'm 100% sure that it
>> wasn't so in the recent past.
>
> Dunno really. I figured I'd give it a go and see what happens. So far
> the machine still works. But I was hoping Andy would have an opinion on
> this.

I thought we had a stack in all these places even before the
trampoline.  There was an issue with *entry*, but I think exit has
always been okay.

>
>> Let me see if I'm reading the assembly right.
>
> Yep, seems you can read asm :-)
>
>
>> > +DECLARE_PER_CPU(unsigned long, __asid_flush);
>>
>> Could we spare enough space to make this something like
>> user_asid_flush_pending_mask?
>
> Yeah, if I can get it all working we'll bikeshed on a name ;-)
>
>> It took me a minute to realize that it was a mask.  Also, since we only
>> have 6 asids, should we bit a bit more stingy with the type?
>
> I picked unsigned long because our bitops (__set_bit in this case, use
> it), and I know we're LE and could simply use a shorter type, but meh.
>
>> It took me a minute to realize that mixing these is still OK, even if
>> the mm associated with the ASID changes.  It's because once the ASID is
>> stale, it doesn't matter *why* it is stale.  Just that the next guy who
>> *uses* it needs to do the flush.  You can do 1,000 tlb flushes, a
>> context switch, a tlb flush and another context switch, but if you only
>> go out to userspace once, you only need 1 ASID flush.  That fits
>> perfectly with this bit that gets set a bunch of times and only cleared
>> once at exit to userspace.
>
> Just so.
>
> I'm now staring at the RESTORE_CR3 stuff, and that appears to be called
> in the NMI handling where the stack is not to be used (if I read it
> right), so that's going to be a little more tricky.

I think it should be fine.  A very old version of the patches had that
problem, but, in -tip, the nmi RESTORE_CR3 is in the fancy
recursion-protected region, and the stack is okay.  The idea is that
we're already on the old (possibly user) CR3 before we do the crazy
recursion-checking bits.  But that's fine, since all that's accessed
there is the IST stack, and that's in the cpu_entry_area and thus safe
regardless of CR3.

Side question: on extremely quick read, you're doing bt then btr.  Why
not just do a single btr and be done with it?  Are you trying to avoid
getting exclusive access to the cacheline when not needed?

From 1585339883049959903@xxx Tue Nov 28 19:38:49 +0000 2017
X-GM-THRID: 1585216393710549190
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread