Re: [PATCH v10 8/9] xen: retrieve reserved pages on populate_physmap

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.09.2022 09:08, Penny Zheng wrote:
> Hi jan 
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Wednesday, August 17, 2022 6:05 PM
>> To: Penny Zheng <Penny.Zheng@xxxxxxx>
>> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Andrew Cooper
>> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>;
>> Julien Grall <julien@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
>> Wei Liu <wl@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>> 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).

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