Re: [Xen-devel] [PATCH for-4.12 4/8] x86/shadow: alloc enough pages so initialization doesn't fail

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.02.19 at 12:47, <roger.pau@xxxxxxxxxx> wrote:
> On Tue, Feb 05, 2019 at 04:21:52AM -0700, Jan Beulich wrote:
>> >>> On 30.01.19 at 11:36, <roger.pau@xxxxxxxxxx> wrote:
>> > Current code in shadow_enable will allocate a shadow pool of 4MB
>> > regardless of the values of sh_min_allocation or
>> > shadow_min_acceptable_pages, which means that calls to
>> > shadow_alloc_p2m_page can fail even after the check and allocation
>> > done just above.
>> > 
>> > Fix this by always checking that the pool is big enough so the rest of
>> > the shadow_init function cannot fail due to lack of pages in the
>> > shadow pool. This is relevant to shadow_alloc_p2m_page which requires
>> > a minimum amount of shadow_min_acceptable_pages(d) + 1 in the pool.
>> > 
>> > This allows booting a guest using shadow and more than 6 vCPUs.
>> 
>> I'm routinely booting 8-vCPU guests without issues.
> 
> For me the following simple example with 8 vcpus doesn't work:
> 
> # cat test.cfg
> name = "test"
> type = "hvm"
> 
> memory = 256

I admit I've never tried this small a guest with ...

> vcpus = 8

... this many vCPU-s.

> hap = 0
> # xl create test.cfg
> Parsing config from test.cfg
> libxl: error: libxl_create.c:578:libxl__domain_make: domain creation fail: 
> Cannot allocate memory
> libxl: error: libxl_create.c:975:initiate_domain_create: cannot make domain: 
> -3
> 
>> 
>> > --- a/xen/arch/x86/mm/shadow/common.c
>> > +++ b/xen/arch/x86/mm/shadow/common.c
>> > @@ -2705,6 +2705,11 @@ int shadow_enable(struct domain *d, u32 mode)
>> >      uint32_t *e;
>> >      int rv = 0;
>> >      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> > +    /*
>> > +     * Required minimum amount of pool pages plus 4MB. This is required 
>> > so the
>> > +     * calls to p2m_alloc_table and shadow_alloc_p2m_page below don't 
>> > fail.
>> > +     */
>> > +    unsigned int min_pages = shadow_min_acceptable_pages(d) + 1024;
>> 
>> sh_min_allocation() also takes the memory size of the domain into
>> account. Aren't you therefore risking to regress larger guests by
>> instead using a fixed amount here? The more that ...
> 
> shadow_enabled is called by domain_create, and at this point the
> memory size of the guest is not yet known AFAICT. I assume the
> toolstack will make further hypercalls to set a suitable sized shadow
> memory pool after the domain has been created and before populating
> the physmap.

Hmm, good point, and no, I don't think there are subsequent calls
to shadow_enable(); at least I can't find an invocation of
XEN_DOMCTL_SHADOW_OP_ENABLE.

But then the correct course of action would be to suitably grow the
shadow pool as memory gets added to the domain (be it Dom0 or
a DomU). Sticking to a fixed value of 1024 can't very well be the
best course of action in all possible cases.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel