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

From: "Jan Beulich" <JBeulich@suse.com>
To: "Julien Grall" <julien.grall@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	"andrii_anisov@epam.com" <andrii_anisov@epam.com>,
	WeiLiu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	oleksandr_tyshchenko@epam.com,
	xen-devel <xen-devel@lists.xenproject.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v3 07/14] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call
Date: Wed, 05 Jun 2019 01:14:11 -0600	[thread overview]
Message-ID: <5CF76BC30200007800235596@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <b85178eb-05ad-da33-3f97-9fdcdf65727b@arm.com>

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

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel