Re: [Xen-devel] [PATCH v7 01/12] x86/mm: allow a privileged PV domain to map guest mfns

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 25 September 2017 14:03
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v7 01/12] x86/mm: allow a privileged PV domain to map
> guest mfns
> 
> >>> On 18.09.17 at 17:31, <paul.durrant@xxxxxxxxxx> wrote:
> > In the case where a PV domain is mapping guest resources then it needs
> make
> > the HYPERVISOR_mmu_update call using DOMID_SELF, rather than the
> guest
> > domid, so that the passed in gmfn values are correctly treated as mfns
> > rather than gfns present in the guest p2m.
> 
> Since things are presently working fine, I think the description is not
> really accurate. You only require the new behavior if you don't know
> the GFN of the page you want to map, and that it has to be
> DOMID_SELF that should be passed also doesn't appear to derive
> from anything else. To properly judge about the need for this patch
> it would help if it was briefly explained why being able to map by GFN
> is no longer sufficient, and to re-word the DOMID_SELF part.
> 
> The other aspect I don't understand is why this is needed for PV
> Dom0, but not for PVH. The answer here can't be "because PVH
> Dom0 isn't supported yet", because it eventually will be, and then
> there will still be the problem of PVH supposedly having no notion
> of MFNs (be their own or foreign guest ones). The answer also
> can't be "since it would use XENMAPSPACE_gmfn_foreign", as
> that's acting in terms of GFN too.
> 
> > This patch removes a check which currently disallows mapping of a page
> when
> > the owner of the page tables matches the domain passed to
> > HYPERVISOR_mmu_update, but that domain is not the real owner of the
> page.
> > The check was introduced by patch d3c6a215ca9 ("x86: Clean up
> > get_page_from_l1e() to correctly distinguish between owner-of-pte and
> > owner-of-data-page in all cases") but it's not clear why it was needed.
> 
> I think the goal here simply was to not permit anything that doesn't
> really need permitting. Furthermore the check being "introduced"
> there was, afaict, replacing the earlier d != curr->domain.
> 
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -1024,12 +1024,15 @@ get_page_from_l1e(
> >                     (real_pg_owner != dom_cow) ) )
> >      {
> >          /*
> > -         * Let privileged domains transfer the right to map their target
> > -         * domain's pages. This is used to allow stub-domain pvfb export to
> > -         * dom0, until pvfb supports granted mappings. At that time this
> > -         * minor hack can go away.
> > +         * If the real page owner is not the domain specified in the
> > +         * hypercall then establish that the specified domain has
> > +         * mapping privilege over the page owner.
> > +         * This is used to allow stub-domain pvfb export to dom0. It is
> > +         * also used to allow a privileged PV domain to map mfns using
> > +         * DOMID_SELF, which is needed for mapping guest resources such
> > +         * grant table frames.
> 
> How do grant table frames come into the picture here? So far
> I had assumed only ioreq server pages are in need of this.
> 
> >           */
> > -        if ( (real_pg_owner == NULL) || (pg_owner == l1e_owner) ||
> > +        if ( (real_pg_owner == NULL) ||
> >               xsm_priv_mapping(XSM_TARGET, pg_owner, real_pg_owner) )
> >          {
> >              gdprintk(XENLOG_WARNING,
> 
> I'm concerned of the effect of the change on the code paths
> which you're not really interested in: alloc_l1_table(),
> ptwr_emulated_update(), and shadow_get_page_from_l1e() all
> explicitly pass both domains identical, and are now suddenly able
> to do things they weren't supposed to do. A similar concern
> applies to __do_update_va_mapping() calling mod_l1_table().
> 
> I therefore wonder whether the solution to your problem
> wouldn't rather be MMU_FOREIGN_PT_UPDATE (name subject
> to improvement suggestions). This at the same time would
> address my concern regarding the misleading DOMID_SELF
> passing when really a foreign domain's page is meant.

Looking at this I wonder whether a cleaner solution would be to introduce a new 
domid: DOMID_ANY. This meaning of this would be along the same sort of lines as 
DOMID_XEN or DOMID_IO and would be used in mmu_update to mean 'any page over 
which the caller has privilege'. Does that sound reasonable?

  Paul

> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel