Re: [PATCH] x86/ept: limit calls to memory_type_changed()

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH] x86/ept: limit calls to memory_type_changed()
Date: Mon, 26 Sep 2022 17:36:41 +0200	[thread overview]
Message-ID: <f8c080ca-33a1-ab15-963e-9c5c02c99bce@suse.com> (raw)
In-Reply-To: <YzHEcdCtch7lKh4G@MacBook-Air-de-Roger.local>

On 26.09.2022 17:25, Roger Pau Monné wrote:
> On Mon, Sep 26, 2022 at 04:50:22PM +0200, Roger Pau Monné wrote:
>> On Mon, Sep 26, 2022 at 09:33:10AM +0200, Jan Beulich wrote:
>>> On 23.09.2022 10:35, Roger Pau Monné wrote:
>>>> On Thu, Sep 22, 2022 at 09:21:59PM +0200, Jan Beulich wrote:
>>>>> On 22.09.2022 18:05, Roger Pau Monne wrote:
>>>>> And if we were to restrict the calls, I think we need to clearly
>>>>> tie together the various places which need updating together in
>>>>> case e.g. the condition in epte_get_entry_emt() is changed.
>>>>> Minimally by way of comments, but maybe by way of a small helper
>>>>> function (for which I can't seem to be able to think of a good
>>>>> name) sitting next to epte_get_entry_emt().
>>>>
>>>> Such helper function is also kind of problematic, as it would have to
>>>> live in p2m-ept.c but be used in domctl.c and x86/domctl.c?  It would
>>>> have to go through the p2m_domain indirection structure.
>>>
>>> It would need abstraction at the arch level as well as for !HVM configs
>>> on x86. I'm not sure the indirection layer would actually be needed, as
>>> the contents of the function - despite wanting placing in p2m-ept.c -
>>> isn't really vendor dependent. (If AMD/SVM gained a need for a similar
>>> helper, things would nee re-evaluating.)
>>
>> Maybe it would be better to add the calls to memory_type_changed()
>> directly in iomem_{permit,deny}_access() and
>> ioports_{permit,deny}_access itself?

I'm of two minds - on one hand that would nicely take the call "out of
sight", but otoh this would feel like a layering violation. Yet then
maybe it's a layering violation no matter where that call lives.

>> That would also allow to remove the noop Arm memory_type_changed()
>> halper.
> 
> Correction: the Arm memory_type_changed() needs to stay, as
> iomem_{permit,deny}_access() is common code.

Right, or we'd need some other arch abstraction. (I wonder whether
long term Arm can actually get away without this. Even on the AMD side
of x86 I don't think it's quite right that adding/removing of MMIO
ranges has no effect on the memory type of accesses.)

Jan