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

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 23 January 2020 15:32
> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau Monné
> <roger.pau@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxxxxx>; Ian
> Jackson <ian.jackson@xxxxxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Konrad
> Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>; Kevin
> Tian <kevin.tian@xxxxxxxxx>
> Subject: Re: [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for
> APIC_DEFAULT_PHYS_BASE
> 
> 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.
"

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

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

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

  Paul 

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