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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 1/23/20 3:32 PM, Jan Beulich wrote:
> 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.

This basically boils down to the "memory accounting swamp", where
various random features need to allocate domain pages to function.

>> - 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.

This seems like a reasonable approach to look into.

 -George

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