Re: [Xen-devel] [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Jan Beulich <jbeulich@suse.com>]

On 21.01.2020 13:00, Paul Durrant wrote:
> vmx_alloc_vlapic_mapping() currently contains some very odd looking code
> that allocates a MEMF_no_owner domheap page and then shares with the guest
> as if it were a xenheap page. This then requires vmx_free_vlapic_mapping()
> to call a special function in the mm code: free_shared_domheap_page().
> 
> By using a 'normal' domheap page (i.e. by not passing MEMF_no_owner to
> alloc_domheap_page()), the odd looking code in vmx_alloc_vlapic_mapping()
> can simply use get_page_and_type() to set up a writable mapping before
> insertion in the P2M and vmx_free_vlapic_mapping() can simply release the
> page using put_page_alloc_ref() followed by put_page_and_type(). This
> then allows free_shared_domheap_page() to be purged.
> 
> There is, however, some fall-out from this simplification:
> 
> - alloc_domheap_page() will now call assign_pages() and run into the fact
>   that 'max_pages' is not set until some time after domain_create(). To
>   avoid an allocation failure, assign_pages() is modified to ignore the
>   max_pages limit if 'creation_finished' is false. That value is not set
>   to true until domain_unpause_by_systemcontroller() is called, and thus
>   the guest cannot run (and hence cause memory allocation) until
>   creation_finished is set to true.

But this check is also to guard against the tool stack (or possibly
the controlling stubdom) to cause excess allocation. I don't think
the checking should be undermined like this (and see also below).

Since certainly you've looked into this while creating the patch,
could you remind me why it is that this page needs to be owned (as
in its owner field set accordingly) by the guest? It's a helper
page only, after all.

> @@ -3034,12 +3034,22 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
>      if ( !cpu_has_vmx_virtualize_apic_accesses )
>          return 0;
>  
> -    pg = alloc_domheap_page(d, MEMF_no_owner);
> +    pg = alloc_domheap_page(d, 0);

Did you consider passing MEMF_no_refcount here, to avoid the
fiddling with assign_pages()? That'll in particular also
avoid ...

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2269,7 +2269,8 @@ int assign_pages(
>  
>      if ( !(memflags & MEMF_no_refcount) )
>      {
> -        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) )
> +        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) &&
> +             d->creation_finished )
>          {
>              gprintk(XENLOG_INFO, "Over-allocation for domain %u: "
>                      "%u > %u\n", d->domain_id,

... invoking domain_adjust_tot_pages() right below here, which
is wrong for helper pages like this one (as it reduces the
amount the domain is actually permitted to allocate).

Jan

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