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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.09.2022 09:14, Penny Zheng wrote:
> Hi Jan
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Tuesday, September 6, 2022 2:34 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 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).
>>
> 
> 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.

Why do you say "misused"?

> 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?

Perhaps. It feels as if we had been there before, quite some time ago.

Jan