Re: [Xen-devel] [PATCH v3 07/14] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 6/4/19 5:14 PM, Jan Beulich wrote:
> > > > On 03.06.19 at 18:03, <julien.grall@xxxxxxx> wrote:
> > While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty
> > bogus as we directly return the MFN passed in parameter.
> >
> > Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There
> > are only 3 callers:
> >      - iommu_hwdom_init: mfn_to_gmfn is used for creating IOMMU
> >      page-tables when the P2M is not shared with the IOMMU. No issues so
> >      far as Arm does not yet support non-shared P2M case.
> >      - memory_exchange: Arm cannot not use it because steal_page is not
> >      implemented.
> >      - getdomaininfo: Toolstack may map the shared page. It looks like
> >      this is mostly used for mapping the P2M of PV guest. Therefore the
> >      issue might be minor.
> >
> > Implementing the M2P on Arm is not planned. The M2P would require
> > significant
> > amount of VA address (very tough on 32-bit) that can hardly be justified with
> > the current use of mfn_to_gmfn.
> >      - iommu_hwdom_init: mfn_to_gmfn is used because the creating of the
> >      IOMMU page-tables is delayed until the first device is assigned.
> >      In the embedded case, we will known in most of the times what
> >      devices are assigned during the domain creation. So it is possible
> >      to take to enable the IOMMU from start. See [1] for the patch.
> >      - memory_exchange: This does not work and I haven't seen any
> >      request for it so far.
> >      - getdomaininfo: The structure on Arm does not seem to contain a lot
> >      of useful information on Arm. It is unclear whether we want to
> >      allow the toolstack mapping it on Arm.
> >
> > This patch introduces a config option HAS_M2P to tell whether an
> > architecture implements the M2P.
> >      - iommu_hwdom_init: For now, we require the M2P support when the IOMMU
> >      is not sharing the P2M.
> >      - memory_exchange: The hypercall is marked as not supported when the
> >      M2P does not exist.
> But where's the connection between there being M2P and the
> availability of this operation? I think I've suggested so before:
> Why don't we simply disable this operation for translated
> guests (in an independent patch)?
And I answered that mfn_to_gmfn() is used in the function. I really 
don't want to implement the macro on Arm (even if it is dummy).
You haven't answered back to that comment and I assumed this was fine 
with you...
> >      - getdomaininfo: A new helper is introduced to wrap the call to
> >      mfn_to_gfn/mfn_to_gmfn. For Arm, a fixed value will be provided that will
> >      fail on mapping if used.
> This reads slightly stale now.
ok.

> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> >       hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
> >       if ( need_iommu_pt_sync(d) )
> >       {
> > +        int rc = 0;
> > +#ifdef CONFIG_HAS_M2P
> >           struct page_info *page;
> >           unsigned int i = 0, flush_flags = 0;
> > -        int rc = 0;
> >   
> >           page_list_for_each ( page, &d->page_list )
> >           {
> > @@ -221,6 +222,11 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> >           if ( rc )
> >               printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
> >                      d->domain_id, rc);
> > +#else
> > +        ASSERT_UNREACHABLE();
> > +        rc = -EOPNOTSUPP;
> > +#endif
> > +
> >       }
> Please don't add a blank line ahead of a closing brace.
ok.

Cheers,

--
Julien Grall

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