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,

On 05/06/2019 08:14, Jan Beulich wrote:
> > > > On 04.06.19 at 18:22, <julien.grall@xxxxxxx> wrote:
> > On 6/4/19 5:14 PM, Jan Beulich wrote:
> > > > > > On 03.06.19 at 18:03, <julien.grall@xxxxxxx> wrote:
> > > > 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...
> Well, I guess it was, but supplying the "why" in the description (or
> attached as a brief comment to the #ifdef) would have helped
> avoid re-raising the same question. However, thinking about it
> again I'm not sure I agree with #ifdef-ing out the entire (large)
> function body - I'd really prefer the alternative approach
> suggested above.
I would appreciate if you clarified it in the previous version rather than 
expecting me to understand the hidden meaning. Because ...
> Or otherwise I'd see yet another separate Kconfig option
> identifying whether an arch supports non-translated mode in the
> first place. That option would select the M2P one then, as I can't
> see how one would go about supporting non-translated guests
> without M2P. In this case you'd add an #ifdef here (placement
> subject to further discussion; personally I'd favor if it was put
> around just the problematic invocation of mfn_to_gmfn(), with
> a suitable #else; alternatively have common code provide a
> stub mfn_to_gmfn()) _and_ a paging_mode_translate() check
> near the top of the function, thus yielding consistent behavior.
... I would have avoided to waste time resending the series without addressing 
your concern.
In this case, I will strongly nack any definition of mfn_to_gmfn() in the 
non-M2P case. We saw the implication on Arm and I just want to see the macro 
completely disappear from Arm.
Furthermore, #ifdef chunk of code means they completely disappear from the code 
base without the aid of the compiler itself (via optimization). This is less 
error-prone than a potentially wrong check and would also help safety 
certification as we don't need.
So I still favor using #ifdef here. I could replace by a specific Kconfig (see a 
previous attempt [1]).

> I find it odd that no such check is there right now, as the public
> header clearly says this is a PV-only interface.
>
> Note that with a paging_mode_translate() check at the top of
> the function (or even at its only call site) the one towards the
> end of the function should then be ditched.
See above my opinion on paging_mode_translate().

Cheers,

[1] https://lists.gt.net/xen/devel/411530

> Jan
--
Julien Grall

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