[PATCH] x86/paging: Package up the log dirty function pointers

[PATCH] x86/paging: Package up the log dirty function pointers

[Andrew Cooper]

They depend soley on paging mode, so don't need to be repeated per domain, and
can live in .rodata.  While making this change, drop the redundant log_dirty
from the function pointer names.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/mm/hap/hap.c       | 10 +++++++---
 xen/arch/x86/mm/paging.c        | 23 ++++++++---------------
 xen/arch/x86/mm/shadow/common.c |  9 +++++++--
 xen/include/asm-x86/domain.h    |  8 +++++---
 xen/include/asm-x86/paging.h    |  6 +-----
 5 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index b5870bf..e7bad69 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -444,14 +444,18 @@ static void hap_destroy_monitor_table(struct vcpu* v, mfn_t mmfn)
 /************************************************/
 void hap_domain_init(struct domain *d)
 {
+    static const struct log_dirty_ops hap_ops = {
+        .enable  = hap_enable_log_dirty,
+        .disable = hap_disable_log_dirty,
+        .clean   = hap_clean_dirty_bitmap,
+    };
+
     INIT_PAGE_LIST_HEAD(&d->arch.paging.hap.freelist);
 
     d->arch.paging.gfn_bits = hap_paddr_bits - PAGE_SHIFT;
 
     /* Use HAP logdirty mechanism. */
-    paging_log_dirty_init(d, hap_enable_log_dirty,
-                          hap_disable_log_dirty,
-                          hap_clean_dirty_bitmap);
+    paging_log_dirty_init(d, &hap_ops);
 }
 
 /* return 0 for success, -errno for failure */
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index d964ed5..58d7f13 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -234,7 +234,7 @@ int paging_log_dirty_enable(struct domain *d, bool_t log_global)
         return -EINVAL;
 
     domain_pause(d);
-    ret = d->arch.paging.log_dirty.enable_log_dirty(d, log_global);
+    ret = d->arch.paging.log_dirty.ops->enable(d, log_global);
     domain_unpause(d);
 
     return ret;
@@ -250,7 +250,7 @@ static int paging_log_dirty_disable(struct domain *d, bool_t resuming)
         /* Safe because the domain is paused. */
         if ( paging_mode_log_dirty(d) )
         {
-            ret = d->arch.paging.log_dirty.disable_log_dirty(d);
+            ret = d->arch.paging.log_dirty.ops->disable(d);
             ASSERT(ret <= 0);
         }
     }
@@ -572,7 +572,7 @@ static int paging_log_dirty_op(struct domain *d,
     {
         /* We need to further call clean_dirty_bitmap() functions of specific
          * paging modes (shadow or hap).  Safe because the domain is paused. */
-        d->arch.paging.log_dirty.clean_dirty_bitmap(d);
+        d->arch.paging.log_dirty.ops->clean(d);
     }
     domain_unpause(d);
     return rv;
@@ -624,22 +624,15 @@ void paging_log_dirty_range(struct domain *d,
     flush_tlb_mask(d->domain_dirty_cpumask);
 }
 
-/* Note that this function takes three function pointers. Callers must supply
- * these functions for log dirty code to call. This function usually is
- * invoked when paging is enabled. Check shadow_enable() and hap_enable() for
- * reference.
+/* Callers must supply log_dirty_ops for the log dirty code to call. This
+ * function usually is invoked when paging is enabled. Check shadow_enable()
+ * and hap_enable() for reference.
  *
  * These function pointers must not be followed with the log-dirty lock held.
  */
-void paging_log_dirty_init(struct domain *d,
-                           int    (*enable_log_dirty)(struct domain *d,
-                                                      bool_t log_global),
-                           int    (*disable_log_dirty)(struct domain *d),
-                           void   (*clean_dirty_bitmap)(struct domain *d))
+void paging_log_dirty_init(struct domain *d, const struct log_dirty_ops *ops)
 {
-    d->arch.paging.log_dirty.enable_log_dirty = enable_log_dirty;
-    d->arch.paging.log_dirty.disable_log_dirty = disable_log_dirty;
-    d->arch.paging.log_dirty.clean_dirty_bitmap = clean_dirty_bitmap;
+    d->arch.paging.log_dirty.ops = ops;
 }
 
 /************************************************/
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 1c9d9b9..a18aa25 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -48,6 +48,12 @@ static void sh_clean_dirty_bitmap(struct domain *);
  * Called for every domain from arch_domain_create() */
 int shadow_domain_init(struct domain *d, unsigned int domcr_flags)
 {
+    static const struct log_dirty_ops sh_ops = {
+        .enable  = sh_enable_log_dirty,
+        .disable = sh_disable_log_dirty,
+        .clean   = sh_clean_dirty_bitmap,
+    };
+
     INIT_PAGE_LIST_HEAD(&d->arch.paging.shadow.freelist);
     INIT_PAGE_LIST_HEAD(&d->arch.paging.shadow.pinned_shadows);
 
@@ -62,8 +68,7 @@ int shadow_domain_init(struct domain *d, unsigned int domcr_flags)
 #endif
 
     /* Use shadow pagetables for log-dirty support */
-    paging_log_dirty_init(d, sh_enable_log_dirty,
-                          sh_disable_log_dirty, sh_clean_dirty_bitmap);
+    paging_log_dirty_init(d, &sh_ops);
 
 #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
     d->arch.paging.shadow.oos_active = 0;
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 9bb070f..f84f57b 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -175,9 +175,11 @@ struct log_dirty_domain {
     unsigned int   dirty_count;
 
     /* functions which are paging mode specific */
-    int            (*enable_log_dirty   )(struct domain *d, bool_t log_global);
-    int            (*disable_log_dirty  )(struct domain *d);
-    void           (*clean_dirty_bitmap )(struct domain *d);
+    const struct log_dirty_ops {
+        int        (*enable  )(struct domain *d, bool log_global);
+        int        (*disable )(struct domain *d);
+        void       (*clean   )(struct domain *d);
+    } *ops;
 };
 
 struct paging_domain {
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index cec6bfd..9ad74be 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -150,11 +150,7 @@ void paging_log_dirty_range(struct domain *d,
 int paging_log_dirty_enable(struct domain *d, bool_t log_global);
 
 /* log dirty initialization */
-void paging_log_dirty_init(struct domain *d,
-                           int  (*enable_log_dirty)(struct domain *d,
-                                                    bool_t log_global),
-                           int  (*disable_log_dirty)(struct domain *d),
-                           void (*clean_dirty_bitmap)(struct domain *d));
+void paging_log_dirty_init(struct domain *d, const struct log_dirty_ops *ops);
 
 /* mark a page as dirty */
 void paging_mark_dirty(struct domain *d, mfn_t gmfn);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/paging: Package up the log dirty function pointers

[Jan Beulich]

>>> On 16.02.17 at 18:13, <andrew.cooper3@citrix.com> wrote:
> They depend soley on paging mode, so don't need to be repeated per domain, and
> can live in .rodata.  While making this change, drop the redundant log_dirty
> from the function pointer names.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
but ...

> @@ -624,22 +624,15 @@ void paging_log_dirty_range(struct domain *d,
>      flush_tlb_mask(d->domain_dirty_cpumask);
>  }
>  
> -/* Note that this function takes three function pointers. Callers must supply
> - * these functions for log dirty code to call. This function usually is
> - * invoked when paging is enabled. Check shadow_enable() and hap_enable() for
> - * reference.
> +/* Callers must supply log_dirty_ops for the log dirty code to call. This
> + * function usually is invoked when paging is enabled. Check shadow_enable()
> + * and hap_enable() for reference.

... would you mind fixing the style of this comment as you're
touching it anyway?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/paging: Package up the log dirty function pointers

[Tim Deegan]

At 17:13 +0000 on 16 Feb (1487265184), Andrew Cooper wrote:
> They depend soley on paging mode, so don't need to be repeated per domain, and
> can live in .rodata.  While making this change, drop the redundant log_dirty
> from the function pointer names.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Tim Deegan <tim@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/paging: Package up the log dirty function pointers

[George Dunlap]

On 16/02/17 17:13, Andrew Cooper wrote:
> They depend soley on paging mode, so don't need to be repeated per domain, and
> can live in .rodata.  While making this change, drop the redundant log_dirty
> from the function pointer names.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

Sorry for the delay.



> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>  xen/arch/x86/mm/hap/hap.c       | 10 +++++++---
>  xen/arch/x86/mm/paging.c        | 23 ++++++++---------------
>  xen/arch/x86/mm/shadow/common.c |  9 +++++++--
>  xen/include/asm-x86/domain.h    |  8 +++++---
>  xen/include/asm-x86/paging.h    |  6 +-----
>  5 files changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index b5870bf..e7bad69 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -444,14 +444,18 @@ static void hap_destroy_monitor_table(struct vcpu* v, mfn_t mmfn)
>  /************************************************/
>  void hap_domain_init(struct domain *d)
>  {
> +    static const struct log_dirty_ops hap_ops = {
> +        .enable  = hap_enable_log_dirty,
> +        .disable = hap_disable_log_dirty,
> +        .clean   = hap_clean_dirty_bitmap,
> +    };
> +
>      INIT_PAGE_LIST_HEAD(&d->arch.paging.hap.freelist);
>  
>      d->arch.paging.gfn_bits = hap_paddr_bits - PAGE_SHIFT;
>  
>      /* Use HAP logdirty mechanism. */
> -    paging_log_dirty_init(d, hap_enable_log_dirty,
> -                          hap_disable_log_dirty,
> -                          hap_clean_dirty_bitmap);
> +    paging_log_dirty_init(d, &hap_ops);
>  }
>  
>  /* return 0 for success, -errno for failure */
> diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> index d964ed5..58d7f13 100644
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -234,7 +234,7 @@ int paging_log_dirty_enable(struct domain *d, bool_t log_global)
>          return -EINVAL;
>  
>      domain_pause(d);
> -    ret = d->arch.paging.log_dirty.enable_log_dirty(d, log_global);
> +    ret = d->arch.paging.log_dirty.ops->enable(d, log_global);
>      domain_unpause(d);
>  
>      return ret;
> @@ -250,7 +250,7 @@ static int paging_log_dirty_disable(struct domain *d, bool_t resuming)
>          /* Safe because the domain is paused. */
>          if ( paging_mode_log_dirty(d) )
>          {
> -            ret = d->arch.paging.log_dirty.disable_log_dirty(d);
> +            ret = d->arch.paging.log_dirty.ops->disable(d);
>              ASSERT(ret <= 0);
>          }
>      }
> @@ -572,7 +572,7 @@ static int paging_log_dirty_op(struct domain *d,
>      {
>          /* We need to further call clean_dirty_bitmap() functions of specific
>           * paging modes (shadow or hap).  Safe because the domain is paused. */
> -        d->arch.paging.log_dirty.clean_dirty_bitmap(d);
> +        d->arch.paging.log_dirty.ops->clean(d);
>      }
>      domain_unpause(d);
>      return rv;
> @@ -624,22 +624,15 @@ void paging_log_dirty_range(struct domain *d,
>      flush_tlb_mask(d->domain_dirty_cpumask);
>  }
>  
> -/* Note that this function takes three function pointers. Callers must supply
> - * these functions for log dirty code to call. This function usually is
> - * invoked when paging is enabled. Check shadow_enable() and hap_enable() for
> - * reference.
> +/* Callers must supply log_dirty_ops for the log dirty code to call. This
> + * function usually is invoked when paging is enabled. Check shadow_enable()
> + * and hap_enable() for reference.
>   *
>   * These function pointers must not be followed with the log-dirty lock held.
>   */
> -void paging_log_dirty_init(struct domain *d,
> -                           int    (*enable_log_dirty)(struct domain *d,
> -                                                      bool_t log_global),
> -                           int    (*disable_log_dirty)(struct domain *d),
> -                           void   (*clean_dirty_bitmap)(struct domain *d))
> +void paging_log_dirty_init(struct domain *d, const struct log_dirty_ops *ops)
>  {
> -    d->arch.paging.log_dirty.enable_log_dirty = enable_log_dirty;
> -    d->arch.paging.log_dirty.disable_log_dirty = disable_log_dirty;
> -    d->arch.paging.log_dirty.clean_dirty_bitmap = clean_dirty_bitmap;
> +    d->arch.paging.log_dirty.ops = ops;
>  }
>  
>  /************************************************/
> diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
> index 1c9d9b9..a18aa25 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -48,6 +48,12 @@ static void sh_clean_dirty_bitmap(struct domain *);
>   * Called for every domain from arch_domain_create() */
>  int shadow_domain_init(struct domain *d, unsigned int domcr_flags)
>  {
> +    static const struct log_dirty_ops sh_ops = {
> +        .enable  = sh_enable_log_dirty,
> +        .disable = sh_disable_log_dirty,
> +        .clean   = sh_clean_dirty_bitmap,
> +    };
> +
>      INIT_PAGE_LIST_HEAD(&d->arch.paging.shadow.freelist);
>      INIT_PAGE_LIST_HEAD(&d->arch.paging.shadow.pinned_shadows);
>  
> @@ -62,8 +68,7 @@ int shadow_domain_init(struct domain *d, unsigned int domcr_flags)
>  #endif
>  
>      /* Use shadow pagetables for log-dirty support */
> -    paging_log_dirty_init(d, sh_enable_log_dirty,
> -                          sh_disable_log_dirty, sh_clean_dirty_bitmap);
> +    paging_log_dirty_init(d, &sh_ops);
>  
>  #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
>      d->arch.paging.shadow.oos_active = 0;
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 9bb070f..f84f57b 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -175,9 +175,11 @@ struct log_dirty_domain {
>      unsigned int   dirty_count;
>  
>      /* functions which are paging mode specific */
> -    int            (*enable_log_dirty   )(struct domain *d, bool_t log_global);
> -    int            (*disable_log_dirty  )(struct domain *d);
> -    void           (*clean_dirty_bitmap )(struct domain *d);
> +    const struct log_dirty_ops {
> +        int        (*enable  )(struct domain *d, bool log_global);
> +        int        (*disable )(struct domain *d);
> +        void       (*clean   )(struct domain *d);
> +    } *ops;
>  };
>  
>  struct paging_domain {
> diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
> index cec6bfd..9ad74be 100644
> --- a/xen/include/asm-x86/paging.h
> +++ b/xen/include/asm-x86/paging.h
> @@ -150,11 +150,7 @@ void paging_log_dirty_range(struct domain *d,
>  int paging_log_dirty_enable(struct domain *d, bool_t log_global);
>  
>  /* log dirty initialization */
> -void paging_log_dirty_init(struct domain *d,
> -                           int  (*enable_log_dirty)(struct domain *d,
> -                                                    bool_t log_global),
> -                           int  (*disable_log_dirty)(struct domain *d),
> -                           void (*clean_dirty_bitmap)(struct domain *d));
> +void paging_log_dirty_init(struct domain *d, const struct log_dirty_ops *ops);
>  
>  /* mark a page as dirty */
>  void paging_mark_dirty(struct domain *d, mfn_t gmfn);
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel