On Tue, Jan 04, 2022 at 04:36:28PM -0800, Dave Hansen wrote:
> On 12/28/21 3:37 PM, Jarkko Sakkinen wrote:
> > On Mon, Dec 20, 2021 at 09:46:40AM -0800, Kristen Carlson Accardi wrote:
> >> +int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index,
> >> + struct sgx_backing *backing)
> >> +{
> >> + return sgx_encl_get_backing(encl, page_index, backing);
> >> +}
> > Is this wrapping necessary?
>
> Yes, I think so.
>
> > Also, there is ambiguous terminology:
> >
> > 1. Local function: "get_backing"
> > 2. Exported function: "lookup_backing"
>
> I'm not sure what you're getting at.
>
> There are three important things that you do with backing storage:
>
> 1. Allocate it
> 2. Find it
> 3. De-allocate (free) it
>
> Right now, the code has a pattern where it does:
>
> get_backing();
> // do something
> put_backing();
>
> That sure as heck looks like it is allocating and freeing it. But, it's
> actually *maybe* doing an allocation. The "find it" path also looks
> *EXACTLY* the same as the actual allocation path. You might also recall
> that the original code didn't even *have* a (real) free path.
>
> The "wrapping" is really just naming the two different operations that
> use the "get" function: lookup and allocate. It's not just wrapping,
> it's clarify the logical behavior.
Why it makes sense to keep sgx_encl_get_backing(), if it has zero call
sites and not open-code its implementation to sgx_encl_lookup_backing().
I'm also wondering, why here the function is not named as
sgx_encl_charge_backing(), i.e. follow the naming convention? It would be
easier to remember the flow, when reading the code. Since we use "not as
common name", let's take advantage of it to make maintaining the code
easier later on.
The commit message says:
"Modify the existing flow for requesting backing pages to reduce the
available backing page counter and confirm that the limit has not been
exceeded. Backing page usage for loading EPC pages back out of the shared
memory do not incur a charge."
I would add, in order to make this less abstract:
"
In other words, replace call sites of sgx_encl_get_backing() with either:
* sgx_encl_lookup_backing() for ELDU, which does not cause sgx_charge_mem()
to be invoked.
* sgx_encl_alloc_backing() for EWB, which does cause sgx_charge_mem()
to be invoked.
"
It's currently way too abstract description of the code change.
/Jarkko
