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
