Hi Han,
> On 29 Sep 2022, at 12:13, Jan Beulich <jbeul...@suse.com> wrote:
>
> On 28.09.2022 16:11, 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.
>>
>> Signed-off-by: Roger Pau Monné <roger....@citrix.com>
>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
> with one minor remark at the end, which can be taken care of while committing.
>
>> ---
>> Changes since v2:
>> - Split the Arm side changes into a pre-patch.
>
> Despite this I'd prefer to have an Arm maintainer view on this as well. As
> previously pointed out the resulting code is going to be sub-optimal there.
On arm none of those will be called at runtime, it happens only during guest
creation
so the potential performance impact is very reduce.
Cheers
Bertrand
>
>> --- 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>
>> +
>> +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,
>
> A blank line would be nice between these two (and similarly for the
> x86-only pair). Omitting such blank lines is imo advisable only for
> trivial inline functions.
>
> Jan
