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

[Don Slutz <dslutz@verizon.com>]

On 11/26/14 13:17, Stefano Stabellini wrote:
> On Tue, 25 Nov 2014, Andrew Cooper wrote:
>> On 25/11/14 17:45, Stefano Stabellini wrote:
>>> Increase maxmem before calling xc_domain_populate_physmap_exact to avoid
>>> the risk of running out of guest memory. This way we can also avoid
>>> complex memory calculations in libxl at domain construction time.
>>>
>>> This patch fixes an abort() when assigning more than 4 NICs to a VM.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>
>>> diff --git a/xen-hvm.c b/xen-hvm.c
>>> index 5c69a8d..38e08c3 100644
>>> --- a/xen-hvm.c
>>> +++ b/xen-hvm.c
>>> @@ -218,6 +218,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
>>>       unsigned long nr_pfn;
>>>       xen_pfn_t *pfn_list;
>>>       int i;
>>> +    xc_dominfo_t info;
>>>   
>>>       if (runstate_check(RUN_STATE_INMIGRATE)) {
>>>           /* RAM already populated in Xen */
>>> @@ -240,6 +241,13 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
>>>           pfn_list[i] = (ram_addr >> TARGET_PAGE_BITS) + i;
>>>       }
>>>   
>>> +    if (xc_domain_getinfo(xen_xc, xen_domid, 1, &info) < 0) {
>> xc_domain_getinfo()'s interface is mad, and provides no guarantee that
>> it returns the information for the domain you requested.  It also won't
>> return -1 on error.  The correct error handing is:
>>
>> (xc_domain_getinfo(xen_xc, xen_domid, 1, &info) != 1) || (info.domid !=
>> xen_domid)
> It might be wiser to switch to xc_domain_getinfolist

Either needs the same tests, since both return an vector of info.

>
>> ~Andrew
>>
>>> +        hw_error("xc_domain_getinfo failed");
>>> +    }
>>> +    if (xc_domain_setmaxmem(xen_xc, xen_domid, info.max_memkb +
>>> +                            (nr_pfn * XC_PAGE_SIZE / 1024)) < 0) {

There are two big issues and 1 minor one with this.
1) You will allocate the videoram again.
2) You will never use the 1 MB already allocated for option ROMs.

And the minor one is that you can increase maxmem more then is needed.


Here is a better if:

-    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;
+    need_pages = nr_pfn - free_pages;
+    if ((free_pages < nr_pfn) &&
+       (xc_domain_setmaxmem(xen_xc, xen_domid, info.max_memkb +
+                            (need_pages * XC_PAGE_SIZE / 1024)) < 0)) {


My testing shows a free 32 pages that I am not sure where they come 
from.  But
the code about is passing my 8 nics of e1000.


    -Don Slutz


>>> +        hw_error("xc_domain_setmaxmem failed");
>>> +    }
>>>       if (xc_domain_populate_physmap_exact(xen_xc, xen_domid, nr_pfn, 0, 0, pfn_list)) {
>>>           hw_error("xen: failed to populate ram at " RAM_ADDR_FMT, ram_addr);
>>>       }
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
>>