[PATCH RFC 0/2] Misc paging code clean up

[PATCH RFC 0/2] Misc paging code clean up

[Wei Liu]

Wei Liu (2):
  xen: remove CONFIG_PAGING_ASSISTANCE
  x86/mm: PG_translate implies PG_refcounts

 xen/arch/x86/domain.c        |  6 ++----
 xen/arch/x86/hvm/hvm.c       |  2 +-
 xen/arch/x86/mm.c            | 20 ++++++++++----------
 xen/arch/x86/mm/paging.c     |  8 +++-----
 xen/include/asm-arm/config.h |  2 --
 xen/include/asm-x86/config.h |  1 -
 xen/include/asm-x86/paging.h |  5 +----
 xen/include/asm-x86/shadow.h |  2 +-
 xen/include/xen/paging.h     | 19 -------------------
 9 files changed, 18 insertions(+), 47 deletions(-)

-- 
2.11.0


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

[PATCH RFC 1/2] xen: remove CONFIG_PAGING_ASSISTANCE

[Wei Liu]

Arm should always set it, while on x86 xen can't build with it set to
0, which means people haven't used it for years.

Remove it and simplify xen/paging.h.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/config.h |  2 --
 xen/include/asm-x86/config.h |  1 -
 xen/include/xen/paging.h     | 19 -------------------
 3 files changed, 22 deletions(-)

diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 7da94698e1..210c9f858b 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -24,8 +24,6 @@
 /* xen_ulong_t is always 64 bits */
 #define BITS_PER_XEN_ULONG 64
 
-#define CONFIG_PAGING_ASSISTANCE 1
-
 #define CONFIG_PAGING_LEVELS 3
 
 #define CONFIG_ARM 1
diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
index 0130ac864f..f63a9cb952 100644
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -19,7 +19,6 @@
 
 #define BITS_PER_XEN_ULONG BITS_PER_LONG
 
-#define CONFIG_PAGING_ASSISTANCE 1
 #define CONFIG_X86_PM_TIMER 1
 #define CONFIG_HPET_TIMER 1
 #define CONFIG_X86_MCE_THERMAL 1
diff --git a/xen/include/xen/paging.h b/xen/include/xen/paging.h
index 214bde5576..a126c9bf58 100644
--- a/xen/include/xen/paging.h
+++ b/xen/include/xen/paging.h
@@ -1,26 +1,7 @@
-
 #ifndef __XEN_PAGING_H__
 #define __XEN_PAGING_H__
 
-#if defined CONFIG_PAGING_ASSISTANCE
-
 #include <asm/paging.h>
 #include <asm/p2m.h>
 
-#elif defined CONFIG_SHADOW_PAGING
-
-#include <asm/shadow.h>
-
-#define paging_mode_translate(d)  shadow_mode_translate(d)
-#define paging_mode_external(d)   (0)
-
-#else
-
-#define paging_mode_translate(d)              (0)
-#define paging_mode_external(d)               (0)
-#define guest_physmap_add_page(d, p, m, o)    ((void)0)
-#define guest_physmap_remove_page(d, p, m, o) ((void)0)
-
-#endif
-
 #endif /* __XEN_PAGING_H__ */
-- 
2.11.0


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

Re: [PATCH RFC 1/2] xen: remove CONFIG_PAGING_ASSISTANCE

[Jan Beulich]

>>> On 23.08.17 at 17:58, <wei.liu2@citrix.com> wrote:
> Arm should always set it, while on x86 xen can't build with it set to
> 0, which means people haven't used it for years.
> 
> Remove it and simplify xen/paging.h.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

This is something I certainly would want Tim to see (and perhaps
approve).

Jan

> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
>  xen/include/asm-arm/config.h |  2 --
>  xen/include/asm-x86/config.h |  1 -
>  xen/include/xen/paging.h     | 19 -------------------
>  3 files changed, 22 deletions(-)
> 
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index 7da94698e1..210c9f858b 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -24,8 +24,6 @@
>  /* xen_ulong_t is always 64 bits */
>  #define BITS_PER_XEN_ULONG 64
>  
> -#define CONFIG_PAGING_ASSISTANCE 1
> -
>  #define CONFIG_PAGING_LEVELS 3
>  
>  #define CONFIG_ARM 1
> diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
> index 0130ac864f..f63a9cb952 100644
> --- a/xen/include/asm-x86/config.h
> +++ b/xen/include/asm-x86/config.h
> @@ -19,7 +19,6 @@
>  
>  #define BITS_PER_XEN_ULONG BITS_PER_LONG
>  
> -#define CONFIG_PAGING_ASSISTANCE 1
>  #define CONFIG_X86_PM_TIMER 1
>  #define CONFIG_HPET_TIMER 1
>  #define CONFIG_X86_MCE_THERMAL 1
> diff --git a/xen/include/xen/paging.h b/xen/include/xen/paging.h
> index 214bde5576..a126c9bf58 100644
> --- a/xen/include/xen/paging.h
> +++ b/xen/include/xen/paging.h
> @@ -1,26 +1,7 @@
> -
>  #ifndef __XEN_PAGING_H__
>  #define __XEN_PAGING_H__
>  
> -#if defined CONFIG_PAGING_ASSISTANCE
> -
>  #include <asm/paging.h>
>  #include <asm/p2m.h>
>  
> -#elif defined CONFIG_SHADOW_PAGING
> -
> -#include <asm/shadow.h>
> -
> -#define paging_mode_translate(d)  shadow_mode_translate(d)
> -#define paging_mode_external(d)   (0)
> -
> -#else
> -
> -#define paging_mode_translate(d)              (0)
> -#define paging_mode_external(d)               (0)
> -#define guest_physmap_add_page(d, p, m, o)    ((void)0)
> -#define guest_physmap_remove_page(d, p, m, o) ((void)0)
> -
> -#endif
> -
>  #endif /* __XEN_PAGING_H__ */
> -- 
> 2.11.0




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

Re: [PATCH RFC 1/2] xen: remove CONFIG_PAGING_ASSISTANCE

[Tim Deegan]

At 10:04 -0600 on 23 Aug (1503482672), Jan Beulich wrote:
> >>> On 23.08.17 at 17:58, <wei.liu2@citrix.com> wrote:
> > Arm should always set it, while on x86 xen can't build with it set to
> > 0, which means people haven't used it for years.
> > 
> > Remove it and simplify xen/paging.h.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> This is something I certainly would want Tim to see (and perhaps
> approve).

Thanks.  I see and approve. :)

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

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

Re: [PATCH RFC 1/2] xen: remove CONFIG_PAGING_ASSISTANCE

[Jan Beulich]

>>> On 24.08.17 at 12:04, <tim@xen.org> wrote:
> At 10:04 -0600 on 23 Aug (1503482672), Jan Beulich wrote:
>> >>> On 23.08.17 at 17:58, <wei.liu2@citrix.com> wrote:
>> > Arm should always set it, while on x86 xen can't build with it set to
>> > 0, which means people haven't used it for years.
>> > 
>> > Remove it and simplify xen/paging.h.
>> > 
>> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> 
>> This is something I certainly would want Tim to see (and perhaps
>> approve).
> 
> Thanks.  I see and approve. :)
> 
> Acked-by: Tim Deegan <tim@xen.org>

Thanks, in which case both patches can have my ack too (in case
it's still needed at all).

Jan


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

Re: [PATCH RFC 1/2] xen: remove CONFIG_PAGING_ASSISTANCE

[Julien Grall]

Hi,

On 23/08/17 16:58, Wei Liu wrote:
> Arm should always set it, while on x86 xen can't build with it set to
> 0, which means people haven't used it for years.
>
> Remove it and simplify xen/paging.h.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Arm bits:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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

[PATCH RFC 2/2] x86/mm: PG_translate implies PG_refcounts

[Wei Liu]

After 404595352 ("x86/paging: Enforce PG_external == PG_translate ==
PG_refcounts"), PG_refcounts and PG_translate always need to be set
together.

Squash PG_refcounts to simplify code. All calls paging_mode_refcounts
are replaced by calls to paging_mode_translate.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/domain.c        |  6 ++----
 xen/arch/x86/hvm/hvm.c       |  2 +-
 xen/arch/x86/mm.c            | 20 ++++++++++----------
 xen/arch/x86/mm/paging.c     |  8 +++-----
 xen/include/asm-x86/paging.h |  5 +----
 xen/include/asm-x86/shadow.h |  2 +-
 6 files changed, 18 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9b4b9596d8..bbe545b165 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1017,8 +1017,6 @@ int arch_set_info_guest(
 
     if ( !cr3_page )
         rc = -EINVAL;
-    else if ( paging_mode_refcounts(d) )
-        /* nothing */;
     else if ( cr3_page == v->arch.old_guest_table )
     {
         v->arch.old_guest_table = NULL;
@@ -1040,7 +1038,7 @@ int arch_set_info_guest(
             break;
         case 0:
             if ( !compat && !VM_ASSIST(d, m2p_strict) &&
-                 !paging_mode_refcounts(d) )
+                 !paging_mode_translate(d) )
                 fill_ro_mpt(cr3_gfn);
             break;
         default:
@@ -1061,7 +1059,7 @@ int arch_set_info_guest(
 
             if ( !cr3_page )
                 rc = -EINVAL;
-            else if ( !paging_mode_refcounts(d) )
+            else if ( !paging_mode_translate(d) )
             {
                 rc = get_page_type_preemptible(cr3_page, PGT_root_page_table);
                 switch ( rc )
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 6cb903def5..eee0ff422e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -589,7 +589,7 @@ int hvm_domain_initialise(struct domain *d, unsigned long domcr_flags,
 
     hvm_init_cacheattr_region_list(d);
 
-    rc = paging_enable(d, PG_refcounts|PG_translate|PG_external);
+    rc = paging_enable(d, PG_translate|PG_external);
     if ( rc != 0 )
         goto fail0;
 
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index ed77270586..8adb7b6649 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1925,7 +1925,7 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e,
     if ( unlikely(__copy_from_user(&ol1e, pl1e, sizeof(ol1e)) != 0) )
         return -EFAULT;
 
-    ASSERT(!paging_mode_refcounts(pt_dom));
+    ASSERT(!paging_mode_translate(pt_dom));
 
     if ( l1e_get_flags(nl1e) & _PAGE_PRESENT )
     {
@@ -2257,7 +2257,7 @@ int get_page(struct page_info *page, struct domain *domain)
     if ( likely(owner == domain) )
         return 1;
 
-    if ( !paging_mode_refcounts(domain) && !domain->is_dying )
+    if ( !paging_mode_translate(domain) && !domain->is_dying )
         gprintk(XENLOG_INFO,
                 "Error mfn %"PRI_mfn": rd=%d od=%d caf=%08lx taf=%" PRtype_info "\n",
                 mfn_x(page_to_mfn(page)), domain->domain_id,
@@ -2719,7 +2719,7 @@ int vcpu_destroy_pagetables(struct vcpu *v)
     if ( mfn )
     {
         page = mfn_to_page(_mfn(mfn));
-        if ( paging_mode_refcounts(v->domain) )
+        if ( paging_mode_translate(v->domain) )
             put_page(page);
         else
             rc = put_page_and_type_preemptible(page);
@@ -2740,7 +2740,7 @@ int vcpu_destroy_pagetables(struct vcpu *v)
         if ( mfn )
         {
             page = mfn_to_page(_mfn(mfn));
-            if ( paging_mode_refcounts(v->domain) )
+            if ( paging_mode_translate(v->domain) )
                 put_page(page);
             else
                 rc = put_page_and_type_preemptible(page);
@@ -2811,7 +2811,7 @@ int new_guest_cr3(unsigned long mfn)
         return 0;
     }
 
-    rc = paging_mode_refcounts(d)
+    rc = paging_mode_translate(d)
          ? (get_page_from_mfn(_mfn(mfn), d) ? 0 : -EINVAL)
          : get_page_and_type_from_mfn(_mfn(mfn), PGT_root_page_table, d, 0, 1);
     switch ( rc )
@@ -2829,7 +2829,7 @@ int new_guest_cr3(unsigned long mfn)
 
     invalidate_shadow_ldt(curr, 0);
 
-    if ( !VM_ASSIST(d, m2p_strict) && !paging_mode_refcounts(d) )
+    if ( !VM_ASSIST(d, m2p_strict) && !paging_mode_translate(d) )
         fill_ro_mpt(mfn);
     curr->arch.guest_table = pagetable_from_pfn(mfn);
     update_cr3(curr);
@@ -2840,7 +2840,7 @@ int new_guest_cr3(unsigned long mfn)
     {
         struct page_info *page = mfn_to_page(_mfn(old_base_mfn));
 
-        if ( paging_mode_refcounts(d) )
+        if ( paging_mode_translate(d) )
             put_page(page);
         else
             switch ( rc = put_page_and_type_preemptible(page) )
@@ -3059,7 +3059,7 @@ long do_mmuext_op(
             if ( (op.cmd - MMUEXT_PIN_L1_TABLE) > (CONFIG_PAGING_LEVELS - 1) )
                 break;
 
-            if ( paging_mode_refcounts(pg_owner) )
+            if ( paging_mode_translate(pg_owner) )
                 break;
 
             page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC);
@@ -3121,7 +3121,7 @@ long do_mmuext_op(
             break;
 
         case MMUEXT_UNPIN_TABLE:
-            if ( paging_mode_refcounts(pg_owner) )
+            if ( paging_mode_translate(pg_owner) )
                 break;
 
             page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC);
@@ -3564,7 +3564,7 @@ long do_mmu_update(
             p2m_type_t p2mt;
 
             rc = -EOPNOTSUPP;
-            if ( unlikely(paging_mode_refcounts(pt_owner)) )
+            if ( unlikely(paging_mode_translate(pt_owner)) )
                 break;
 
             xsm_needed |= XSM_MMU_NORMAL_UPDATE;
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 75f5fc0024..354d96d4cc 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -839,11 +839,11 @@ int paging_enable(struct domain *d, u32 mode)
     if ( mode & ~PG_MASK )
         return -EINVAL;
 
-    /* All of external|translate|refcounts, or none. */
-    switch ( mode & (PG_external | PG_translate | PG_refcounts) )
+    /* Both of external|translate, or none. */
+    switch ( mode & (PG_external | PG_translate) )
     {
     case 0:
-    case PG_external | PG_translate | PG_refcounts:
+    case PG_external | PG_translate:
         break;
     default:
         return -EINVAL;
@@ -881,8 +881,6 @@ void paging_dump_domain_info(struct domain *d)
             printk("shadow ");
         if ( paging_mode_hap(d) )
             printk("hap ");
-        if ( paging_mode_refcounts(d) )
-            printk("refcounts ");
         if ( paging_mode_log_dirty(d) )
             printk("log_dirty ");
         if ( paging_mode_translate(d) )
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index 64bf2f968a..496a80a1ad 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -47,8 +47,6 @@
 
 /* common paging mode bits */
 #define PG_mode_shift  10 
-/* Refcounts based on shadow tables instead of guest tables */
-#define PG_refcounts   (XEN_DOMCTL_SHADOW_ENABLE_REFCOUNT << PG_mode_shift)
 /* Enable log dirty mode */
 #define PG_log_dirty   (XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY << PG_mode_shift)
 /* Xen does p2m translation, not guest */
@@ -58,13 +56,12 @@
 #define PG_external    (XEN_DOMCTL_SHADOW_ENABLE_EXTERNAL << PG_mode_shift)
 
 /* All paging modes. */
-#define PG_MASK (PG_refcounts | PG_log_dirty | PG_translate | PG_external)
+#define PG_MASK (PG_log_dirty | PG_translate | PG_external)
 
 #define paging_mode_enabled(_d)   (!!(_d)->arch.paging.mode)
 #define paging_mode_shadow(_d)    (!!((_d)->arch.paging.mode & PG_SH_enable))
 #define paging_mode_hap(_d)       (!!((_d)->arch.paging.mode & PG_HAP_enable))
 
-#define paging_mode_refcounts(_d) (!!((_d)->arch.paging.mode & PG_refcounts))
 #define paging_mode_log_dirty(_d) (!!((_d)->arch.paging.mode & PG_log_dirty))
 #define paging_mode_translate(_d) (!!((_d)->arch.paging.mode & PG_translate))
 #define paging_mode_external(_d)  (!!((_d)->arch.paging.mode & PG_external))
diff --git a/xen/include/asm-x86/shadow.h b/xen/include/asm-x86/shadow.h
index 678b5d48bb..e0607b9620 100644
--- a/xen/include/asm-x86/shadow.h
+++ b/xen/include/asm-x86/shadow.h
@@ -35,7 +35,7 @@
 
 #define shadow_mode_enabled(_d)    paging_mode_shadow(_d)
 #define shadow_mode_refcounts(_d) (paging_mode_shadow(_d) && \
-                                   paging_mode_refcounts(_d))
+                                   paging_mode_translate(_d))
 #define shadow_mode_log_dirty(_d) (paging_mode_shadow(_d) && \
                                    paging_mode_log_dirty(_d))
 #define shadow_mode_translate(_d) (paging_mode_shadow(_d) && \
-- 
2.11.0


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

Re: [PATCH RFC 2/2] x86/mm: PG_translate implies PG_refcounts

[Wei Liu]

On Wed, Aug 23, 2017 at 04:58:24PM +0100, Wei Liu wrote:
> After 404595352 ("x86/paging: Enforce PG_external == PG_translate ==
> PG_refcounts"), PG_refcounts and PG_translate always need to be set
> together.
> 
> Squash PG_refcounts to simplify code. All calls paging_mode_refcounts
> are replaced by calls to paging_mode_translate.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/domain.c        |  6 ++----
>  xen/arch/x86/hvm/hvm.c       |  2 +-
>  xen/arch/x86/mm.c            | 20 ++++++++++----------
>  xen/arch/x86/mm/paging.c     |  8 +++-----
>  xen/include/asm-x86/paging.h |  5 +----
>  xen/include/asm-x86/shadow.h |  2 +-
>  6 files changed, 18 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 9b4b9596d8..bbe545b165 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1017,8 +1017,6 @@ int arch_set_info_guest(
>  
>      if ( !cr3_page )
>          rc = -EINVAL;
> -    else if ( paging_mode_refcounts(d) )
> -        /* nothing */;

It appears I accidentally deleted this hunk, but my tests were still
happy...

Given this function is rather convoluted I intend to add it back and
deal with that later.

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

Re: [PATCH RFC 2/2] x86/mm: PG_translate implies PG_refcounts

[Tim Deegan]

At 16:58 +0100 on 23 Aug (1503507504), Wei Liu wrote:
> After 404595352 ("x86/paging: Enforce PG_external == PG_translate ==
> PG_refcounts"), PG_refcounts and PG_translate always need to be set
> together.
> 
> Squash PG_refcounts to simplify code. All calls paging_mode_refcounts
> are replaced by calls to paging_mode_translate.

Would it be a good idea to merge all three and have only PG_external?

> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

with one adjustment:

>  /* common paging mode bits */
>  #define PG_mode_shift  10 
> -/* Refcounts based on shadow tables instead of guest tables */
> -#define PG_refcounts   (XEN_DOMCTL_SHADOW_ENABLE_REFCOUNT << PG_mode_shift)
>  /* Enable log dirty mode */
>  #define PG_log_dirty   (XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY << PG_mode_shift)
>  /* Xen does p2m translation, not guest */

Please add "and handles refcounts" to the comment describing PG_translate.

Cheers,

Tim.

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

Re: [PATCH RFC 2/2] x86/mm: PG_translate implies PG_refcounts

[Wei Liu]

On Thu, Aug 24, 2017 at 11:02:33AM +0100, Tim Deegan wrote:
> At 16:58 +0100 on 23 Aug (1503507504), Wei Liu wrote:
> > After 404595352 ("x86/paging: Enforce PG_external == PG_translate ==
> > PG_refcounts"), PG_refcounts and PG_translate always need to be set
> > together.
> > 
> > Squash PG_refcounts to simplify code. All calls paging_mode_refcounts
> > are replaced by calls to paging_mode_translate.
> 
> Would it be a good idea to merge all three and have only PG_external?
> 

My reverse-engineering is that when PV guest is migrated it has
PG_external and (the new) PG_translate.

I would be happy to squash all three into one if you tell me I'm wrong.
:-)

> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Tim Deegan <tim@xen.org>
> 
> with one adjustment:
> 
> >  /* common paging mode bits */
> >  #define PG_mode_shift  10 
> > -/* Refcounts based on shadow tables instead of guest tables */
> > -#define PG_refcounts   (XEN_DOMCTL_SHADOW_ENABLE_REFCOUNT << PG_mode_shift)
> >  /* Enable log dirty mode */
> >  #define PG_log_dirty   (XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY << PG_mode_shift)
> >  /* Xen does p2m translation, not guest */
> 
> Please add "and handles refcounts" to the comment describing PG_translate.
> 

Sure.

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

Re: [PATCH RFC 2/2] x86/mm: PG_translate implies PG_refcounts

[Wei Liu]

On Thu, Aug 24, 2017 at 11:05:36AM +0100, Wei Liu wrote:
> On Thu, Aug 24, 2017 at 11:02:33AM +0100, Tim Deegan wrote:
> > At 16:58 +0100 on 23 Aug (1503507504), Wei Liu wrote:
> > > After 404595352 ("x86/paging: Enforce PG_external == PG_translate ==
> > > PG_refcounts"), PG_refcounts and PG_translate always need to be set
> > > together.
> > > 
> > > Squash PG_refcounts to simplify code. All calls paging_mode_refcounts
> > > are replaced by calls to paging_mode_translate.
> > 
> > Would it be a good idea to merge all three and have only PG_external?
> > 
> 
> My reverse-engineering is that when PV guest is migrated it has
> PG_external and (the new) PG_translate.

Sorry, I meant (!PG_external && PG_translate)

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

Re: [PATCH RFC 2/2] x86/mm: PG_translate implies PG_refcounts

[Andrew Cooper]

On 24/08/17 11:07, Wei Liu wrote:
> On Thu, Aug 24, 2017 at 11:05:36AM +0100, Wei Liu wrote:
>> On Thu, Aug 24, 2017 at 11:02:33AM +0100, Tim Deegan wrote:
>>> At 16:58 +0100 on 23 Aug (1503507504), Wei Liu wrote:
>>>> After 404595352 ("x86/paging: Enforce PG_external == PG_translate ==
>>>> PG_refcounts"), PG_refcounts and PG_translate always need to be set
>>>> together.
>>>>
>>>> Squash PG_refcounts to simplify code. All calls paging_mode_refcounts
>>>> are replaced by calls to paging_mode_translate.
>>> Would it be a good idea to merge all three and have only PG_external?
>>>
>> My reverse-engineering is that when PV guest is migrated it has
>> PG_external and (the new) PG_translate.
> Sorry, I meant (!PG_external && PG_translate)

PV guests migrating should get logdirty and none of
PG_external|PG_translate|PG_refcounts

~Andrew

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

Re: [PATCH RFC 2/2] x86/mm: PG_translate implies PG_refcounts

[Wei Liu]

On Thu, Aug 24, 2017 at 11:09:38AM +0100, Andrew Cooper wrote:
> On 24/08/17 11:07, Wei Liu wrote:
> > On Thu, Aug 24, 2017 at 11:05:36AM +0100, Wei Liu wrote:
> >> On Thu, Aug 24, 2017 at 11:02:33AM +0100, Tim Deegan wrote:
> >>> At 16:58 +0100 on 23 Aug (1503507504), Wei Liu wrote:
> >>>> After 404595352 ("x86/paging: Enforce PG_external == PG_translate ==
> >>>> PG_refcounts"), PG_refcounts and PG_translate always need to be set
> >>>> together.
> >>>>
> >>>> Squash PG_refcounts to simplify code. All calls paging_mode_refcounts
> >>>> are replaced by calls to paging_mode_translate.
> >>> Would it be a good idea to merge all three and have only PG_external?
> >>>
> >> My reverse-engineering is that when PV guest is migrated it has
> >> PG_external and (the new) PG_translate.
> > Sorry, I meant (!PG_external && PG_translate)
> 
> PV guests migrating should get logdirty and none of
> PG_external|PG_translate|PG_refcounts
> 

OK, in that case I will squash PG_external as well.

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

Re: [PATCH RFC 2/2] x86/mm: PG_translate implies PG_refcounts

[Tim Deegan]

At 11:05 +0100 on 24 Aug (1503572736), Wei Liu wrote:
> On Thu, Aug 24, 2017 at 11:02:33AM +0100, Tim Deegan wrote:
> > At 16:58 +0100 on 23 Aug (1503507504), Wei Liu wrote:
> > > After 404595352 ("x86/paging: Enforce PG_external == PG_translate ==
> > > PG_refcounts"), PG_refcounts and PG_translate always need to be set
> > > together.
> > > 
> > > Squash PG_refcounts to simplify code. All calls paging_mode_refcounts
> > > are replaced by calls to paging_mode_translate.
> > 
> > Would it be a good idea to merge all three and have only PG_external?
> > 
> 
> My reverse-engineering is that when PV guest is migrated it has
> PG_external and (the new) PG_translate.
> 
> I would be happy to squash all three into one if you tell me I'm wrong.
> :-)

I _think_ you're wrong :) but can't check right now as I'm
semi-offline.  Migrating PV guests should have paging enabled but
not any of external, translate or refcounts.

There was once code that used PG_translate with PV guests but never
AFAIK checked in to upstream Xen.  There was some talk of using that
combination for PVH at one point but IIRC PVH now uses
translate|refcounts|external like HVM.

Cheers,

Tim.

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