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 6/5/19 11:10 AM, Jan Beulich wrote:
> > > > On 05.06.19 at 11:35, <julien.grall@xxxxxxx> wrote:
> > 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 ...
> I'm sorry for this, but "thinking about this again" means I was coming
> to that conclusion now, not back then.
>
> > > 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.
> That's one perspective to take. The other is that people changing and
> testing only Arm code will not easily notice if they break that function.
That's not different from any files that are not compiled out on Arm. 
See compat, kexec & co. So why #ifdef is a concern here?
> See for example Andrew's request and follow-up patch to my hweight()
> series, converting from #ifdef to if().

> > So I still favor using #ifdef here. I could replace by a specific Kconfig (see a
> > previous attempt [1]).
> Did you perhaps supply the wrong link? There's no Kconfig option
> being introduced/suggested there.
This was a link to an attempt to add support for XENMEM_exchange on Arm. 
From the discussion back then it is quite clear that the hypercall is 
unlikely to be implemented on Arm. So it feels to me that trying to keep 
the code around is not an option.
> > > 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().
> As said - as per the public header such a check should be added
> _anyway_. The #ifdef, if we decide to go that route, imo should
> then live _after_ that check, with ASSERT_UNREACHABLE() in its #else.
That's an option.

Cheers,

--
Julien Grall

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