On Thu, Feb 24, 2022 at 09:14:05AM -0800, Dave Hansen wrote:
On 2/22/22 04:03, Jarkko Sakkinen wrote:
- if (pcmd_page_empty) {
pgoff_t pcmd_off = encl->size + PAGE_SIZE /* SECS */ +page_index * sizeof(struct sgx_pcmd);sgx_encl_truncate_backing_page(encl, PFN_DOWN(pcmd_off));- }
- return ret;
} @@ -583,7 +613,7 @@ static struct page *sgx_encl_get_backing_page(struct sgx_encl *encl, static int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, struct sgx_backing *backing) {
- pgoff_t pcmd_index = PFN_DOWN(encl->size) + 1 + (page_index >> 5);
- pgoff_t pcmd_off = encl->size + PAGE_SIZE /* SECS */ + page_index * sizeof(struct sgx_pcmd);
Jarkko, I really don't like how this looks. The '/* SECS */' thing is pretty ugly and the comment in the middle of an arithmetic operation is just really hard to read.
Then, there's the fact that this gem is copied-and-pasted. Oh, and it looks a wee bit over 80 columns.
Today you can have 100.
I went to the trouble of writing a nice, fully-fleshed-out helper function for this with a comment included:
https://lore.kernel.org/all/8afec431-4dfc-d8df-152b-76cca0e17ccb@intel.com/
Keeping full byte offset up until parts of it are required for something makes the formula just a simple equation of additions and multiplications, e.g. nothing like "/ sizeof(struct sgx_pcmd)" is required.
Then you get the PCMD page index will be just PFN_DOWN(pcmd_off) and offset inside that page is pcmd_off & PAGE_MASK. At least fro me this is more intuitive way to do the calculations.
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); }
Was there a problem using that? The change from the last version is:
- Sanitized the offset calculations.
Given that there have been multiple different calculations over the four versions so far, which version was right? v3 or v4?
This one has correct and tested calculations but for peer test probably Reinette should verify that. I tested this with my laptop in bare metal.
BR, Jarkko