Re: [PATCH v6 1/7] x86/sgx: Provide indication of life-cycle of EPC pages

["Luck, Tony" <tony.luck@intel.com>]

On Thu, Sep 23, 2021 at 11:24:35PM +0300, Jarkko Sakkinen wrote:
> On Thu, 2021-09-23 at 23:21 +0300, Jarkko Sakkinen wrote:
> > On Wed, 2021-09-22 at 11:21 -0700, Tony Luck wrote:
> > > -		section->pages[i].owner = NULL;
> > > +		section->pages[i].owner = (void *)-1;
> > 
> > Probably should have a named constant.
> > 
> > Anyway, I wonder why we want to do tricks with 'owner', when the
> > struct has a flags field?
> > 
> > Right now its use is so nice and straight forward, and most
> > importantly intuitive.
> > 
> > So what I would do instead of this, would be to add something
> > like
> > 
> > /* Pages, which are being tracked by the page reclaimer. */
> > #define SGX_EPC_PAGE_RECLAIMER_TRACKED	BIT(0)
> > 
> > /* Pages, which are allocated for use. */
> > #define SGX_EPC_PAGE_ALLOCATED		BIT(1)
> > 
> > This would be set by sgx_alloc_epc_page() and reset by
> > sgx_free_epc_page().
> > 
> > In the subsequent patch you could then instead of
> > 
> >        /*
> >         * If there is no owner, then the page is on a free list.
> >         * Move it to the poison page list.
> >         */
> >        if (!page->owner) {
> >                list_del(&page->list);
> >                list_add(&page->list, &sgx_poison_page_list);
> >                goto out;
> >        }
> > 
> > you would
> > 
> >        /*
> >         * If there is no owner, then the page is on a free list.
> >         * Move it to the poison page list.
> >         */
> >        if (!page->flags) {
> >                list_del(&page->list);
> >                list_add(&page->list, &sgx_poison_page_list);
> >                goto out;
> >        }
> > 
> > You don't actually need to compare to that flag because the
> > invariant would be that it is set, as long as the page is
> > not explicitly freed.
> > 
> > I think this is a better solution than in the patch set
> > because it does not introduce any unorthodox use of anything.
> 
> And does not contain any special cases, e.g. when you debug
> something you can always assume that a valid owner pointer is
> always a legit sgx_encl_page instance.
> 
Jarkko,

That's nice. It avoids having to create a fictitious owner for
the dirty pages, and for the sgx_alloc_va_page() case. Which
in turn means that the owner field in struct sgx_epc_page can
remain as "struct sgx_encl_page *owner;" (neatly avoiding DaveH's
request that it be an anonymous union of all the possible types,
because it is back to just being one type).

Thanks!  Will include in next version.

-Tony