[PATCH v3 0/2] Move calls to memory_type_changed()

[PATCH v3 0/2] Move calls to memory_type_changed()

[Roger Pau Monne]

Hello,

The current calls to memory_type_changed() are wider than strictly
necessary.  Move them inside the iocap handlers and also limit to only
issue them when required.

I would really like to get some feedback on the Arm change, since this
is now a prereq for the actual fix.

Thanks, Roger.

Roger Pau Monne (2):
  arm/vgic: drop const attribute from gic_iomem_deny_access()
  x86/ept: limit calls to memory_type_changed()

 xen/arch/arm/gic-v2.c            |  2 +-
 xen/arch/arm/gic-v3.c            |  2 +-
 xen/arch/arm/gic.c               |  2 +-
 xen/arch/arm/include/asm/gic.h   |  4 ++--
 xen/arch/x86/domctl.c            |  4 ----
 xen/arch/x86/include/asm/iocap.h | 33 +++++++++++++++++++++++----
 xen/arch/x86/mm/p2m-ept.c        |  4 ++++
 xen/common/domctl.c              |  4 ----
 xen/include/xen/iocap.h          | 38 ++++++++++++++++++++++++++++----
 9 files changed, 72 insertions(+), 21 deletions(-)

-- 
2.37.3

[PATCH v3 1/2] arm/vgic: drop const attribute from gic_iomem_deny_access()

[Roger Pau Monne]

While correct from a code point of view, the usage of the const
attribute for the domain parameter of gic_iomem_deny_access() is at
least partially bogus.  Contents of the domain structure (the iomem
rangeset) is modified by the function.  Such modifications succeed
because right now the iomem rangeset is allocated separately from
struct domain, and hence is not subject to the constness of struct
domain.

Amend this by dropping the const attribute from the function
parameter.

This is required by further changes that will convert
iomem_{permit,deny}_access into a function.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/arm/gic-v2.c          | 2 +-
 xen/arch/arm/gic-v3.c          | 2 +-
 xen/arch/arm/gic.c             | 2 +-
 xen/arch/arm/include/asm/gic.h | 4 ++--
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index bd773bcc67..ae5bd8e95f 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1083,7 +1083,7 @@ static void __init gicv2_dt_init(void)
     gicv2_extension_dt_init(node);
 }
 
-static int gicv2_iomem_deny_access(const struct domain *d)
+static int gicv2_iomem_deny_access(struct domain *d)
 {
     int rc;
     unsigned long mfn, nr;
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 64b36cec25..018fa0dfa0 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1424,7 +1424,7 @@ static void __init gicv3_dt_init(void)
                               &vbase, &vsize);
 }
 
-static int gicv3_iomem_deny_access(const struct domain *d)
+static int gicv3_iomem_deny_access(struct domain *d)
 {
     int rc, i;
     unsigned long mfn, nr;
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 3b0331b538..9b82325442 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -462,7 +462,7 @@ unsigned long gic_get_hwdom_madt_size(const struct domain *d)
 }
 #endif
 
-int gic_iomem_deny_access(const struct domain *d)
+int gic_iomem_deny_access(struct domain *d)
 {
     return gic_hw_ops->iomem_deny_access(d);
 }
diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
index 3692fae393..76e3fa5dc4 100644
--- a/xen/arch/arm/include/asm/gic.h
+++ b/xen/arch/arm/include/asm/gic.h
@@ -392,7 +392,7 @@ struct gic_hw_operations {
     /* Map extra GIC MMIO, irqs and other hw stuffs to the hardware domain. */
     int (*map_hwdom_extra_mappings)(struct domain *d);
     /* Deny access to GIC regions */
-    int (*iomem_deny_access)(const struct domain *d);
+    int (*iomem_deny_access)(struct domain *d);
     /* Handle LPIs, which require special handling */
     void (*do_LPI)(unsigned int lpi);
 };
@@ -449,7 +449,7 @@ unsigned long gic_get_hwdom_madt_size(const struct domain *d);
 #endif
 
 int gic_map_hwdom_extra_mappings(struct domain *d);
-int gic_iomem_deny_access(const struct domain *d);
+int gic_iomem_deny_access(struct domain *d);
 
 #endif /* __ASSEMBLY__ */
 #endif
-- 
2.37.3

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

[Roger Pau Monne]

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.pau@citrix.com>
---
Changes since v2:
 - Split the Arm side changes into a pre-patch.

Changes since v1:
 - Place the calls to memory_type_changed() inside the
   io{ports,mem}_{permit,deny}_access() helpers.
---
 xen/arch/x86/domctl.c            |  4 ----
 xen/arch/x86/include/asm/iocap.h | 33 +++++++++++++++++++++++----
 xen/arch/x86/mm/p2m-ept.c        |  4 ++++
 xen/common/domctl.c              |  4 ----
 xen/include/xen/iocap.h          | 38 ++++++++++++++++++++++++++++----
 5 files changed, 67 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 020df615bd..e9bfbc57a7 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -232,8 +232,6 @@ long arch_do_domctl(
             ret = ioports_permit_access(d, fp, fp + np - 1);
         else
             ret = ioports_deny_access(d, fp, fp + np - 1);
-        if ( !ret )
-            memory_type_changed(d);
         break;
     }
 
@@ -666,8 +664,6 @@ long arch_do_domctl(
                        "ioport_map: error %ld denying dom%d access to [%x,%x]\n",
                        ret, d->domain_id, fmp, fmp + np - 1);
         }
-        if ( !ret )
-            memory_type_changed(d);
         break;
     }
 
diff --git a/xen/arch/x86/include/asm/iocap.h b/xen/arch/x86/include/asm/iocap.h
index eee47228d4..ce83c3d8a4 100644
--- a/xen/arch/x86/include/asm/iocap.h
+++ b/xen/arch/x86/include/asm/iocap.h
@@ -7,10 +7,11 @@
 #ifndef __X86_IOCAP_H__
 #define __X86_IOCAP_H__
 
-#define ioports_permit_access(d, s, e)                  \
-    rangeset_add_range((d)->arch.ioport_caps, s, e)
-#define ioports_deny_access(d, s, e)                    \
-    rangeset_remove_range((d)->arch.ioport_caps, s, e)
+#include <xen/sched.h>
+#include <xen/rangeset.h>
+
+#include <asm/p2m.h>
+
 #define ioports_access_permitted(d, s, e)               \
     rangeset_contains_range((d)->arch.ioport_caps, s, e)
 
@@ -18,4 +19,28 @@
     (!rangeset_is_empty((d)->iomem_caps) ||             \
      !rangeset_is_empty((d)->arch.ioport_caps))
 
+static inline int ioports_permit_access(struct domain *d, unsigned long s,
+                                        unsigned long e)
+{
+    bool flush = cache_flush_permitted(d);
+    int ret = rangeset_add_range(d->arch.ioport_caps, s, e);
+
+    if ( !ret && !is_iommu_enabled(d) && !flush )
+        /* See comment in iomem_permit_access(). */
+        memory_type_changed(d);
+
+    return ret;
+}
+static inline int ioports_deny_access(struct domain *d, unsigned long s,
+                                      unsigned long e)
+{
+    int ret = rangeset_remove_range(d->arch.ioport_caps, s, e);
+
+    if ( !ret && !is_iommu_enabled(d) && !cache_flush_permitted(d) )
+        /* See comment in iomem_deny_access(). */
+        memory_type_changed(d);
+
+    return ret;
+}
+
 #endif /* __X86_IOCAP_H__ */
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index b4919bad51..d61d66c20e 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -518,6 +518,10 @@ int epte_get_entry_emt(struct domain *d, gfn_t gfn, mfn_t mfn,
         return MTRR_TYPE_UNCACHABLE;
     }
 
+    /*
+     * Conditional must be kept in sync with the code in
+     * {iomem,ioports}_{permit,deny}_access().
+     */
     if ( type != p2m_mmio_direct && !is_iommu_enabled(d) &&
          !cache_flush_permitted(d) )
     {
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 452266710a..69fb9abd34 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -716,8 +716,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
         else
             ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
-        if ( !ret )
-            memory_type_changed(d);
         break;
     }
 
@@ -778,8 +776,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
                        "memory_map: error %ld removing dom%d access to [%lx,%lx]\n",
                        ret, d->domain_id, mfn, mfn_end);
         }
-        /* Do this unconditionally to cover errors on above failure paths. */
-        memory_type_changed(d);
         break;
     }
 
diff --git a/xen/include/xen/iocap.h b/xen/include/xen/iocap.h
index 1ca3858fc0..0ca4c9745f 100644
--- 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,
+                                    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;
+}
 
-#define iomem_permit_access(d, s, e)                    \
-    rangeset_add_range((d)->iomem_caps, s, e)
-#define iomem_deny_access(d, s, e)                      \
-    rangeset_remove_range((d)->iomem_caps, s, e)
 #define iomem_access_permitted(d, s, e)                 \
     rangeset_contains_range((d)->iomem_caps, s, e)
 
-- 
2.37.3

Re: [PATCH v3 1/2] arm/vgic: drop const attribute from gic_iomem_deny_access()

[Bertrand Marquis]

Hi Roger,

> On 28 Sep 2022, at 16:11, Roger Pau Monne <roger.pau@citrix.com> wrote:
> 
> While correct from a code point of view, the usage of the const
> attribute for the domain parameter of gic_iomem_deny_access() is at
> least partially bogus.  Contents of the domain structure (the iomem
> rangeset) is modified by the function.  Such modifications succeed
> because right now the iomem rangeset is allocated separately from
> struct domain, and hence is not subject to the constness of struct
> domain.
> 
> Amend this by dropping the const attribute from the function
> parameter.
> 
> This is required by further changes that will convert
> iomem_{permit,deny}_access into a function.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/arch/arm/gic-v2.c          | 2 +-
> xen/arch/arm/gic-v3.c          | 2 +-
> xen/arch/arm/gic.c             | 2 +-
> xen/arch/arm/include/asm/gic.h | 4 ++--
> 4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index bd773bcc67..ae5bd8e95f 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1083,7 +1083,7 @@ static void __init gicv2_dt_init(void)
>     gicv2_extension_dt_init(node);
> }
> 
> -static int gicv2_iomem_deny_access(const struct domain *d)
> +static int gicv2_iomem_deny_access(struct domain *d)
> {
>     int rc;
>     unsigned long mfn, nr;
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 64b36cec25..018fa0dfa0 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1424,7 +1424,7 @@ static void __init gicv3_dt_init(void)
>                               &vbase, &vsize);
> }
> 
> -static int gicv3_iomem_deny_access(const struct domain *d)
> +static int gicv3_iomem_deny_access(struct domain *d)
> {
>     int rc, i;
>     unsigned long mfn, nr;
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 3b0331b538..9b82325442 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -462,7 +462,7 @@ unsigned long gic_get_hwdom_madt_size(const struct domain *d)
> }
> #endif
> 
> -int gic_iomem_deny_access(const struct domain *d)
> +int gic_iomem_deny_access(struct domain *d)
> {
>     return gic_hw_ops->iomem_deny_access(d);
> }
> diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
> index 3692fae393..76e3fa5dc4 100644
> --- a/xen/arch/arm/include/asm/gic.h
> +++ b/xen/arch/arm/include/asm/gic.h
> @@ -392,7 +392,7 @@ struct gic_hw_operations {
>     /* Map extra GIC MMIO, irqs and other hw stuffs to the hardware domain. */
>     int (*map_hwdom_extra_mappings)(struct domain *d);
>     /* Deny access to GIC regions */
> -    int (*iomem_deny_access)(const struct domain *d);
> +    int (*iomem_deny_access)(struct domain *d);
>     /* Handle LPIs, which require special handling */
>     void (*do_LPI)(unsigned int lpi);
> };
> @@ -449,7 +449,7 @@ unsigned long gic_get_hwdom_madt_size(const struct domain *d);
> #endif
> 
> int gic_map_hwdom_extra_mappings(struct domain *d);
> -int gic_iomem_deny_access(const struct domain *d);
> +int gic_iomem_deny_access(struct domain *d);
> 
> #endif /* __ASSEMBLY__ */
> #endif
> -- 
> 2.37.3
> 

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

[Jan Beulich]

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.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@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.

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

Re: [PATCH v3 0/2] Move calls to memory_type_changed()

[Jan Beulich]

On 28.09.2022 16:11, Roger Pau Monne wrote:
> The current calls to memory_type_changed() are wider than strictly
> necessary.  Move them inside the iocap handlers and also limit to only
> issue them when required.
> 
> I would really like to get some feedback on the Arm change, since this
> is now a prereq for the actual fix.
> 
> Thanks, Roger.
> 
> Roger Pau Monne (2):
>   arm/vgic: drop const attribute from gic_iomem_deny_access()
>   x86/ept: limit calls to memory_type_changed()

Are there intentions for having these on 4.17?

Jan

For 4.17 (was: Re: [PATCH v3 0/2] Move calls to memory_type_changed())

[Roger Pau Monné]

On Thu, Sep 29, 2022 at 12:14:10PM +0200, Jan Beulich wrote:
> On 28.09.2022 16:11, Roger Pau Monne wrote:
> > The current calls to memory_type_changed() are wider than strictly
> > necessary.  Move them inside the iocap handlers and also limit to only
> > issue them when required.
> > 
> > I would really like to get some feedback on the Arm change, since this
> > is now a prereq for the actual fix.
> > 
> > Thanks, Roger.
> > 
> > Roger Pau Monne (2):
> >   arm/vgic: drop const attribute from gic_iomem_deny_access()
> >   x86/ept: limit calls to memory_type_changed()
> 
> Are there intentions for having these on 4.17?

I wasn't sure.  From XenServer PoV it's certainly a bug fix,
otherwise some workloads related to GPU passthrough are simply too
slow to be usable.

I would certainly be fine with it making it's way into 4.17, let me
add Henry:

Cons:
 - Changes the number of issued memory_type_changed(), so there's a
   risk I misplaced some of the conditions and we end up with wrong
   cache types in the guest p2m due to missing memory_type_changed()
   calls.  That however won't affect Xen itself, just the guest.

Pros:
 - Removes unneeded memory_type_changed(), thus making some operations
   faster.  It's effect it's greatly dependent on using a set of
   hypercalls against a domain, which doesn't seem common in upstream.
   It's possible other products based on Xen apart from XenServer will
   also see an speedup as a result.

Thanks, Roger.

RE: For 4.17 (was: Re: [PATCH v3 0/2] Move calls to memory_type_changed())

[Henry Wang]

Hi Roger and Jan,

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Subject: For 4.17 (was: Re: [PATCH v3 0/2] Move calls to
> memory_type_changed())
> 
> On Thu, Sep 29, 2022 at 12:14:10PM +0200, Jan Beulich wrote:
> > On 28.09.2022 16:11, Roger Pau Monne wrote:
> > > The current calls to memory_type_changed() are wider than strictly
> > > necessary.  Move them inside the iocap handlers and also limit to only
> > > issue them when required.
> > >
> > > I would really like to get some feedback on the Arm change, since this
> > > is now a prereq for the actual fix.
> > >
> > > Thanks, Roger.
> > >
> > > Roger Pau Monne (2):
> > >   arm/vgic: drop const attribute from gic_iomem_deny_access()
> > >   x86/ept: limit calls to memory_type_changed()
> >
> > Are there intentions for having these on 4.17?
> 
> I wasn't sure.  From XenServer PoV it's certainly a bug fix,
> otherwise some workloads related to GPU passthrough are simply too
> slow to be usable.
> 
> I would certainly be fine with it making it's way into 4.17, let me
> add Henry:
> 
> Cons:
>  - Changes the number of issued memory_type_changed(), so there's a
>    risk I misplaced some of the conditions and we end up with wrong
>    cache types in the guest p2m due to missing memory_type_changed()
>    calls.  That however won't affect Xen itself, just the guest.
> 
> Pros:
>  - Removes unneeded memory_type_changed(), thus making some
> operations
>    faster.  It's effect it's greatly dependent on using a set of
>    hypercalls against a domain, which doesn't seem common in upstream.
>    It's possible other products based on Xen apart from XenServer will
>    also see an speedup as a result.

Thanks for the information and the detailed summary!

I think my understanding is the same as what Jan has in 
"x86/NUMA: correct memnode_shift calculation for single node system",
- we are still not in code freeze but in feature freeze, so properly-reviewed
fixes are eligible for the release. For this specific series, (to me) it looks ok
and I will not block the merging of this series if maintainers want to merge
it :))

Kind regards,
Henry

> 
> Thanks, Roger.

Re: For 4.17 (was: Re: [PATCH v3 0/2] Move calls to memory_type_changed())

[Jan Beulich]

On 29.09.2022 13:10, Henry Wang wrote:
>> -----Original Message-----
>> From: Roger Pau Monné <roger.pau@citrix.com>
>>
>> On Thu, Sep 29, 2022 at 12:14:10PM +0200, Jan Beulich wrote:
>>> On 28.09.2022 16:11, Roger Pau Monne wrote:
>>>> The current calls to memory_type_changed() are wider than strictly
>>>> necessary.  Move them inside the iocap handlers and also limit to only
>>>> issue them when required.
>>>>
>>>> I would really like to get some feedback on the Arm change, since this
>>>> is now a prereq for the actual fix.
>>>>
>>>> Thanks, Roger.
>>>>
>>>> Roger Pau Monne (2):
>>>>   arm/vgic: drop const attribute from gic_iomem_deny_access()
>>>>   x86/ept: limit calls to memory_type_changed()
>>>
>>> Are there intentions for having these on 4.17?
>>
>> I wasn't sure.  From XenServer PoV it's certainly a bug fix,
>> otherwise some workloads related to GPU passthrough are simply too
>> slow to be usable.
>>
>> I would certainly be fine with it making it's way into 4.17, let me
>> add Henry:
>>
>> Cons:
>>  - Changes the number of issued memory_type_changed(), so there's a
>>    risk I misplaced some of the conditions and we end up with wrong
>>    cache types in the guest p2m due to missing memory_type_changed()
>>    calls.  That however won't affect Xen itself, just the guest.
>>
>> Pros:
>>  - Removes unneeded memory_type_changed(), thus making some
>> operations
>>    faster.  It's effect it's greatly dependent on using a set of
>>    hypercalls against a domain, which doesn't seem common in upstream.
>>    It's possible other products based on Xen apart from XenServer will
>>    also see an speedup as a result.
> 
> Thanks for the information and the detailed summary!
> 
> I think my understanding is the same as what Jan has in 
> "x86/NUMA: correct memnode_shift calculation for single node system",
> - we are still not in code freeze but in feature freeze, so properly-reviewed
> fixes are eligible for the release. For this specific series, (to me) it looks ok
> and I will not block the merging of this series if maintainers want to merge
> it :))

Thanks. Then, according to my reply to patch 2, the only open thing is
to have at least informal agreement there by (at least) one of the Arm
maintainers. There's no guarantee this will arrive by tomorrow.

Jan

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

[Bertrand Marquis]

Hi Han,

> On 29 Sep 2022, at 12:13, Jan Beulich <jbeulich@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.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@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