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.
> + * @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.
> + * 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?
> +
> + /*
> + * 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.
> + /*
> + * 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?
> + 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.
