On Mon, Jan 31, 2022 at 03:30:52PM -0800, Dave Hansen wrote:
> 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?
I gave some though and this is what I came up with.
So what do you think about this:
pgoff_t pcmd_off = encl->size + PAGE_SIZE + page_idex * sizeof(struct sgx_pcmd);
This makes the calculations quite simple:
backing->pcmd = pcmd;
backing->pcmd_offset = pcmd_off & PAGE_SIZE;
and
pcmd = sgx_encl_get_backing_page(encl, PFN_DOWN(pcmd_off));
Then pgoff_t is actually also used for what it is meant to be.
/Jarkko
