Re: [PATCH v4] x86/sgx: Free backing memory after faulting the enclave page

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, Feb 28, 2022 at 07:32:02AM -0800, Dave Hansen wrote:
> On 2/28/22 04:55, Jarkko Sakkinen wrote:
> > I thought that the formula is so simple that it does not matter if it is
> > just in two sites open coded but I can wrap it too, if required, e.g.
> > 
> > /* 
> >  * Calculate byte offset of a PCMD struct associated to an enclave page.
> >  * PCMD's follow right after the EPC data in the backing storage. In
> >  * addition to the visible enclave pages, there's one extra page slot
> >  * for SECS, before PCMD data.
> >  */
> > static pgoff_t *sgx_encl_page_index_to_pcmd_offset(struct sgx_encl *encl, unsigned long page_index)
> > {
> >         return encl->size + PAGE_SIZE + page_index * sizeof(struct sgx_pcmd);
> > }
> 
> Yes, it's required.  Please wrap it.
> 
> There's also nothing wrong with spreading that calculation across
> several lines.  It may be arithmetically simple, but it's combining
> three or four logical steps.  There's no shame in separating and
> commenting some of those separately.

I can do that but one thing that would make this more documentative would
be to describe the formula as "encl->size + sizeof(struct sgx_secs) +
sizeof(struct sgx_pcmd)", i.e. PAGE_SIZE is a magic number so the
end result would be:

/* 
 * Calculate byte offset of a PCMD struct associated to an enclave page. PCMD's
 * follow right after the EPC data in the backing storage. In addition to the
 * visible enclave pages, there's one extra page slot for SECS, before PCMD
 * structs.
 */
static inline pgoff_t sgx_encl_page_index_to_pcmd_offset(struct sgx_encl *encl, unsigned long page_index)
{
        pgoff_t epc_end_off = encl->size + sizeof(struct sgx_secs);

        return epc_end_off + page_index * sizeof(struct sgx_pcmd);
}

Now it is hard to get this wrong and also the file offset has the nice quality
of packing the page index for PCMD page, and offset within that page. IMHO,
this will nice and clean long-term way to sort this out.

/Jarkko