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
References: <20200303233609.713348-1-jarkko.sakkinen@linux.intel.com>
 <20200303233609.713348-22-jarkko.sakkinen@linux.intel.com>
 <CAOASepPi4byhQ21hngsSx8tosCC-xa=y6r4j=pWo2MZGeyhi4Q@mail.gmail.com>
 <20200315012523.GC208715@linux.intel.com> <CAOASepP9GeTEqs1DSfPiSm9ER0whj9qwSc46ZiNj_K4dMekOfQ@mail.gmail.com>
 <7f9f2efe-e9af-44da-6719-040600f5b351@fortanix.com>
In-Reply-To: <7f9f2efe-e9af-44da-6719-040600f5b351@fortanix.com>
From:   Nathaniel McCallum <npmccallum@redhat.com>
Date:   Mon, 16 Mar 2020 09:57:28 -0400
Message-ID: <CAOASepNifZdBmg59sJcP1mqSYMW_C=KsdKq-fCmvAU_5iQ9DFw@mail.gmail.com>
Subject: Re: [PATCH v28 21/22] x86/vdso: Implement a vDSO for Intel SGX
 enclave call
To:     Jethro Beekman <jethro@fortanix.com>
Cc:     Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
        linux-kernel@vger.kernel.org, x86@kernel.org,
        linux-sgx@vger.kernel.org, akpm@linux-foundation.org,
        dave.hansen@intel.com,
        "Christopherson, Sean J" <sean.j.christopherson@intel.com>,
        Neil Horman <nhorman@redhat.com>,
        "Huang, Haitao" <haitao.huang@intel.com>,
        andriy.shevchenko@linux.intel.com, tglx@linutronix.de,
        "Svahn, Kai" <kai.svahn@intel.com>, bp@alien8.de,
        Josh Triplett <josh@joshtriplett.org>, luto@kernel.org,
        kai.huang@intel.com, David Rientjes <rientjes@google.com>,
        cedric.xing@intel.com, Patrick Uiterwijk <puiterwijk@redhat.com>,
        Andy Lutomirski <luto@amacapital.net>,
        Connor Kuehl <ckuehl@redhat.com>,
        Harald Hoyer <harald@redhat.com>,
        Lily Sturmann <lsturman@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Mar 16, 2020 at 9:32 AM Jethro Beekman <jethro@fortanix.com> wrote:
>
> On 2020-03-15 18:53, Nathaniel McCallum wrote:
> > On Sat, Mar 14, 2020 at 9:25 PM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> >>
> >> On Wed, Mar 11, 2020 at 01:30:07PM -0400, Nathaniel McCallum wrote:
> >>> Currently, the selftest has a wrapper around
> >>> __vdso_sgx_enter_enclave() which preserves all x86-64 ABI callee-saved
> >>> registers (CSRs), though it uses none of them. Then it calls this
> >>> function which uses %rbx but preserves none of the CSRs. Then it jumps
> >>> into an enclave which zeroes all these registers before returning.
> >>> Thus:
> >>>
> >>>   1. wrapper saves all CSRs
> >>>   2. wrapper repositions stack arguments
> >>>   3. __vdso_sgx_enter_enclave() modifies, but does not save %rbx
> >>>   4. selftest zeros all CSRs
> >>>   5. wrapper loads all CSRs
> >>>
> >>> I'd like to propose instead that the enclave be responsible for saving
> >>> and restoring CSRs. So instead of the above we have:
> >>>   1. __vdso_sgx_enter_enclave() saves %rbx
> >>>   2. enclave saves CSRs
> >>>   3. enclave loads CSRs
> >>>   4. __vdso_sgx_enter_enclave() loads %rbx
> >>>
> >>> I know that lots of other stuff happens during enclave transitions,
> >>> but at the very least we could reduce the number of instructions
> >>> through this critical path.
> >>
> >> What Jethro said and also that it is a good general principle to cut
> >> down the semantics of any vdso as minimal as possible.
> >>
> >> I.e. even if saving RBX would make somehow sense it *can* be left
> >> out without loss in terms of what can be done with the vDSO.
> >
> > Please read the rest of the thread. Sean and I have hammered out some
> > sensible and effective changes.
>
> I'm not sure they're sensible? By departing from the ENCLU calling convention, both the VDSO
> and the wrapper become more complicated.

For the vDSO, only marginally. I'm counting +4,-2 instructions in my
suggestions. For the wrapper, things become significantly simpler.

> The wrapper because now it needs to implement all
> kinds of logic for different behavior depending on whether the VDSO is or isn't available.

When isn't the vDSO available? Once the patches are merged it will
always be available. Then we also get to live with this interface
forever. I'd rather have a good, usable interface for the long term.

> I agree with Jarkko that everything should be kept small and simple. Calling a couple extra instructions is going to have a negligible effect compared to the actual time EENTER/EEXIT take.

We all agree on small and simple. Nothing I've proposed fails either
of those criteria.

> Can someone remind me why we're not passing TCS in RBX but on the stack?

If you do that, the vDSO will never be callable from C. And, as you've
stated above, calling a couple extra instructions is going to have a
negligible effect.