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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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;

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.