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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.01.2020 15:03, 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, domain_create() is modified to set
>   max_pages to an initial value, sufficient to cover any domheap
>   allocations required to complete domain creation. The value will be
>   set to the 'real' max_pages when the tool-stack later performs the
>   XEN_DOMCTL_max_mem operation, thus allowing the rest of the domain's
>   memory to be allocated.

I continue to disagree with this approach, and I don't think I've
heard back on the alternative suggestion of using MEMF_no_refcount
instead of MEMF_no_owner. As said before, the page (and any other
ones up to DOMAIN_INIT_PAGES, which I find rather fragile to be
set to 1) will now get accounted against the amount allowed for
the domain to allocate.

It also looks to me as if this will break e.g.
p2m_pod_set_mem_target(), which at the very top has

    /* P == B: Nothing to do (unless the guest is being created). */
    populated = d->tot_pages - p2m->pod.count;
    if ( populated > 0 && p2m->pod.entry_count == 0 )
        goto out;

This code assumes that during domain creation all pages recorded
in ->tot_pages have also been recorded in ->pod.count.

There may be other uses of ->tot_pages which this change conflicts
with.

> - Because the domheap page is no longer a pseudo-xenheap page, the
>   reference counting will prevent the domain from being destroyed. Thus
>   the call to vmx_free_vlapic_mapping() is moved from the
>   domain_destroy() method into the domain_relinquish_resources() method.
>   Whilst in the area, make the domain_destroy() method an optional
>   alternative_vcall() (since it will no longer peform any function in VMX
>   and is stubbed in SVM anyway).

All of this would, afaict, become irrelevant when using MEMF_no_refcount.

There looks to be one issue (which I think we have been discussing
before): By not bumping ->tot_pages, its decrementing upon freeing
the page will be a problem. I can see two possible solutions to this:
- Introduce a flag indicating there should also be no accounting
  upon freeing of the page.
- Instead of avoiding to increment ->tot_pages, _also_ increment
  ->max_pages. The interaction with XEN_DOMCTL_max_mem will then of
  course be, well, interesting.

Jan

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