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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Sep 26, 2022 at 05:36:41PM +0200, Jan Beulich wrote:
> 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.

Kind of, I think it's slightly better than having the callers take
care of calling memory_type_changed(), and prevents new users of
{iomem,ioports}_{permit,deny}_access() missing the calls to
memory_type_changed().

Let me post what I have with this approach.

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

IIRC there's no way for the hypervisor to infer cache attributes on
AMD SVM for NPT entries, but maybe I'm missing something.  Guest MTRRs
settings are completely ignored for AMD guests.  I'm not able ATM
however to find in the AMD PM how effective cache attributes are
calculated when using NPT however.  I would guess host MTRR + guest
PAT?

Thanks, Roger.