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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan