[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.