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
