Re: [Qemu-devel] [Xen-devel] [PATCH] increase maxmem before calling xc_domain_populate_physmap

[Don Slutz <dslutz@verizon.com>]

On 12/03/14 09:50, Stefano Stabellini wrote:
> On Wed, 3 Dec 2014, Don Slutz wrote:
>> On 12/03/14 07:20, Stefano Stabellini wrote:
>>> On Wed, 3 Dec 2014, Wei Liu wrote:
>>>> On Tue, Dec 02, 2014 at 03:23:29PM -0500, Don Slutz wrote:
>>>> [...]
>>>>>>>>>             hw_error("xc_domain_getinfo failed");
>>>>>>>>>         }
>>>>>>>>> -    if (xc_domain_setmaxmem(xen_xc, xen_domid, info.max_memkb +
>>>>>>>>> -                            (nr_pfn * XC_PAGE_SIZE / 1024)) <
>>>>>>>>> 0) {
>>>>>>>>> +    max_pages = info.max_memkb * 1024 / XC_PAGE_SIZE;
>>>>>>>>> +    free_pages = max_pages - info.nr_pages;
>>>>>>>>> +    real_free = free_pages;
>>>>>>>>> +    if (free_pages > VGA_HOLE_SIZE) {
>>>>>>>>> +        free_pages -= VGA_HOLE_SIZE;
>>>>>>>>> +    } else {
>>>>>>>>> +        free_pages = 0;
>>>>>>>>> +    }
>>>>>> I don't think we need to subtract VGA_HOLE_SIZE.
>>>>> If you do not use some slack (like 32 here), xen does report:
>>>>>
>>>>>
>>>>> (d5) [2014-11-29 17:07:21] Loaded SeaBIOS
>>>>> (d5) [2014-11-29 17:07:21] Creating MP tables ...
>>>>> (d5) [2014-11-29 17:07:21] Loading ACPI ...
>>>>> (XEN) [2014-11-29 17:07:21] page_alloc.c:1568:d5 Over-allocation for
>>>>> domain
>>>>> 5: 1057417 > 1057416
>>>>> (XEN) [2014-11-29 17:07:21] memory.c:158:d5 Could not allocate order=0
>>>> This message is a bit red herring.
>>>>
>>>> It's hvmloader trying to populate ram for firmware data. The actual
>>>> amount of extra pages needed depends on the firmware.
>>>>
>>>> In any case it's safe to disallow hvmloader from doing so, it will just
>>>> relocate some pages from ram (hence shrinking *mem_end).
>>> That looks like a better solution
>>>
>> I went with a "leave some slack" so that the error message above is not
>> output.
>>
>> When a change to hvmloader is done so that the message does not appear during
>> normal usage, the extra pages in QEMU can be dropped.
> Although those messages look like fatal errors, they are not. It is
> normal way for hvmloader to operate: firstly it tries to allocate extra
> memory until it gets an error, then it continues with normal memory,
> see:
>
> void mem_hole_populate_ram(xen_pfn_t mfn, uint32_t nr_mfns)
> {
>      static int over_allocated;
>      struct xen_add_to_physmap xatp;
>      struct xen_memory_reservation xmr;
>
>      for ( ; nr_mfns-- != 0; mfn++ )
>      {
>          /* Try to allocate a brand new page in the reserved area. */
>          if ( !over_allocated )
>          {
>              xmr.domid = DOMID_SELF;
>              xmr.mem_flags = 0;
>              xmr.extent_order = 0;
>              xmr.nr_extents = 1;
>              set_xen_guest_handle(xmr.extent_start, &mfn);
>              if ( hypercall_memory_op(XENMEM_populate_physmap, &xmr) == 1 )
>                  continue;
>              over_allocated = 1;
>          }
>
>          /* Otherwise, relocate a page from the ordinary RAM map. */
>
> I think there is really nothing to change there. Maybe we want to make
> those warnings less scary.

It was not so much that hvmloader is the one to change (but having it check
for room first might be good), but more that a change to xen would be good
(like changing the wording or maybe only output in debug=y builds, etc.).

Maybe a new XENMEMF to suppress the message?


    -Don Slutz