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 16:56, Durrant, Paul wrote:
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 23 January 2020 15:32
>>
>> 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.
> 
> I responded in v1:
> 
> "
>> Did you consider passing MEMF_no_refcount here, to avoid the
>> fiddling with assign_pages()? That'll in particular also
>> avoid ...
>>
> 
> You remember what happened last time we did that (with the ioreq
> server page), right? That's why assign_pages() vetoes non-refcounted
> pages.
> "

Interesting - this mail appears to never have reached me. I'm
sorry for implying you didn't reply.

>> 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.
> 
> Yes, that's exactly the problem with assigning MEMF_no_refcount
> pages, which is way it is outlawed.

Outlawed? It's being special treated, but nothing more/worse.

>> I can see two possible solutions to this:
>> - Introduce a flag indicating there should also be no accounting
>>   upon freeing of the page.
> 
> What sort of flag did you have in mind? Do we have space anywhere
> in type-info or count-info to put it? If we can make assigning
> non-refcounted pages safe then it's certainly an attractive option.

Stealing a bit from PGC_count_mask would likely be the way to go,
unless we can figure a PGC_* bit which can safely be overloaded.
Type-info wouldn't be the right place, I guess.

>> - 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.
>>
> 
> Indeed, which is why I opted for the simple approach. As I've said in
> other responses, max_pages ought be set by the toolstack when the
> domain is created so I wanted to come up with something that's not
> too invasive until that change is made so if the pages do need to be
> ref-counted then I guess I'll have to figure out how to make the
> initial allocation co-exist with PoD.

I'd suggest you don't even try to. The interaction with PoD was just
the example I could easily think of because of having touched PoD
code recently. The general accounting issue that I've also explained
in my previous reply is enough reason to not want to go this route:
The amount of memory given / givable to a domain should not change
as a (silent) side effect of this patch.

And let's face it - the reason why the VMX code does what it does
is because there are no (obvious) easy alternatives.

Jan

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