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;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (1.0)
Subject: Re: [RFC PATCH 1/5] x86: introduce preemption disable prefix
From:   Andy Lutomirski <luto@amacapital.net>
In-Reply-To: <20181018005420.82993-2-namit@vmware.com>
Date:   Wed, 17 Oct 2018 18:22:48 -0700
Cc:     Ingo Molnar <mingo@redhat.com>, Andy Lutomirski <luto@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        "H . Peter Anvin" <hpa@zytor.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org, Nadav Amit <nadav.amit@gmail.com>,
        x86@kernel.org, Borislav Petkov <bp@alien8.de>,
        David Woodhouse <dwmw@amazon.co.uk>
Content-Transfer-Encoding: quoted-printable
Message-Id: <D247AB7C-2890-49CE-B102-163E5B368FC2@amacapital.net>
References: <20181018005420.82993-1-namit@vmware.com> <20181018005420.82993-2-namit@vmware.com>
To:     Nadav Amit <namit@vmware.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Oct 17, 2018, at 5:54 PM, Nadav Amit <namit@vmware.com> wrote:
>=20
> It is sometimes beneficial to prevent preemption for very few
> instructions, or prevent preemption for some instructions that precede
> a branch (this latter case will be introduced in the next patches).
>=20
> To provide such functionality on x86-64, we use an empty REX-prefix
> (opcode 0x40) as an indication that preemption is disabled for the
> following instruction.

Nifty!

That being said, I think you have a few bugs.  First, you can=E2=80=99t just=
 ignore a rescheduling interrupt, as you introduce unbounded latency when th=
is happens =E2=80=94 you=E2=80=99re effectively emulating preempt_enable_no_=
resched(), which is not a drop-in replacement for preempt_enable(). To fix t=
his, you may need to jump to a slow-path trampoline that calls schedule() at=
 the end or consider rewinding one instruction instead. Or use TF, which is o=
nly a little bit terrifying...

You also aren=E2=80=99t accounting for the case where you get an exception t=
hat is, in turn, preempted.


>=20
> It is expected that this opcode is not in common use.
>=20
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
> arch/x86/entry/entry_64.S            | 10 ++++++++++
> arch/x86/include/asm/nospec-branch.h | 12 ++++++++++++
> 2 files changed, 22 insertions(+)
>=20
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index cb8a5893fd33..31d59aad496e 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -643,6 +643,16 @@ retint_kernel:
>    jnc    1f
> 0:    cmpl    $0, PER_CPU_VAR(__preempt_count)
>    jnz    1f
> +
> +    /*
> +     * Allow to use hint to prevent preemption on a certain instruction.
> +     * Consider an instruction with the first byte having REX prefix
> +     * without any bits set as an indication for preemption disabled.
> +     */
> +    movq    RIP(%rsp), %rax
> +    cmpb    $PREEMPT_DISABLE_PREFIX, (%rax)
> +    jz    1f
> +
>    call    preempt_schedule_irq
>    jmp    0b
> 1:
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/n=
ospec-branch.h
> index 80dc14422495..0267611eb247 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -52,6 +52,12 @@
>    jnz    771b;                \
>    add    $(BITS_PER_LONG/8) * nr, sp;
>=20
> +/*
> + * An empty REX-prefix is an indication that preemption should not take p=
lace on
> + * this instruction.
> + */
> +#define PREEMPT_DISABLE_PREFIX                 (0x40)
> +
> #ifdef __ASSEMBLY__
>=20
> /*
> @@ -148,6 +154,12 @@
> #endif
> .endm
>=20
> +.macro preempt_disable_prefix
> +#ifdef CONFIG_PREEMPT
> +    .byte    PREEMPT_DISABLE_PREFIX
> +#endif
> +.endm
> +
> #else /* __ASSEMBLY__ */
>=20
> #define ANNOTATE_NOSPEC_ALTERNATIVE                \
> --=20
> 2.17.1
>=20