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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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