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

[Julien Grall <julien.grall@arm.com>]

Hi,

On 05/06/2019 08:14, Jan Beulich wrote:
>>>> On 04.06.19 at 18:22, <julien.grall@arm.com> wrote:
>> On 6/4/19 5:14 PM, Jan Beulich wrote:
>>>>>> On 03.06.19 at 18:03, <julien.grall@arm.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel