> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: 22 January 2020 16:17
> To: Durrant, Paul <pdurr...@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Jun Nakajima <jun.nakaj...@intel.com>;
> Kevin Tian <kevin.t...@intel.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Wei Liu <w...@xen.org>; Roger Pau Monné
> <roger....@citrix.com>; George Dunlap <george.dun...@eu.citrix.com>; Ian
> Jackson <ian.jack...@eu.citrix.com>; Julien Grall <jul...@xen.org>; Konrad
> Rzeszutek Wilk <konrad.w...@oracle.com>; Stefano Stabellini
> <sstabell...@kernel.org>
> Subject: Re: [PATCH 3/3] x86 / vmx: use a 'normal' domheap page for
> APIC_DEFAULT_PHYS_BASE
>
> 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).
>
Ok.
> 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.
>
Not sure why it was done that way. It's inserted into the guest P2M so having
it owned by the guest seems like the right thing to do. A malicious guest could
decrease-reservation it and I guess it avoids special-casing there.
> > @@ -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 ...
>
You remember what happened last time we did that (with the ioreq server page),
right? That's why assign_pages() vetoes non-refcounted pages.
> > --- 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).
>
True, but there is 'slop' to deal with things like the ioreq pages and I think
this page is logically similar.
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
