Hi Jan
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Tuesday, September 6, 2022 2:34 PM
> To: Penny Zheng <penny.zh...@arm.com>
> Cc: Wei Chen <wei.c...@arm.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; George Dunlap <george.dun...@citrix.com>;
> Julien Grall <jul...@xen.org>; Stefano Stabellini <sstabell...@kernel.org>;
> Wei Liu <w...@xen.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v10 8/9] xen: retrieve reserved pages on
> populate_physmap
>
> On 05.09.2022 09:08, Penny Zheng wrote:
> > Hi jan
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeul...@suse.com>
> >> Sent: Wednesday, August 17, 2022 6:05 PM
> >> To: Penny Zheng <penny.zh...@arm.com>
> >> Cc: Wei Chen <wei.c...@arm.com>; Andrew Cooper
> >> <andrew.coop...@citrix.com>; George Dunlap
> >> <george.dun...@citrix.com>; Julien Grall <jul...@xen.org>; Stefano
> >> Stabellini <sstabell...@kernel.org>; Wei Liu <w...@xen.org>;
> >> xen-devel@lists.xenproject.org
> >> Subject: Re: [PATCH v10 8/9] xen: retrieve reserved pages on
> >> populate_physmap
> >>
> >> On 16.08.2022 04:36, Penny Zheng wrote:
> >>> @@ -2867,6 +2854,61 @@ int __init acquire_domstatic_pages(struct
> >>> domain *d, mfn_t smfn,
> >>>
> >>> return 0;
> >>> }
> >>> +
> >>> +/*
> >>> + * Acquire nr_mfns contiguous pages, starting at #smfn, of static
> >>> +memory,
> >>> + * then assign them to one specific domain #d.
> >>> + */
> >>> +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
> >>> + unsigned int nr_mfns, unsigned
> >>> +int
> >>> +memflags) {
> >>> + struct page_info *pg;
> >>> +
> >>> + ASSERT_ALLOC_CONTEXT();
> >>> +
> >>> + pg = acquire_staticmem_pages(smfn, nr_mfns, memflags);
> >>> + if ( !pg )
> >>> + return -ENOENT;
> >>> +
> >>> + if ( assign_domstatic_pages(d, pg, nr_mfns, memflags) )
> >>> + return -EINVAL;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +/*
> >>> + * Acquire a page from reserved page list(resv_page_list), when
> >>> +populating
> >>> + * memory for static domain on runtime.
> >>> + */
> >>> +mfn_t acquire_reserved_page(struct domain *d, unsigned int
> >>> +memflags) {
> >>> + struct page_info *page;
> >>> +
> >>> + ASSERT_ALLOC_CONTEXT();
> >>> +
> >>> + /* Acquire a page from reserved page list(resv_page_list). */
> >>> + spin_lock(&d->page_alloc_lock);
> >>> + page = page_list_remove_head(&d->resv_page_list);
> >>> + spin_unlock(&d->page_alloc_lock);
> >>> + if ( unlikely(!page) )
> >>> + return INVALID_MFN;
> >>> +
> >>> + if ( !prepare_staticmem_pages(page, 1, memflags) )
> >>> + goto fail;
> >>> +
> >>> + if ( assign_domstatic_pages(d, page, 1, memflags) )
> >>> + goto fail_assign;
> >>> +
> >>> + return page_to_mfn(page);
> >>> +
> >>> + fail_assign:
> >>> + free_staticmem_pages(page, 1, memflags & MEMF_no_scrub);
> >>
> >> Doesn't this need to be !(memflags & MEMF_no_scrub)? And then - with
> >
> > I got a bit confused about this flag MEMF_no_scrub, does it mean no
> > need to scrub?
>
> Yes, as its name says.
>
> > Since I saw that in alloc_domheap_pages(...)
> > if ( assign_page(pg, order, d, memflags) )
> > {
> > free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> > return NULL;
> > }
> > It doesn't contain exclamation mark too...
>
> Hmm, you're right - on these error paths the scrubbing is needed if the page
> wasn't previously scrubbed, as part of the set of pages may have been
> transiently exposed to the guest (and by guessing it may have been able to
> actually access the pages; I'm inclined to say it's its own fault though if
> that
> way information is being leaked).
>
Then, the same for the acquire_domstatic_pages(...)
if ( assign_pages(pg, nr_mfns, d, memflags) )
{
free_staticmem_pages(pg, nr_mfns, memflags & MEMF_no_scrub);
return -EINVAL;
}
On this error path, it has misused the MEMF_no_scrub too.
But IMO, as we are talking about these pages will always be reserved to the
guest,
maybe here it also doesn't need scrubbing at all?
> But ...
>
> >> assignment having failed and with it being just a single page we're
> >> talking about, the page was not exposed to the guest at any point
> >> afaict. So I don't see the need for scrubbing in the first place.
>
> while my comment wasn't really correct, as said - you don't need any
> scrubbing here at all, I think.
>
> Jan
