Hi Dave,
On 5/10/2022 2:55 PM, Dave Hansen wrote:
> On 5/9/22 14:48, Reinette Chatre wrote:
> ...
>> +#define PCMDS_PER_PAGE (PAGE_SIZE / sizeof(struct sgx_pcmd))
>> +/*
>> + * 32 PCMD entries share a PCMD page. PCMD_FIRST_MASK is used to
>> + * determine the page index associated with the first PCMD entry
>> + * within a PCMD page.
>> + */
>> +#define PCMD_FIRST_MASK GENMASK(4, 0)
>> +
>> +/**
>> + * pcmd_page_in_use() - Query if any enclave page associated with a PCMD
>> + * page is in process of being reclaimed.
>
> The name is a bit too generic for what it does.
This function is called after a PCMD page is determined to be empty and is
about to be truncated. The question this function needs to answer is, "is this
PCMD page in use?" - that is, even though it is empty, it cannot be truncated
since there is a reference to this page (specifically from the reclaimer) and
a reference is obtained with the intent to write data to the page.
For some other name options, how about:
reclaimer_has_pcmd_ref()
reclaimer_using_pcmd()
Do any of these look better to you?
>
>> + * @encl: Enclave to which PCMD page belongs
>> + * @start_addr: Address of enclave page using first entry within the PCMD page
>> + *
>> + * When an enclave page is reclaimed some Paging Crypto MetaData (PCMD) is
>> + * stored. The PCMD data of a reclaimed enclave page contains enough
>> + * information for the processor to verify the page at the time
>> + * it is loaded back into the Enclave Page Cache (EPC).
>> + *
>> + * The backing storage to which enclave pages are reclaimed is laid out as
>> + * follows:
>> + * All enclave pages:SECS page:PCMD pages
>> + *
>> + * Each PCMD page contains the PCMD metadata of
>> + * PAGE_SIZE/sizeof(struct sgx_pcmd) enclave pages.
>> + *
>> + * A PCMD page can only be truncated if it is (a) empty, and (b) no enclave
>> + * page sharing the PCMD page is in the process of being reclaimed.
>
> Let's also point out that (b) is _because_ the page is about to become
> non-empty.
Thank you. I will emphasize that.
>
>> + * The reclaimer sets the SGX_ENCL_PAGE_BEING_RECLAIMED flag when it
>> + * intends to reclaim that enclave page - it means that the PCMD data
>> + * associated with that enclave page is about to get some data and thus
>> + * even if the PCMD page is empty, it should not be truncated.
>> + *
>> + * Return: 1 the PCMD page is in use, 0 if is not in use, -EFAULT on error
>> + */
>> +static int pcmd_page_in_use(struct sgx_encl *encl,
>> + unsigned long start_addr)
>> +{
>> + struct sgx_encl_page *entry;
>> + unsigned long addr;
>> + int reclaimed = 0;
>> + int i;
>
> Can 'addr' and 'entry' be declared within the loop instead?
Yes, will do.
>
>> +
>> + /*
>> + * PCMD_FIRST_MASK is based on number of PCMD entries within
>> + * PCMD page being 32.
>> + */
>> + BUILD_BUG_ON(PCMDS_PER_PAGE != 32);
>> +
>> + for (i = 0; i < PCMDS_PER_PAGE; i++) {
>> + addr = start_addr + i * PAGE_SIZE;
>> +
>> + /*
>> + * Stop when reaching the SECS page - it does not
>> + * have a page_array entry and its reclaim is
>> + * started and completed with enclave mutex held so
>> + * it does not use the SGX_ENCL_PAGE_BEING_RECLAIMED
>> + * flag.
>> + */
>> + if (addr == encl->base + encl->size)
>> + break;
>> +
>> + entry = xa_load(&encl->page_array, PFN_DOWN(addr));
>> + if (!entry)
>> + return -EFAULT;
>
> Can xa_load() return NULL if it simply can't find an 'encl_page' at
> 'addr'? In that case, there would never be a PCMD entry for the page
> since it doesn't exist. Returning -EFAULT would be a
> pcmd_page_in_use()==true condition, which doesn't seem accurate.
Thank you very much for catching this. This should be:
entry = xa_load(&encl->page_array, PFN_DOWN(addr));
if (!entry)
continue;
>
>> + /*
>> + * VA page slot ID uses same bit as the flag so it is important
>> + * to ensure that the page is not already in backing store.
>> + */
>
> Probably a patch for another day, but isn't this a bit silly? Are we
> short of bits in ->desc or something?
I am not familiar with the history behind this. The VA slot information
do indeed take up quite a few bits though, leaving just three bits
available.
>
>> + if (entry->epc_page &&
>> + (entry->desc & SGX_ENCL_PAGE_BEING_RECLAIMED)) {
>> + reclaimed = 1;
>> + break;
>> + }
>> + }
>> +
>> + return reclaimed;
>> +}
>
> Could we also please add a comment about encl->lock needing to be held
> over this? Without that, there would be all kinds of trouble.
Will do. I will add a "Context:" portion to the function's kernel-doc.
Reinette
