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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan



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