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

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

On Wed, Sep 28, 2022 at 12:45:13PM +0200, Jan Beulich wrote:
> On 28.09.2022 12:08, Roger Pau Monné wrote:
> > On Wed, Sep 28, 2022 at 10:01:26AM +0200, Jan Beulich wrote:
> >> On 27.09.2022 17:39, Roger Pau Monne wrote:
> >>> memory_type_changed() is currently only implemented for Intel EPT, and
> >>> results in the invalidation of EMT attributes on all the entries in
> >>> the EPT page tables.  Such invalidation causes EPT_MISCONFIG vmexits
> >>> when the guest tries to access any gfns for the first time, which
> >>> results in the recalculation of the EMT for the accessed page.  The
> >>> vmexit and the recalculations are expensive, and as such should be
> >>> avoided when possible.
> >>>
> >>> Remove the call to memory_type_changed() from
> >>> XEN_DOMCTL_memory_mapping: there are no modifications of the
> >>> iomem_caps ranges anymore that could alter the return of
> >>> cache_flush_permitted() from that domctl.
> >>>
> >>> Encapsulate calls to memory_type_changed() resulting from changes to
> >>> the domain iomem_caps or ioport_caps ranges in the helpers themselves
> >>> (io{ports,mem}_{permit,deny}_access()), and add a note in
> >>> epte_get_entry_emt() to remind that changes to the logic there likely
> >>> need to be propagaed to the IO capabilities helpers.
> >>>
> >>> Note changes to the IO ports or memory ranges are not very common
> >>> during guest runtime, but Citrix Hypervisor has an use case for them
> >>> related to device passthrough.
> >>>
> >>> Some Arm callers (implementations of the iomem_deny_access function
> >>> pointer field in gic_hw_operations struct) pass a const domain pointer
> >>> to iomem_deny_access(), which is questionable.  It works because
> >>> the rangeset is allocated separately from the domain struct, but
> >>> conceptually seems wrong to me, as passing a const pointer would imply
> >>> no changes to the domain data, and denying iomem accesses does change
> >>> the domain data.  Fix this by removing the const attribute from the
> >>> affected functions and call chain.
> >>
> >> Personally I think this adjustment would better be a separate, prereq
> >> change.
> > 
> > Right - I was about to split it but didn't want to go through the
> > hassle if the approach didn't end up being well received.
> > 
> > Do you think placing the calls to memory_type_changed() inside the
> > {permit,deny}_,access is acceptable?
> 
> Well, as said before - it's not pretty, but the existence of
> memory_type_changed() itself isn't either, nor are the present
> placements of calls to it. So yes, I view this as acceptable.
> 
> >>> --- a/xen/include/xen/iocap.h
> >>> +++ b/xen/include/xen/iocap.h
> >>> @@ -7,13 +7,43 @@
> >>>  #ifndef __XEN_IOCAP_H__
> >>>  #define __XEN_IOCAP_H__
> >>>  
> >>> +#include <xen/sched.h>
> >>>  #include <xen/rangeset.h>
> >>>  #include <asm/iocap.h>
> >>> +#include <asm/p2m.h>
> >>
> >> That's heavy dependencies you're adding. I wonder if the functions
> >> wouldn't better become out-of-line ones (but see also below).

I would expect most callers to already have those dependencies TBH,
and in any case definitions there not used would be dropped anyway.

Or are you worried about the newly added dependencies causing a
circular dependency issue in the future?

> >>> +static inline int iomem_permit_access(struct domain *d, unsigned long s,
> >>> +                                      unsigned long e)
> >>> +{
> >>> +    bool flush = cache_flush_permitted(d);
> >>> +    int ret = rangeset_add_range(d->iomem_caps, s, e);
> >>> +
> >>> +    if ( !ret && !is_iommu_enabled(d) && !flush )
> >>> +        /*
> >>> +         * Only flush if the range(s) are empty before this addition and
> >>> +         * IOMMU is not enabled for the domain, otherwise it makes no
> >>> +         * difference for effective cache attribute calculation purposes.
> >>> +         */
> >>> +        memory_type_changed(d);
> >>> +
> >>> +    return ret;
> >>> +}
> >>> +static inline int iomem_deny_access(struct domain *d, unsigned long s,
> >>> +                                    unsigned long e)
> >>> +{
> >>> +    int ret = rangeset_remove_range(d->iomem_caps, s, e);
> >>> +
> >>> +    if ( !ret && !is_iommu_enabled(d) && !cache_flush_permitted(d) )
> >>> +        /*
> >>> +         * Only flush if the range(s) are empty after this removal and
> >>> +         * IOMMU is not enabled for the domain, otherwise it makes no
> >>> +         * difference for effective cache attribute calculation purposes.
> >>> +         */
> >>> +        memory_type_changed(d);
> >>> +
> >>> +    return ret;
> >>> +}
> >>
> >> I'm surprised Arm's memory_type_changed() is an empty out-of-line function.
> >> This means the compiler can't eliminate this code (except when using LTO).
> >> But then cache_flush_permitted() (resolving to rangeset_is_empty()) can't
> >> be eliminated either, even if memory_type_changed() was. While gcc doc
> >> doesn't explicitly say that it may help (the talk about repeated 
> >> invocations
> >> only), I wonder whether we shouldn't mark rangeset_is_empty() pure. In a
> >> reduced example that does help (once memory_type_changed() is also an
> >> inline function) with gcc12 - no call to rangeset_is_empty() remains.
> > 
> > Can look into it, do you want it to be a prereq of this patch?
> 
> Well, if done, then it being a prereq would seem desirable. But x86 isn't
> affected by this, so I'd leave the "whether" aspect to be judged by Arm folks.

OK, let me split and prepare a new version then.

Thanks, Roger.