On 1/26/22 23:01, Jarkko Sakkinen wrote:
On Sat, Jan 08, 2022 at 04:05:10PM +0200, Jarkko Sakkinen wrote:
+static inline pgoff_t sgx_encl_get_backing_pcmd_nr(struct sgx_encl *encl, pgoff_t index) +{
- return PFN_DOWN(encl->size) + 1 + (index / sizeof(struct sgx_pcmd));
+}
Found it.
This should be
static inline pgoff_t sgx_encl_get_backing_pcmd_nr(struct sgx_encl *encl, pgoff_t index) { return PFN_DOWN(encl->size) + 1 + (index * PAGE_SIZE) / sizeof(struct sgx_pcmd); }
I've looked at this for about 10 minutes and been simultaneously confused as to whether it is right or wrong. That makes it automatically wrong. :)
First, this isn't calculating a "PCMD number". It's calculating backing offset. The "PCMD number" calculation is only a part of it. I think that makes the naming rather sloppy.
Second, I think the typing is sloppy. page_index for example:
static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, struct sgx_epc_page *epc_page, struct sgx_epc_page *secs_page) {
...
pgoff_t page_index;
It's storing a page number:
page_index = PFN_DOWN(encl->size);
not a real offset-into-a-file. That makes it even more confusing when 'page_index' crosses a function boundary, gets renamed to 'index' and then its units get confused.
/* * Given a page number within an enclave (@epc_page_nr), calculate the * offset in bytes into the backing file where that page's PCMD is * located. */ -static inline pgoff_t sgx_encl_get_backing_pcmd_nr(struct sgx_encl *encl, pgoff_t index) +static inline pgoff_t sgx_page_nr_to_pcmd_nr(struct sgx_encl *encl, unsigned long epc_page_nr) { pgoff_t last_epc_offset = PFN_DOWN(encl->size); pgoff_t pcmd_offset;
// The SECS page is stashed in a slot after the // last normal EPC page. Leave space for it: last_epc_offset++;
pcmd_offset = epc_page_nr / sizeof(struct sgx_pcmd);
return last_epc_offset + pcmd_offset; }
Looking at that, I still think your *original* version is correct.
Am I just all twisted around from looking at this code too much? Could you please take another look and send a new version of the patch?