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?
