Re: [PATCH v7 5/7] xen: re-define assign_pages and introduce a new function assign_page

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.09.2021 20:12, Stefano Stabellini wrote:
> On Wed, 15 Sep 2021, Jan Beulich wrote:
>> On 10.09.2021 18:23, Stefano Stabellini wrote:
>>> On Fri, 10 Sep 2021, Jan Beulich wrote:
>>>> On 10.09.2021 04:52, Penny Zheng wrote:
>>>>> In order to deal with the trouble of count-to-order conversion when page 
>>>>> number
>>>>> is not in a power-of-two, this commit re-define assign_pages for nr pages 
>>>>> and
>>>>> assign_page for original page with a single order.
>>>>>
>>>>> Backporting confusion could be helped by altering the order of 
>>>>> assign_pages
>>>>> parameters, such that the compiler would point out that adjustments at 
>>>>> call
>>>>> sites are needed.
>>>>
>>>> Thanks, this now takes care of my primary concern. However, I (now?) have
>>>> another (I thought I would have mentioned this before):
>>>>
>>>>> --- a/xen/common/page_alloc.c
>>>>> +++ b/xen/common/page_alloc.c
>>>>> @@ -2259,9 +2259,9 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
>>>>>  
>>>>>  
>>>>>  int assign_pages(
>>>>> -    struct domain *d,
>>>>>      struct page_info *pg,
>>>>> -    unsigned int order,
>>>>> +    unsigned long nr,
>>>>
>>>> If this really is to be "unsigned long" (and not "unsigned int"), then...
>>>
>>> Thanks for spotting this. I think it makes sense to use "unsigned int
>>> nr" here.
>>
>> I see you've made the change while committing, but the subsequent patch
>> then would have needed adjustment as well: It's now silently truncating
>> an "unsigned long" value to "unsigned int". It was the splitting that's
>> now needed there _anyway_ that made me wonder whether the patch here
>> really is worthwhile to have. But of course acquire_domstatic_pages()
>> could for now also simply reject too large values ...
> 
> Yes. Are you thinking of a check like the following at the beginning of
> acquire_domstatic_pages?
> 
>     if ( nr_mfns > UINT_MAX )
>         return -EINVAL;

Something like this might be the way to go.

> An alternative would be to change acquire_domstatic_pages to take an
> unsigned int as nr_mfns parameter, but then it would just push the
> problem up one level to allocate_static_memory, which is reading the
> value from device tree so it is out of our control. So I think it is a
> good idea to have an explicit check and acquire_domstatic_pages would be
> a good place for it.

If this was put closer to the DT parsing, maybe a (more) sensible error
message could be given?

Jan