[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 16 Sep 2021 08:32:44 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=k945y5QOEnDG8MEEiPbwh7dChsxCNiGHKDa1fcct2dk=; b=ZTqb99Ac4XTDCNvLoDOfufQ5c3M8YrsYvHTnY70t9TVc6UbFDaOK5jOUvlmz3ndAs4lv94wA+u2xTBCRz4kCgjePK0sl+n7jysqW5i0IBxdAXBDBRPqlPYu5tjfRfuhO2zi98l9Q2uM7o8UNDIvCI4dRjR/FzyiTFSz/psfbwdTElKDLShXgmF+fTi7sD8pWrFfmU1JwneY+JZptsKqL2BJiFtvzutKy+D64rnDihSLzwyLy9AZsNOQBMcRAWSFK06lqmHiovB3byhwlTQ/jMtU8xwebYWBQab7W8aIdnpUglhwC92SYxTIZX3k3wxCO1AmzZA+hzu4Q712zRc5S6w==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=QUe8hi+nQ6NVAxbmGMsjbTBuE6BIeB7HRoosTI/kHEV2K++D2/f8rUQ7cOnU81TF/Z59R/kh/3km7hHTBFqKSGs0n78+AVCfhZ456MMqOf1M2ppvJ/CSk+9WHBA1kAE771FrFLG/awWCxlTHjPv91k6ui3hO7i5WVxWlFsyxxsbsBeKzcN4tgLCkg4+Q/sLQrbgntjm6uv4ILR3WglF324Fq/32duAHhw9jyn2/qFuHiyax58cxxpblhB1pWSPzuzniEueu/7JZJPCwP1IY29Wle6GjUOnsR9sEERq4LFfPdiyFX7BaNTsYIioiuC41d1MEuHwExJIi713qas+YQ4A==
  • Authentication-results: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=suse.com;
  • Cc: Penny Zheng <penny.zheng@xxxxxxx>, Bertrand.Marquis@xxxxxxx, Wei.Chen@xxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxxx, julien@xxxxxxx
  • Delivery-date: Thu, 16 Sep 2021 06:32:57 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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?




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.