Date:   Sun, 5 Dec 2021 00:04:05 +0200
From:   Jarkko Sakkinen <jarkko@kernel.org>
To:     Reinette Chatre <reinette.chatre@intel.com>
Cc:     dave.hansen@linux.intel.com, tglx@linutronix.de, bp@alien8.de,
        luto@kernel.org, mingo@redhat.com, linux-sgx@vger.kernel.org,
        x86@kernel.org, seanjc@google.com, kai.huang@intel.com,
        cathy.zhang@intel.com, cedric.xing@intel.com,
        haitao.huang@intel.com, mark.shanahan@intel.com, hpa@zytor.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 02/25] x86/sgx: Add wrappers for SGX2 functions
Message-ID: <Yavl1csrk03Zj4ZW@iki.fi>
References: <cover.1638381245.git.reinette.chatre@intel.com>
 <50ad5005c451f41951327853fb450ba302eadb40.1638381245.git.reinette.chatre@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <50ad5005c451f41951327853fb450ba302eadb40.1638381245.git.reinette.chatre@intel.com>
Precedence: bulk

On Wed, Dec 01, 2021 at 11:23:00AM -0800, Reinette Chatre wrote:
> The SGX ENCLS instruction uses EAX to specify an SGX function and
> may require additional registers, depending on the SGX function.
> ENCLS invokes the specified privileged SGX function for managing
> and debugging enclaves. Several macros are used to wrap the ENCLS
> functionality.
> 
> Add ENCLS wrappers for the SGX2 EMODPR, EMODT, and EAUG functions
> that can make changes to pages of an initialized SGX enclave. The
> EMODPR function is used to restrict enclave page permissions
> as maintained within the enclave (Enclave Page Cache Map (EPCM)
> permissions). The EMODT function is used to change the type of an
> enclave page. The EAUG function is used to dynamically add enclave
> pages to an initialized enclave.
> 
> EMODPR and EMODT accepts two parameters and can fault as well as return
> an SGX error code. EAUG also accepts two parameters but does not return
> an SGX error code. Use existing macros for all new functions.
> 
> Expand enum sgx_return_code with the possible EMODPR and EMODT
> return codes.

These implementation details only obfuscate this commit message, and
it is way too high-level to be useful e.g. for kernel maintenance.

I'd replace it with something like:

"
Add wrappers for ENCLS leaf functions EAUG, EMODT and EMODPR,
which roughly take two steps:

1. EAUG creates a new EPCM entry.
   EMODT and EMODPR modify an existing EPCM entry.
2. Set either .PR = 1 (EMODPR), .MODIFY = 1 (EMODT) or .PENDING = 1 (AUG).

The bit is reset by the enclave by invoking ENCLU leaf function EACCEPT
or EACCEPTCOPY, which will result the EPCM change becoming effective.
"

The current commit message is also not addressing these:

1. What happens if enclaves accesses a memory address with either .PR,
   .MODIFY or .PENDING set in EPCM, other than by the means of EACCEPT
   or EACCEPTCOPY?
2. The calling conditions (e.g. concerning TLB's and ETRACK/IPI/etc
   dance related to it).


If this information was properly contained here, discussing about the
following commits would be much easier.

/Jarkko