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