[PATCH v1 Altp2m cleanup 0/3] Cleaning up altp2m code

[PATCH v1 Altp2m cleanup 0/3] Cleaning up altp2m code

[Paul Lai]

Cleaning up altp2m code per request of xen-devel mailing list.

Paul Lai (3):
  altp2m cleanup work
  Move altp2m specific functions to altp2m files.
  Making altp2m struct dynamically allocated.

 xen/arch/x86/hvm/hvm.c            | 41 ++++++----------
 xen/arch/x86/hvm/vmx/vmx.c        |  2 +-
 xen/arch/x86/mm/altp2m.c          | 43 +++++++++++++++++
 xen/arch/x86/mm/hap/hap.c         | 35 ++------------
 xen/arch/x86/mm/mm-locks.h        |  4 +-
 xen/arch/x86/mm/p2m-ept.c         | 38 +++++++++++++++
 xen/arch/x86/mm/p2m.c             | 98 +++++++++++++--------------------------
 xen/include/asm-x86/altp2m.h      | 10 +++-
 xen/include/asm-x86/domain.h      | 12 ++++-
 xen/include/asm-x86/hvm/hvm.h     | 19 ++++++--
 xen/include/asm-x86/hvm/vmx/vmx.h |  3 ++
 xen/include/asm-x86/p2m.h         | 11 ++---
 xen/include/public/hvm/hvm_op.h   |  2 +
 13 files changed, 178 insertions(+), 140 deletions(-)

-- 
1.9.1


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

[PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work

[Paul Lai]

Indent goto labels by one space
Inline (header) altp2m functions
Define default behavior in switch
Define max and min for range of altp2m macroed values

Signed-off-by: Paul Lai <paul.c.lai@intel.com>
---
 xen/arch/x86/hvm/hvm.c          | 33 +++++++++------------------------
 xen/include/asm-x86/hvm/hvm.h   | 19 ++++++++++++++++---
 xen/include/public/hvm/hvm_op.h |  2 ++
 3 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 22f045e..1595b3e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1926,11 +1926,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
      * Otherwise, this is an error condition. */
     rc = fall_through;
 
-out_put_gfn:
+ out_put_gfn:
     __put_gfn(p2m, gfn);
     if ( ap2m_active )
         __put_gfn(hostp2m, gfn);
-out:
+ out:
     /* All of these are delayed until we exit, since we might 
      * sleep on event ring wait queues, and we must not hold
      * locks in such circumstance */
@@ -5207,9 +5207,11 @@ static int do_altp2m_op(
         return -EFAULT;
 
     if ( a.pad1 || a.pad2 ||
-         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) ||
-         (a.cmd < HVMOP_altp2m_get_domain_state) ||
-         (a.cmd > HVMOP_altp2m_change_gfn) )
+        (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) ||
+        (a.cmd < HVMOP_cmd_min) || (a.cmd > HVMOP_cmd_max))
+        return -EINVAL;
+
+    if (a.cmd > HVMOP_cmd_max)
         return -EINVAL;
 
     d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ?
@@ -5329,6 +5331,8 @@ static int do_altp2m_op(
             rc = p2m_change_altp2m_gfn(d, a.u.change_gfn.view,
                     _gfn(a.u.change_gfn.old_gfn),
                     _gfn(a.u.change_gfn.new_gfn));
+    default:
+        return -EINVAL;
     }
 
  out:
@@ -5816,25 +5820,6 @@ void hvm_toggle_singlestep(struct vcpu *v)
     v->arch.hvm_vcpu.single_step = !v->arch.hvm_vcpu.single_step;
 }
 
-void altp2m_vcpu_update_p2m(struct vcpu *v)
-{
-    if ( hvm_funcs.altp2m_vcpu_update_p2m )
-        hvm_funcs.altp2m_vcpu_update_p2m(v);
-}
-
-void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v)
-{
-    if ( hvm_funcs.altp2m_vcpu_update_vmfunc_ve )
-        hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v);
-}
-
-bool_t altp2m_vcpu_emulate_ve(struct vcpu *v)
-{
-    if ( hvm_funcs.altp2m_vcpu_emulate_ve )
-        return hvm_funcs.altp2m_vcpu_emulate_ve(v);
-    return 0;
-}
-
 int hvm_set_mode(struct vcpu *v, int mode)
 {
 
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index f486ee9..231c921 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -589,13 +589,26 @@ static inline bool_t hvm_altp2m_supported(void)
 }
 
 /* updates the current hardware p2m */
-void altp2m_vcpu_update_p2m(struct vcpu *v);
+static inline void altp2m_vcpu_update_p2m(struct vcpu *v)
+{
+    if ( hvm_funcs.altp2m_vcpu_update_p2m )
+        hvm_funcs.altp2m_vcpu_update_p2m(v);
+}
 
 /* updates VMCS fields related to VMFUNC and #VE */
-void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v);
+static inline void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v)
+{
+    if ( hvm_funcs.altp2m_vcpu_update_vmfunc_ve )
+        hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v);
+}
 
 /* emulates #VE */
-bool_t altp2m_vcpu_emulate_ve(struct vcpu *v);
+static inline bool_t altp2m_vcpu_emulate_ve(struct vcpu *v)
+{
+    if ( hvm_funcs.altp2m_vcpu_emulate_ve )
+        return hvm_funcs.altp2m_vcpu_emulate_ve(v);
+    return 0;
+}
 
 /* Check CR4/EFER values */
 const char *hvm_efer_valid(const struct vcpu *v, uint64_t value,
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index ebb907a..3350492 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -479,6 +479,8 @@ struct xen_hvm_altp2m_op {
 #define HVMOP_altp2m_set_mem_access       7
 /* Change a p2m entry to have a different gfn->mfn mapping */
 #define HVMOP_altp2m_change_gfn           8
+#define HVMOP_cmd_min                     HVMOP_altp2m_get_domain_state
+#define HVMOP_cmd_max                     HVMOP_altp2m_change_gfn
     domid_t domain;
     uint16_t pad1;
     uint32_t pad2;
-- 
1.9.1


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

Re: [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work

[Jan Beulich]

>>> On 21.06.16 at 18:04, <paul.c.lai@intel.com> wrote:
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -479,6 +479,8 @@ struct xen_hvm_altp2m_op {
>  #define HVMOP_altp2m_set_mem_access       7
>  /* Change a p2m entry to have a different gfn->mfn mapping */
>  #define HVMOP_altp2m_change_gfn           8
> +#define HVMOP_cmd_min                     HVMOP_altp2m_get_domain_state
> +#define HVMOP_cmd_max                     HVMOP_altp2m_change_gfn

I don't see why these would need to be in the public interface. Nor
were their names chosen to properly describe their purpose.

Jan


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

Re: [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work

[Lai, Paul C]

I'm opposed to moving HVMOP_cmd_min and HVMOP_cmd_max somewhere else.  That would make reading/understanding of the macros more difficult.  This practice is common.  Also, If min & max are defined elsewhere, it will be more likely to lead to mistakes/bugs. 

The use of "_min" and "_max" should be quite clear and is common use in linux code; Yes, I know this is xen code and I see it here too.  If there's a better way, please propose the better.  Maybe you're suggesting the macro names should be all caps: 
  HVMOP_CMD_MIN, HVMOP_CMD_MAX
?  I was following the coding style within the file itself.

I'm open to suggestions,
-Paul

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@suse.com] 
Sent: Thursday, June 23, 2016 8:45 AM
To: Lai, Paul C <paul.c.lai@intel.com>
Cc: Sahita, Ravi <ravi.sahita@intel.com>; xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work

>>> On 21.06.16 at 18:04, <paul.c.lai@intel.com> wrote:
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -479,6 +479,8 @@ struct xen_hvm_altp2m_op {
>  #define HVMOP_altp2m_set_mem_access       7
>  /* Change a p2m entry to have a different gfn->mfn mapping */
>  #define HVMOP_altp2m_change_gfn           8
> +#define HVMOP_cmd_min                     HVMOP_altp2m_get_domain_state
> +#define HVMOP_cmd_max                     HVMOP_altp2m_change_gfn

I don't see why these would need to be in the public interface. Nor were their names chosen to properly describe their purpose.

Jan


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

Re: [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work

[Jan Beulich]

>>> On 23.06.16 at 20:23, <paul.c.lai@intel.com> wrote:

First of all - please don't top post.

> I'm opposed to moving HVMOP_cmd_min and HVMOP_cmd_max somewhere else.  That 
> would make reading/understanding of the macros more difficult.  This practice 
> is common.  Also, If min & max are defined elsewhere, it will be more likely 
> to lead to mistakes/bugs. 

I understand that, but I'm going to nak any patch that introduces
clutter (which then will need to be retained forever) to the public
interface. A possible compromise might be to frame these in __XEN__
conditionals, but this would need to be outweighed by the resulting
benefits, which I'm not convinced of.

> The use of "_min" and "_max" should be quite clear and is common use in 
> linux code; Yes, I know this is xen code and I see it here too.  If there's a 
> better way, please propose the better.  Maybe you're suggesting the macro 
> names should be all caps: 
>   HVMOP_CMD_MIN, HVMOP_CMD_MAX
> ?  I was following the coding style within the file itself.

The problem isn't the use of min and max, but the selected names
suggesting these are the minimum/maximum HVMOPs, whereas
- afaict - you really mean to frame the altp2m subset. Which btw,
together with the above, points at another issue here: What if
later another altp2m subop gets added, and especially when its
new value ends up not being adjacent to the existing range?

Jan


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

Re: [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work

[Lai, Paul C]

[Paul] inlined....

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@suse.com] 
Sent: Thursday, June 23, 2016 11:19 PM
To: Lai, Paul C <paul.c.lai@intel.com>
Cc: Sahita, Ravi <ravi.sahita@intel.com>; xen-devel <xen-devel@lists.xenproject.org>
Subject: RE: [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work

>>> On 23.06.16 at 20:23, <paul.c.lai@intel.com> wrote:

First of all - please don't top post.
[Paul] Understood

> I'm opposed to moving HVMOP_cmd_min and HVMOP_cmd_max somewhere else.  
> That would make reading/understanding of the macros more difficult.  
> This practice is common.  Also, If min & max are defined elsewhere, it 
> will be more likely to lead to mistakes/bugs.

I understand that, but I'm going to nak any patch that introduces clutter (which then will need to be retained forever) to the public interface. A possible compromise might be to frame these in __XEN__ conditionals, but this would need to be outweighed by the resulting benefits, which I'm not convinced of.

[Paul] Still waiting for a better idea.

> The use of "_min" and "_max" should be quite clear and is common use 
> in linux code; Yes, I know this is xen code and I see it here too.  If 
> there's a better way, please propose the better.  Maybe you're 
> suggesting the macro names should be all caps:
>   HVMOP_CMD_MIN, HVMOP_CMD_MAX
> ?  I was following the coding style within the file itself.

The problem isn't the use of min and max, but the selected names suggesting these are the minimum/maximum HVMOPs, whereas
- afaict - you really mean to frame the altp2m subset. Which btw, together with the above, points at another issue here: What if later another altp2m subop gets added, and especially when its new value ends up not being adjacent to the existing range?

[Paul] Again, waiting for a better idea.

Jan


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

Re: [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work

[George Dunlap]

On Thu, Jun 23, 2016 at 7:23 PM, Lai, Paul C <paul.c.lai@intel.com> wrote:
> I'm opposed to moving HVMOP_cmd_min and HVMOP_cmd_max somewhere else.  That would make reading/understanding of the macros more difficult.  This practice is common.  Also, If min & max are defined elsewhere, it will be more likely to lead to mistakes/bugs.
>
> The use of "_min" and "_max" should be quite clear and is common use in linux code; Yes, I know this is xen code and I see it here too.  If there's a better way, please propose the better.  Maybe you're suggesting the macro names should be all caps:
>   HVMOP_CMD_MIN, HVMOP_CMD_MAX
> ?  I was following the coding style within the file itself.

Jan was suggesting that these should be called HVMOP_altp2m_{max,min}
(or perhaps HVMOP_altp2m_cmd_{max,min}).

But in any case, the most robust thing to do is not to check the
values here at all -- just add a default: clause to the switch()
statement which returns -ENOSYS.

 -George

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

Re: [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work

[George Dunlap]

(Removing Paul, adding Lars)

Ravi,

I just got to this thread, and I was quite disappointed with both the
code and the interaction here.

In patch 1, the naming of the min/max is obviously inappropriate, and
a.cmd is compared to HVMOP_cmd_max twice in a row.

In patch 3, unused elements of the struct are commented out rather than deleted.

These aren't "new to the Xen way of doing things" mistakes, these are
basic programming errors.  Did anyone review this series internally?

 -George

On Tue, Jun 21, 2016 at 5:04 PM, Paul Lai <paul.c.lai@intel.com> wrote:
> Indent goto labels by one space
> Inline (header) altp2m functions
> Define default behavior in switch
> Define max and min for range of altp2m macroed values
>
> Signed-off-by: Paul Lai <paul.c.lai@intel.com>
> ---
>  xen/arch/x86/hvm/hvm.c          | 33 +++++++++------------------------
>  xen/include/asm-x86/hvm/hvm.h   | 19 ++++++++++++++++---
>  xen/include/public/hvm/hvm_op.h |  2 ++
>  3 files changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 22f045e..1595b3e 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1926,11 +1926,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
>       * Otherwise, this is an error condition. */
>      rc = fall_through;
>
> -out_put_gfn:
> + out_put_gfn:
>      __put_gfn(p2m, gfn);
>      if ( ap2m_active )
>          __put_gfn(hostp2m, gfn);
> -out:
> + out:
>      /* All of these are delayed until we exit, since we might
>       * sleep on event ring wait queues, and we must not hold
>       * locks in such circumstance */
> @@ -5207,9 +5207,11 @@ static int do_altp2m_op(
>          return -EFAULT;
>
>      if ( a.pad1 || a.pad2 ||
> -         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) ||
> -         (a.cmd < HVMOP_altp2m_get_domain_state) ||
> -         (a.cmd > HVMOP_altp2m_change_gfn) )
> +        (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) ||
> +        (a.cmd < HVMOP_cmd_min) || (a.cmd > HVMOP_cmd_max))
> +        return -EINVAL;
> +
> +    if (a.cmd > HVMOP_cmd_max)
>          return -EINVAL;
>
>      d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ?
> @@ -5329,6 +5331,8 @@ static int do_altp2m_op(
>              rc = p2m_change_altp2m_gfn(d, a.u.change_gfn.view,
>                      _gfn(a.u.change_gfn.old_gfn),
>                      _gfn(a.u.change_gfn.new_gfn));
> +    default:
> +        return -EINVAL;
>      }
>
>   out:
> @@ -5816,25 +5820,6 @@ void hvm_toggle_singlestep(struct vcpu *v)
>      v->arch.hvm_vcpu.single_step = !v->arch.hvm_vcpu.single_step;
>  }
>
> -void altp2m_vcpu_update_p2m(struct vcpu *v)
> -{
> -    if ( hvm_funcs.altp2m_vcpu_update_p2m )
> -        hvm_funcs.altp2m_vcpu_update_p2m(v);
> -}
> -
> -void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v)
> -{
> -    if ( hvm_funcs.altp2m_vcpu_update_vmfunc_ve )
> -        hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v);
> -}
> -
> -bool_t altp2m_vcpu_emulate_ve(struct vcpu *v)
> -{
> -    if ( hvm_funcs.altp2m_vcpu_emulate_ve )
> -        return hvm_funcs.altp2m_vcpu_emulate_ve(v);
> -    return 0;
> -}
> -
>  int hvm_set_mode(struct vcpu *v, int mode)
>  {
>
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index f486ee9..231c921 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -589,13 +589,26 @@ static inline bool_t hvm_altp2m_supported(void)
>  }
>
>  /* updates the current hardware p2m */
> -void altp2m_vcpu_update_p2m(struct vcpu *v);
> +static inline void altp2m_vcpu_update_p2m(struct vcpu *v)
> +{
> +    if ( hvm_funcs.altp2m_vcpu_update_p2m )
> +        hvm_funcs.altp2m_vcpu_update_p2m(v);
> +}
>
>  /* updates VMCS fields related to VMFUNC and #VE */
> -void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v);
> +static inline void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v)
> +{
> +    if ( hvm_funcs.altp2m_vcpu_update_vmfunc_ve )
> +        hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v);
> +}
>
>  /* emulates #VE */
> -bool_t altp2m_vcpu_emulate_ve(struct vcpu *v);
> +static inline bool_t altp2m_vcpu_emulate_ve(struct vcpu *v)
> +{
> +    if ( hvm_funcs.altp2m_vcpu_emulate_ve )
> +        return hvm_funcs.altp2m_vcpu_emulate_ve(v);
> +    return 0;
> +}
>
>  /* Check CR4/EFER values */
>  const char *hvm_efer_valid(const struct vcpu *v, uint64_t value,
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index ebb907a..3350492 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -479,6 +479,8 @@ struct xen_hvm_altp2m_op {
>  #define HVMOP_altp2m_set_mem_access       7
>  /* Change a p2m entry to have a different gfn->mfn mapping */
>  #define HVMOP_altp2m_change_gfn           8
> +#define HVMOP_cmd_min                     HVMOP_altp2m_get_domain_state
> +#define HVMOP_cmd_max                     HVMOP_altp2m_change_gfn
>      domid_t domain;
>      uint16_t pad1;
>      uint32_t pad2;
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[PATCH v1 Altp2m cleanup 2/3] Move altp2m specific functions to altp2m files.

[Paul Lai]

Move altp2m specific functions to altp2m files.  This makes the code
a little easier to read.

Signed-off-by: Paul Lai <paul.c.lai@intel.com>
---
 xen/arch/x86/mm/altp2m.c          | 43 +++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/mm/hap/hap.c         | 35 +++++--------------------------
 xen/arch/x86/mm/p2m-ept.c         | 38 ++++++++++++++++++++++++++++++++++
 xen/arch/x86/mm/p2m.c             | 41 +------------------------------------
 xen/include/asm-x86/altp2m.h      |  3 ++-
 xen/include/asm-x86/hvm/vmx/vmx.h |  3 +++
 xen/include/asm-x86/p2m.h         |  9 +++-----
 7 files changed, 95 insertions(+), 77 deletions(-)

diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index 10605c8..1caf6b4 100644
--- a/xen/arch/x86/mm/altp2m.c
+++ b/xen/arch/x86/mm/altp2m.c
@@ -17,6 +17,7 @@
 
 #include <asm/hvm/support.h>
 #include <asm/hvm/hvm.h>
+#include <asm/domain.h>
 #include <asm/p2m.h>
 #include <asm/altp2m.h>
 
@@ -65,6 +66,48 @@ altp2m_vcpu_destroy(struct vcpu *v)
         vcpu_unpause(v);
 }
 
+int
+hvm_altp2m_init( struct domain *d) {
+    int rv = 0;
+    unsigned int i = 0;
+
+    /* Init alternate p2m data */
+    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
+    {
+        rv = -ENOMEM;
+        goto out;
+    }
+
+    for ( i = 0; i < MAX_EPTP; i++ )
+        d->arch.altp2m_eptp[i] = INVALID_MFN;
+
+    for ( i = 0; i < MAX_ALTP2M; i++ )
+    {
+        rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
+        if ( rv != 0 )
+           goto out;
+    }
+
+    d->arch.altp2m_active = 0;
+ out:
+    return rv;
+}
+
+void
+hvm_altp2m_teardown( struct domain *d) {
+    unsigned int i = 0;
+    d->arch.altp2m_active = 0;
+
+    if ( d->arch.altp2m_eptp )
+    {
+        free_xenheap_page(d->arch.altp2m_eptp);
+        d->arch.altp2m_eptp = NULL;
+    }
+
+    for ( i = 0; i < MAX_ALTP2M; i++ )
+        p2m_teardown(d->arch.altp2m_p2m[i]);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 9c2cd49..07833b7 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -37,6 +37,7 @@
 #include <asm/hap.h>
 #include <asm/paging.h>
 #include <asm/p2m.h>
+#include <asm/altp2m.h>
 #include <asm/domain.h>
 #include <xen/numa.h>
 #include <asm/hvm/nestedhvm.h>
@@ -501,24 +502,9 @@ int hap_enable(struct domain *d, u32 mode)
 
     if ( hvm_altp2m_supported() )
     {
-        /* Init alternate p2m data */
-        if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
-        {
-            rv = -ENOMEM;
-            goto out;
-        }
-
-        for ( i = 0; i < MAX_EPTP; i++ )
-            d->arch.altp2m_eptp[i] = INVALID_MFN;
-
-        for ( i = 0; i < MAX_ALTP2M; i++ )
-        {
-            rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
-            if ( rv != 0 )
-               goto out;
-        }
-
-        d->arch.altp2m_active = 0;
+        rv = hvm_altp2m_init(d);
+        if ( rv != 0 )
+           goto out;
     }
 
     /* Now let other users see the new mode */
@@ -534,18 +520,7 @@ void hap_final_teardown(struct domain *d)
     unsigned int i;
 
     if ( hvm_altp2m_supported() )
-    {
-        d->arch.altp2m_active = 0;
-
-        if ( d->arch.altp2m_eptp )
-        {
-            free_xenheap_page(d->arch.altp2m_eptp);
-            d->arch.altp2m_eptp = NULL;
-        }
-
-        for ( i = 0; i < MAX_ALTP2M; i++ )
-            p2m_teardown(d->arch.altp2m_p2m[i]);
-    }
+        hvm_altp2m_teardown(d);
 
     /* Destroy nestedp2m's first */
     for (i = 0; i < MAX_NESTEDP2M; i++) {
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 7166c71..dff34b1 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1329,6 +1329,44 @@ void setup_ept_dump(void)
     register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
 }
 
+void p2m_init_altp2m_helper( struct domain *d, unsigned int i) {
+    struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
+    struct ept_data *ept;
+
+    p2m->min_remapped_gfn = INVALID_GFN;
+    p2m->max_remapped_gfn = 0;
+    ept = &p2m->ept;
+    ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m));
+    d->arch.altp2m_eptp[i] = ept_get_eptp(ept);
+}
+
+unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
+{
+    struct p2m_domain *p2m;
+    struct ept_data *ept;
+    unsigned int i;
+
+    altp2m_list_lock(d);
+
+    for ( i = 0; i < MAX_ALTP2M; i++ )
+    {
+        if ( d->arch.altp2m_eptp[i] == INVALID_MFN )
+            continue;
+
+        p2m = d->arch.altp2m_p2m[i];
+        ept = &p2m->ept;
+
+        if ( eptp == ept_get_eptp(ept) )
+            goto out;
+    }
+
+    i = INVALID_ALTP2M;
+
+ out:
+    altp2m_list_unlock(d);
+    return i;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 89462b2..90f2d95 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -196,8 +196,8 @@ static void p2m_teardown_altp2m(struct domain *d)
         if ( !d->arch.altp2m_p2m[i] )
             continue;
         p2m = d->arch.altp2m_p2m[i];
-        d->arch.altp2m_p2m[i] = NULL;
         p2m_free_one(p2m);
+        d->arch.altp2m_p2m[i] = NULL;
     }
 }
 
@@ -2270,33 +2270,6 @@ int unmap_mmio_regions(struct domain *d,
     return i == nr ? 0 : i ?: ret;
 }
 
-unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
-{
-    struct p2m_domain *p2m;
-    struct ept_data *ept;
-    unsigned int i;
-
-    altp2m_list_lock(d);
-
-    for ( i = 0; i < MAX_ALTP2M; i++ )
-    {
-        if ( d->arch.altp2m_eptp[i] == INVALID_MFN )
-            continue;
-
-        p2m = d->arch.altp2m_p2m[i];
-        ept = &p2m->ept;
-
-        if ( eptp == ept_get_eptp(ept) )
-            goto out;
-    }
-
-    i = INVALID_ALTP2M;
-
- out:
-    altp2m_list_unlock(d);
-    return i;
-}
-
 bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx)
 {
     struct domain *d = v->domain;
@@ -2402,18 +2375,6 @@ void p2m_flush_altp2m(struct domain *d)
     altp2m_list_unlock(d);
 }
 
-static void p2m_init_altp2m_helper(struct domain *d, unsigned int i)
-{
-    struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
-    struct ept_data *ept;
-
-    p2m->min_remapped_gfn = INVALID_GFN;
-    p2m->max_remapped_gfn = 0;
-    ept = &p2m->ept;
-    ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m));
-    d->arch.altp2m_eptp[i] = ept_get_eptp(ept);
-}
-
 int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
 {
     int rc = -EINVAL;
diff --git a/xen/include/asm-x86/altp2m.h b/xen/include/asm-x86/altp2m.h
index 64c7618..7ce047d 100644
--- a/xen/include/asm-x86/altp2m.h
+++ b/xen/include/asm-x86/altp2m.h
@@ -18,7 +18,6 @@
 #ifndef __ASM_X86_ALTP2M_H
 #define __ASM_X86_ALTP2M_H
 
-#include <xen/types.h>
 #include <xen/sched.h>         /* for struct vcpu, struct domain */
 #include <asm/hvm/vcpu.h>      /* for vcpu_altp2m */
 
@@ -37,5 +36,7 @@ static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
 {
     return vcpu_altp2m(v).p2midx;
 }
+int hvm_altp2m_init(struct domain *d);
+void hvm_altp2m_teardown(struct domain *d);
 
 #endif /* __ASM_X86_ALTP2M_H */
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 359b2a9..98032fb 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -556,6 +556,9 @@ void ept_p2m_uninit(struct p2m_domain *p2m);
 void ept_walk_table(struct domain *d, unsigned long gfn);
 bool_t ept_handle_misconfig(uint64_t gpa);
 void setup_ept_dump(void);
+void p2m_init_altp2m_helper( struct domain *d, unsigned int i);
+/* Locate an alternate p2m by its EPTP */
+unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp);
 
 void update_guest_eip(void);
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 65675a2..d7c8c12 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -23,8 +23,8 @@
  * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef _XEN_P2M_H
-#define _XEN_P2M_H
+#ifndef _XEN_ASM_X86_P2M_H
+#define _XEN_ASM_X86_P2M_H
 
 #include <xen/config.h>
 #include <xen/paging.h>
@@ -779,9 +779,6 @@ static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v)
     return v->domain->arch.altp2m_p2m[index];
 }
 
-/* Locate an alternate p2m by its EPTP */
-unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp);
-
 /* Switch alternate p2m for a single vcpu */
 bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx);
 
@@ -843,7 +840,7 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
     return flags;
 }
 
-#endif /* _XEN_P2M_H */
+#endif /* _XEN_ASM_X86_P2M_H */
 
 /*
  * Local variables:
-- 
1.9.1


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

Re: [PATCH v1 Altp2m cleanup 2/3] Move altp2m specific functions to altp2m files.

[George Dunlap]

On Tue, Jun 21, 2016 at 5:04 PM, Paul Lai <paul.c.lai@intel.com> wrote:
> Move altp2m specific functions to altp2m files.  This makes the code
> a little easier to read.
>
> Signed-off-by: Paul Lai <paul.c.lai@intel.com>
> ---
>  xen/arch/x86/mm/altp2m.c          | 43 +++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/mm/hap/hap.c         | 35 +++++--------------------------
>  xen/arch/x86/mm/p2m-ept.c         | 38 ++++++++++++++++++++++++++++++++++
>  xen/arch/x86/mm/p2m.c             | 41 +------------------------------------
>  xen/include/asm-x86/altp2m.h      |  3 ++-
>  xen/include/asm-x86/hvm/vmx/vmx.h |  3 +++
>  xen/include/asm-x86/p2m.h         |  9 +++-----

You forgot to CC' the MM maintainer (me).  :-)

Please see MAINTAINERS file, and/or use get-maintainers.pl in the
future, or your patches risk dropping through the cracks.

(I've put this on my list to review now.)

 -George

>  7 files changed, 95 insertions(+), 77 deletions(-)
>
> diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
> index 10605c8..1caf6b4 100644
> --- a/xen/arch/x86/mm/altp2m.c
> +++ b/xen/arch/x86/mm/altp2m.c
> @@ -17,6 +17,7 @@
>
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/hvm.h>
> +#include <asm/domain.h>
>  #include <asm/p2m.h>
>  #include <asm/altp2m.h>
>
> @@ -65,6 +66,48 @@ altp2m_vcpu_destroy(struct vcpu *v)
>          vcpu_unpause(v);
>  }
>
> +int
> +hvm_altp2m_init( struct domain *d) {
> +    int rv = 0;
> +    unsigned int i = 0;
> +
> +    /* Init alternate p2m data */
> +    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +    {
> +        rv = -ENOMEM;
> +        goto out;
> +    }
> +
> +    for ( i = 0; i < MAX_EPTP; i++ )
> +        d->arch.altp2m_eptp[i] = INVALID_MFN;
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> +        if ( rv != 0 )
> +           goto out;
> +    }
> +
> +    d->arch.altp2m_active = 0;
> + out:
> +    return rv;
> +}
> +
> +void
> +hvm_altp2m_teardown( struct domain *d) {
> +    unsigned int i = 0;
> +    d->arch.altp2m_active = 0;
> +
> +    if ( d->arch.altp2m_eptp )
> +    {
> +        free_xenheap_page(d->arch.altp2m_eptp);
> +        d->arch.altp2m_eptp = NULL;
> +    }
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +        p2m_teardown(d->arch.altp2m_p2m[i]);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 9c2cd49..07833b7 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -37,6 +37,7 @@
>  #include <asm/hap.h>
>  #include <asm/paging.h>
>  #include <asm/p2m.h>
> +#include <asm/altp2m.h>
>  #include <asm/domain.h>
>  #include <xen/numa.h>
>  #include <asm/hvm/nestedhvm.h>
> @@ -501,24 +502,9 @@ int hap_enable(struct domain *d, u32 mode)
>
>      if ( hvm_altp2m_supported() )
>      {
> -        /* Init alternate p2m data */
> -        if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> -        {
> -            rv = -ENOMEM;
> -            goto out;
> -        }
> -
> -        for ( i = 0; i < MAX_EPTP; i++ )
> -            d->arch.altp2m_eptp[i] = INVALID_MFN;
> -
> -        for ( i = 0; i < MAX_ALTP2M; i++ )
> -        {
> -            rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> -            if ( rv != 0 )
> -               goto out;
> -        }
> -
> -        d->arch.altp2m_active = 0;
> +        rv = hvm_altp2m_init(d);
> +        if ( rv != 0 )
> +           goto out;
>      }
>
>      /* Now let other users see the new mode */
> @@ -534,18 +520,7 @@ void hap_final_teardown(struct domain *d)
>      unsigned int i;
>
>      if ( hvm_altp2m_supported() )
> -    {
> -        d->arch.altp2m_active = 0;
> -
> -        if ( d->arch.altp2m_eptp )
> -        {
> -            free_xenheap_page(d->arch.altp2m_eptp);
> -            d->arch.altp2m_eptp = NULL;
> -        }
> -
> -        for ( i = 0; i < MAX_ALTP2M; i++ )
> -            p2m_teardown(d->arch.altp2m_p2m[i]);
> -    }
> +        hvm_altp2m_teardown(d);
>
>      /* Destroy nestedp2m's first */
>      for (i = 0; i < MAX_NESTEDP2M; i++) {
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 7166c71..dff34b1 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1329,6 +1329,44 @@ void setup_ept_dump(void)
>      register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
>  }
>
> +void p2m_init_altp2m_helper( struct domain *d, unsigned int i) {
> +    struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
> +    struct ept_data *ept;
> +
> +    p2m->min_remapped_gfn = INVALID_GFN;
> +    p2m->max_remapped_gfn = 0;
> +    ept = &p2m->ept;
> +    ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m));
> +    d->arch.altp2m_eptp[i] = ept_get_eptp(ept);
> +}
> +
> +unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
> +{
> +    struct p2m_domain *p2m;
> +    struct ept_data *ept;
> +    unsigned int i;
> +
> +    altp2m_list_lock(d);
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        if ( d->arch.altp2m_eptp[i] == INVALID_MFN )
> +            continue;
> +
> +        p2m = d->arch.altp2m_p2m[i];
> +        ept = &p2m->ept;
> +
> +        if ( eptp == ept_get_eptp(ept) )
> +            goto out;
> +    }
> +
> +    i = INVALID_ALTP2M;
> +
> + out:
> +    altp2m_list_unlock(d);
> +    return i;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 89462b2..90f2d95 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -196,8 +196,8 @@ static void p2m_teardown_altp2m(struct domain *d)
>          if ( !d->arch.altp2m_p2m[i] )
>              continue;
>          p2m = d->arch.altp2m_p2m[i];
> -        d->arch.altp2m_p2m[i] = NULL;
>          p2m_free_one(p2m);
> +        d->arch.altp2m_p2m[i] = NULL;
>      }
>  }
>
> @@ -2270,33 +2270,6 @@ int unmap_mmio_regions(struct domain *d,
>      return i == nr ? 0 : i ?: ret;
>  }
>
> -unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
> -{
> -    struct p2m_domain *p2m;
> -    struct ept_data *ept;
> -    unsigned int i;
> -
> -    altp2m_list_lock(d);
> -
> -    for ( i = 0; i < MAX_ALTP2M; i++ )
> -    {
> -        if ( d->arch.altp2m_eptp[i] == INVALID_MFN )
> -            continue;
> -
> -        p2m = d->arch.altp2m_p2m[i];
> -        ept = &p2m->ept;
> -
> -        if ( eptp == ept_get_eptp(ept) )
> -            goto out;
> -    }
> -
> -    i = INVALID_ALTP2M;
> -
> - out:
> -    altp2m_list_unlock(d);
> -    return i;
> -}
> -
>  bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx)
>  {
>      struct domain *d = v->domain;
> @@ -2402,18 +2375,6 @@ void p2m_flush_altp2m(struct domain *d)
>      altp2m_list_unlock(d);
>  }
>
> -static void p2m_init_altp2m_helper(struct domain *d, unsigned int i)
> -{
> -    struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
> -    struct ept_data *ept;
> -
> -    p2m->min_remapped_gfn = INVALID_GFN;
> -    p2m->max_remapped_gfn = 0;
> -    ept = &p2m->ept;
> -    ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m));
> -    d->arch.altp2m_eptp[i] = ept_get_eptp(ept);
> -}
> -
>  int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>  {
>      int rc = -EINVAL;
> diff --git a/xen/include/asm-x86/altp2m.h b/xen/include/asm-x86/altp2m.h
> index 64c7618..7ce047d 100644
> --- a/xen/include/asm-x86/altp2m.h
> +++ b/xen/include/asm-x86/altp2m.h
> @@ -18,7 +18,6 @@
>  #ifndef __ASM_X86_ALTP2M_H
>  #define __ASM_X86_ALTP2M_H
>
> -#include <xen/types.h>
>  #include <xen/sched.h>         /* for struct vcpu, struct domain */
>  #include <asm/hvm/vcpu.h>      /* for vcpu_altp2m */
>
> @@ -37,5 +36,7 @@ static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
>  {
>      return vcpu_altp2m(v).p2midx;
>  }
> +int hvm_altp2m_init(struct domain *d);
> +void hvm_altp2m_teardown(struct domain *d);
>
>  #endif /* __ASM_X86_ALTP2M_H */
> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
> index 359b2a9..98032fb 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -556,6 +556,9 @@ void ept_p2m_uninit(struct p2m_domain *p2m);
>  void ept_walk_table(struct domain *d, unsigned long gfn);
>  bool_t ept_handle_misconfig(uint64_t gpa);
>  void setup_ept_dump(void);
> +void p2m_init_altp2m_helper( struct domain *d, unsigned int i);
> +/* Locate an alternate p2m by its EPTP */
> +unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp);
>
>  void update_guest_eip(void);
>
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 65675a2..d7c8c12 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -23,8 +23,8 @@
>   * along with this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>
> -#ifndef _XEN_P2M_H
> -#define _XEN_P2M_H
> +#ifndef _XEN_ASM_X86_P2M_H
> +#define _XEN_ASM_X86_P2M_H
>
>  #include <xen/config.h>
>  #include <xen/paging.h>
> @@ -779,9 +779,6 @@ static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v)
>      return v->domain->arch.altp2m_p2m[index];
>  }
>
> -/* Locate an alternate p2m by its EPTP */
> -unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp);
> -
>  /* Switch alternate p2m for a single vcpu */
>  bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx);
>
> @@ -843,7 +840,7 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
>      return flags;
>  }
>
> -#endif /* _XEN_P2M_H */
> +#endif /* _XEN_ASM_X86_P2M_H */
>
>  /*
>   * Local variables:
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

Re: [PATCH v1 Altp2m cleanup 2/3] Move altp2m specific functions to altp2m files.

[Jan Beulich]

>>> On 21.06.16 at 18:04, <paul.c.lai@intel.com> wrote:
> @@ -65,6 +66,48 @@ altp2m_vcpu_destroy(struct vcpu *v)
>          vcpu_unpause(v);
>  }
>  
> +int
> +hvm_altp2m_init( struct domain *d) {

Coding style (stray blank and misplaced brace).

> +    int rv = 0;

I guess rc or ret would be the more conventional names.

> +    unsigned int i = 0;

Pointless initializer.

> +    /* Init alternate p2m data */

Missing full stop.

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1329,6 +1329,44 @@ void setup_ept_dump(void)
>      register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
>  }
>  
> +void p2m_init_altp2m_helper( struct domain *d, unsigned int i) {

While moving it here, please adjust the name to make clear this
is EPT specific. Also it looks like the first parameter could become
const.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -196,8 +196,8 @@ static void p2m_teardown_altp2m(struct domain *d)
>          if ( !d->arch.altp2m_p2m[i] )
>              continue;
>          p2m = d->arch.altp2m_p2m[i];
> -        d->arch.altp2m_p2m[i] = NULL;
>          p2m_free_one(p2m);
> +        d->arch.altp2m_p2m[i] = NULL;
>      }

Why, without any other changes (you're only moving code around)?

> @@ -37,5 +36,7 @@ static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
>  {
>      return vcpu_altp2m(v).p2midx;
>  }
> +int hvm_altp2m_init(struct domain *d);
> +void hvm_altp2m_teardown(struct domain *d);

Missing separating blank line.

Jan


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

[PATCH v1 Altp2m cleanup 3/3] Making altp2m struct dynamically allocated.

[Paul Lai]

Ravi Sahita's dynamically allocated altp2m structs

Signed-off-by: Paul Lai <paul.c.lai@intel.com>
---
 xen/arch/x86/hvm/hvm.c       |  8 +++---
 xen/arch/x86/hvm/vmx/vmx.c   |  2 +-
 xen/arch/x86/mm/altp2m.c     | 18 +++++++-------
 xen/arch/x86/mm/mm-locks.h   |  4 +--
 xen/arch/x86/mm/p2m-ept.c    |  8 +++---
 xen/arch/x86/mm/p2m.c        | 59 ++++++++++++++++++++++++--------------------
 xen/include/asm-x86/altp2m.h |  7 +++++-
 xen/include/asm-x86/domain.h | 12 ++++++++-
 xen/include/asm-x86/p2m.h    |  2 +-
 9 files changed, 70 insertions(+), 50 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 1595b3e..40270d0 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5228,7 +5228,7 @@ static int do_altp2m_op(
 
     if ( (a.cmd != HVMOP_altp2m_get_domain_state) &&
          (a.cmd != HVMOP_altp2m_set_domain_state) &&
-         !d->arch.altp2m_active )
+         ! altp2m_active(d) )
     {
         rc = -EOPNOTSUPP;
         goto out;
@@ -5262,11 +5262,11 @@ static int do_altp2m_op(
             break;
         }
 
-        ostate = d->arch.altp2m_active;
-        d->arch.altp2m_active = !!a.u.domain_state.state;
+        ostate = altp2m_active(d);
+	set_altp2m_active(d, !!a.u.domain_state.state);
 
         /* If the alternate p2m state has changed, handle appropriately */
-        if ( d->arch.altp2m_active != ostate &&
+        if ( altp2m_active(d) != ostate &&
              (ostate || !(rc = p2m_init_altp2m_by_id(d, 0))) )
         {
             for_each_vcpu( d, v )
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 670d7dc..b522578 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2017,7 +2017,7 @@ static void vmx_vcpu_update_vmfunc_ve(struct vcpu *v)
     {
         v->arch.hvm_vmx.secondary_exec_control |= mask;
         __vmwrite(VM_FUNCTION_CONTROL, VMX_VMFUNC_EPTP_SWITCHING);
-        __vmwrite(EPTP_LIST_ADDR, virt_to_maddr(d->arch.altp2m_eptp));
+        __vmwrite(EPTP_LIST_ADDR, virt_to_maddr(d->arch.altp2m->altp2m_eptp));
 
         if ( cpu_has_vmx_virt_exceptions )
         {
diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index 1caf6b4..77187c9 100644
--- a/xen/arch/x86/mm/altp2m.c
+++ b/xen/arch/x86/mm/altp2m.c
@@ -72,23 +72,23 @@ hvm_altp2m_init( struct domain *d) {
     unsigned int i = 0;
 
     /* Init alternate p2m data */
-    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
+    if ( (d->arch.altp2m->altp2m_eptp = alloc_xenheap_page()) == NULL )
     {
         rv = -ENOMEM;
         goto out;
     }
 
     for ( i = 0; i < MAX_EPTP; i++ )
-        d->arch.altp2m_eptp[i] = INVALID_MFN;
+        d->arch.altp2m->altp2m_eptp[i] = INVALID_MFN;
 
     for ( i = 0; i < MAX_ALTP2M; i++ )
     {
-        rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
+        rv = p2m_alloc_table(d->arch.altp2m->altp2m_p2m[i]);
         if ( rv != 0 )
            goto out;
     }
 
-    d->arch.altp2m_active = 0;
+    set_altp2m_active(d, 0);
  out:
     return rv;
 }
@@ -96,16 +96,16 @@ hvm_altp2m_init( struct domain *d) {
 void
 hvm_altp2m_teardown( struct domain *d) {
     unsigned int i = 0;
-    d->arch.altp2m_active = 0;
+    set_altp2m_active(d, 0);
 
-    if ( d->arch.altp2m_eptp )
+    if ( d->arch.altp2m->altp2m_eptp )
     {
-        free_xenheap_page(d->arch.altp2m_eptp);
-        d->arch.altp2m_eptp = NULL;
+        free_xenheap_page(d->arch.altp2m->altp2m_eptp);
+        d->arch.altp2m->altp2m_eptp = NULL;
     }
 
     for ( i = 0; i < MAX_ALTP2M; i++ )
-        p2m_teardown(d->arch.altp2m_p2m[i]);
+        p2m_teardown(d->arch.altp2m->altp2m_p2m[i]);
 }
 
 /*
diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
index 086c8bb..4d17b0a 100644
--- a/xen/arch/x86/mm/mm-locks.h
+++ b/xen/arch/x86/mm/mm-locks.h
@@ -251,8 +251,8 @@ declare_mm_rwlock(p2m);
  */
 
 declare_mm_lock(altp2mlist)
-#define altp2m_list_lock(d)   mm_lock(altp2mlist, &(d)->arch.altp2m_list_lock)
-#define altp2m_list_unlock(d) mm_unlock(&(d)->arch.altp2m_list_lock)
+#define altp2m_list_lock(d)   mm_lock(altp2mlist, &(d)->arch.altp2m->altp2m_list_lock)
+#define altp2m_list_unlock(d) mm_unlock(&(d)->arch.altp2m->altp2m_list_lock)
 
 /* P2M lock (per-altp2m-table)
  *
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index dff34b1..754b660 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1330,14 +1330,14 @@ void setup_ept_dump(void)
 }
 
 void p2m_init_altp2m_helper( struct domain *d, unsigned int i) {
-    struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
+    struct p2m_domain *p2m = d->arch.altp2m->altp2m_p2m[i];
     struct ept_data *ept;
 
     p2m->min_remapped_gfn = INVALID_GFN;
     p2m->max_remapped_gfn = 0;
     ept = &p2m->ept;
     ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m));
-    d->arch.altp2m_eptp[i] = ept_get_eptp(ept);
+    d->arch.altp2m->altp2m_eptp[i] = ept_get_eptp(ept);
 }
 
 unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
@@ -1350,10 +1350,10 @@ unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
 
     for ( i = 0; i < MAX_ALTP2M; i++ )
     {
-        if ( d->arch.altp2m_eptp[i] == INVALID_MFN )
+        if ( d->arch.altp2m->altp2m_eptp[i] == INVALID_MFN )
             continue;
 
-        p2m = d->arch.altp2m_p2m[i];
+        p2m = d->arch.altp2m->altp2m_p2m[i];
         ept = &p2m->ept;
 
         if ( eptp == ept_get_eptp(ept) )
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 90f2d95..70a8b15 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -193,12 +193,15 @@ static void p2m_teardown_altp2m(struct domain *d)
 
     for ( i = 0; i < MAX_ALTP2M; i++ )
     {
-        if ( !d->arch.altp2m_p2m[i] )
+        if ( !d->arch.altp2m->altp2m_p2m[i] )
             continue;
-        p2m = d->arch.altp2m_p2m[i];
+        p2m = d->arch.altp2m->altp2m_p2m[i];
         p2m_free_one(p2m);
-        d->arch.altp2m_p2m[i] = NULL;
+        d->arch.altp2m->altp2m_p2m[i] = NULL;
     }
+
+    if (d->arch.altp2m) 
+        xfree(d->arch.altp2m);
 }
 
 static int p2m_init_altp2m(struct domain *d)
@@ -206,10 +209,12 @@ static int p2m_init_altp2m(struct domain *d)
     unsigned int i;
     struct p2m_domain *p2m;
 
-    mm_lock_init(&d->arch.altp2m_list_lock);
+    d->arch.altp2m = xzalloc(struct altp2m_domain);
+
+    mm_lock_init(&d->arch.altp2m->altp2m_list_lock);
     for ( i = 0; i < MAX_ALTP2M; i++ )
     {
-        d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
+        d->arch.altp2m->altp2m_p2m[i] = p2m = p2m_init_one(d);
         if ( p2m == NULL )
         {
             p2m_teardown_altp2m(d);
@@ -1838,10 +1843,10 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
     if ( altp2m_idx )
     {
         if ( altp2m_idx >= MAX_ALTP2M ||
-             d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN )
+             d->arch.altp2m->altp2m_eptp[altp2m_idx] == INVALID_MFN )
             return -EINVAL;
 
-        ap2m = d->arch.altp2m_p2m[altp2m_idx];
+        ap2m = d->arch.altp2m->altp2m_p2m[altp2m_idx];
     }
 
     switch ( access )
@@ -2280,7 +2285,7 @@ bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx)
 
     altp2m_list_lock(d);
 
-    if ( d->arch.altp2m_eptp[idx] != INVALID_MFN )
+    if ( d->arch.altp2m->altp2m_eptp[idx] != INVALID_MFN )
     {
         if ( idx != vcpu_altp2m(v).p2midx )
         {
@@ -2365,11 +2370,11 @@ void p2m_flush_altp2m(struct domain *d)
 
     for ( i = 0; i < MAX_ALTP2M; i++ )
     {
-        p2m_flush_table(d->arch.altp2m_p2m[i]);
+        p2m_flush_table(d->arch.altp2m->altp2m_p2m[i]);
         /* Uninit and reinit ept to force TLB shootdown */
-        ept_p2m_uninit(d->arch.altp2m_p2m[i]);
-        ept_p2m_init(d->arch.altp2m_p2m[i]);
-        d->arch.altp2m_eptp[i] = INVALID_MFN;
+        ept_p2m_uninit(d->arch.altp2m->altp2m_p2m[i]);
+        ept_p2m_init(d->arch.altp2m->altp2m_p2m[i]);
+        d->arch.altp2m->altp2m_eptp[i] = INVALID_MFN;
     }
 
     altp2m_list_unlock(d);
@@ -2384,7 +2389,7 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
 
     altp2m_list_lock(d);
 
-    if ( d->arch.altp2m_eptp[idx] == INVALID_MFN )
+    if ( d->arch.altp2m->altp2m_eptp[idx] == INVALID_MFN )
     {
         p2m_init_altp2m_helper(d, idx);
         rc = 0;
@@ -2403,7 +2408,7 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
 
     for ( i = 0; i < MAX_ALTP2M; i++ )
     {
-        if ( d->arch.altp2m_eptp[i] != INVALID_MFN )
+        if ( d->arch.altp2m->altp2m_eptp[i] != INVALID_MFN )
             continue;
 
         p2m_init_altp2m_helper(d, i);
@@ -2429,17 +2434,17 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
 
     altp2m_list_lock(d);
 
-    if ( d->arch.altp2m_eptp[idx] != INVALID_MFN )
+    if ( d->arch.altp2m->altp2m_eptp[idx] != INVALID_MFN )
     {
-        p2m = d->arch.altp2m_p2m[idx];
+        p2m = d->arch.altp2m->altp2m_p2m[idx];
 
         if ( !_atomic_read(p2m->active_vcpus) )
         {
-            p2m_flush_table(d->arch.altp2m_p2m[idx]);
+            p2m_flush_table(d->arch.altp2m->altp2m_p2m[idx]);
             /* Uninit and reinit ept to force TLB shootdown */
-            ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
-            ept_p2m_init(d->arch.altp2m_p2m[idx]);
-            d->arch.altp2m_eptp[idx] = INVALID_MFN;
+            ept_p2m_uninit(d->arch.altp2m->altp2m_p2m[idx]);
+            ept_p2m_init(d->arch.altp2m->altp2m_p2m[idx]);
+            d->arch.altp2m->altp2m_eptp[idx] = INVALID_MFN;
             rc = 0;
         }
     }
@@ -2463,7 +2468,7 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx)
 
     altp2m_list_lock(d);
 
-    if ( d->arch.altp2m_eptp[idx] != INVALID_MFN )
+    if ( d->arch.altp2m->altp2m_eptp[idx] != INVALID_MFN )
     {
         for_each_vcpu( d, v )
             if ( idx != vcpu_altp2m(v).p2midx )
@@ -2494,11 +2499,11 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     unsigned int page_order;
     int rc = -EINVAL;
 
-    if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == INVALID_MFN )
+    if ( idx >= MAX_ALTP2M || d->arch.altp2m->altp2m_eptp[idx] == INVALID_MFN )
         return rc;
 
     hp2m = p2m_get_hostp2m(d);
-    ap2m = d->arch.altp2m_p2m[idx];
+    ap2m = d->arch.altp2m->altp2m_p2m[idx];
 
     p2m_lock(ap2m);
 
@@ -2589,10 +2594,10 @@ void p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
 
     for ( i = 0; i < MAX_ALTP2M; i++ )
     {
-        if ( d->arch.altp2m_eptp[i] == INVALID_MFN )
+        if ( d->arch.altp2m->altp2m_eptp[i] == INVALID_MFN )
             continue;
 
-        p2m = d->arch.altp2m_p2m[i];
+        p2m = d->arch.altp2m->altp2m_p2m[i];
         m = get_gfn_type_access(p2m, gfn_x(gfn), &t, &a, 0, NULL);
 
         /* Check for a dropped page that may impact this altp2m */
@@ -2613,10 +2618,10 @@ void p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
                 for ( i = 0; i < MAX_ALTP2M; i++ )
                 {
                     if ( i == last_reset_idx ||
-                         d->arch.altp2m_eptp[i] == INVALID_MFN )
+                         d->arch.altp2m->altp2m_eptp[i] == INVALID_MFN )
                         continue;
 
-                    p2m = d->arch.altp2m_p2m[i];
+                    p2m = d->arch.altp2m->altp2m_p2m[i];
                     p2m_lock(p2m);
                     p2m_reset_altp2m(p2m);
                     p2m_unlock(p2m);
diff --git a/xen/include/asm-x86/altp2m.h b/xen/include/asm-x86/altp2m.h
index 7ce047d..eca0ec7 100644
--- a/xen/include/asm-x86/altp2m.h
+++ b/xen/include/asm-x86/altp2m.h
@@ -24,7 +24,12 @@
 /* Alternate p2m HVM on/off per domain */
 static inline bool_t altp2m_active(const struct domain *d)
 {
-    return d->arch.altp2m_active;
+    return d->arch.altp2m->altp2m_active;
+}
+
+static inline void set_altp2m_active(const struct domain *d, bool_t v)
+{
+    d->arch.altp2m->altp2m_active = v;
 }
 
 /* Alternate p2m VCPU */
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 783fa4f..614dd40 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -274,6 +274,13 @@ struct monitor_write_data {
     uint64_t cr4;
 };
 
+struct altp2m_domain {
+    bool_t altp2m_active;
+    struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
+    mm_lock_t altp2m_list_lock;
+    uint64_t *altp2m_eptp;
+};
+
 struct arch_domain
 {
     struct page_info *perdomain_l3_pg;
@@ -320,10 +327,13 @@ struct arch_domain
     mm_lock_t nested_p2m_lock;
 
     /* altp2m: allow multiple copies of host p2m */
+    /*
     bool_t altp2m_active;
     struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
     mm_lock_t altp2m_list_lock;
-    uint64_t *altp2m_eptp;
+    uint64_t *altp2m_eptp; 
+    */
+    struct altp2m_domain *altp2m;
 
     /* NB. protected by d->event_lock and by irq_desc[irq].lock */
     struct radix_tree_root irq_pirq;
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index d7c8c12..3185ec7 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -776,7 +776,7 @@ static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v)
 
     BUG_ON(index >= MAX_ALTP2M);
 
-    return v->domain->arch.altp2m_p2m[index];
+    return v->domain->arch.altp2m->altp2m_p2m[index];
 }
 
 /* Switch alternate p2m for a single vcpu */
-- 
1.9.1


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

Re: [PATCH v1 Altp2m cleanup 3/3] Making altp2m struct dynamically allocated.

[Jan Beulich]

>>> On 21.06.16 at 18:04, <paul.c.lai@intel.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5228,7 +5228,7 @@ static int do_altp2m_op(
>  
>      if ( (a.cmd != HVMOP_altp2m_get_domain_state) &&
>           (a.cmd != HVMOP_altp2m_set_domain_state) &&
> -         !d->arch.altp2m_active )
> +         ! altp2m_active(d) )

Stray blank.

> @@ -5262,11 +5262,11 @@ static int do_altp2m_op(
>              break;
>          }
>  
> -        ostate = d->arch.altp2m_active;
> -        d->arch.altp2m_active = !!a.u.domain_state.state;
> +        ostate = altp2m_active(d);
> +	set_altp2m_active(d, !!a.u.domain_state.state);

Bogus tab indentation.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -193,12 +193,15 @@ static void p2m_teardown_altp2m(struct domain *d)
>  
>      for ( i = 0; i < MAX_ALTP2M; i++ )
>      {
> -        if ( !d->arch.altp2m_p2m[i] )
> +        if ( !d->arch.altp2m->altp2m_p2m[i] )
>              continue;
> -        p2m = d->arch.altp2m_p2m[i];
> +        p2m = d->arch.altp2m->altp2m_p2m[i];
>          p2m_free_one(p2m);
> -        d->arch.altp2m_p2m[i] = NULL;
> +        d->arch.altp2m->altp2m_p2m[i] = NULL;
>      }
> +
> +    if (d->arch.altp2m) 

Missing blanks.

> +        xfree(d->arch.altp2m);

But the conditional is pointless anyway.

> @@ -206,10 +209,12 @@ static int p2m_init_altp2m(struct domain *d)
>      unsigned int i;
>      struct p2m_domain *p2m;
>  
> -    mm_lock_init(&d->arch.altp2m_list_lock);
> +    d->arch.altp2m = xzalloc(struct altp2m_domain);
> +
> +    mm_lock_init(&d->arch.altp2m->altp2m_list_lock);

Missing error check.

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -274,6 +274,13 @@ struct monitor_write_data {
>      uint64_t cr4;
>  };
>  
> +struct altp2m_domain {
> +    bool_t altp2m_active;
> +    struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
> +    mm_lock_t altp2m_list_lock;
> +    uint64_t *altp2m_eptp;
> +};

No point prefixing all the fields with altp2m_. And also the structure
now doesn't belong here anymore - it should move to e.g. p2m.h.

> @@ -320,10 +327,13 @@ struct arch_domain
>      mm_lock_t nested_p2m_lock;
>  
>      /* altp2m: allow multiple copies of host p2m */
> +    /*
>      bool_t altp2m_active;
>      struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
>      mm_lock_t altp2m_list_lock;
> -    uint64_t *altp2m_eptp;
> +    uint64_t *altp2m_eptp; 
> +    */

What's the purpose of this comment?

Jan


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

Re: [PATCH v1 Altp2m cleanup 0/3] Cleaning up altp2m code

[Jan Beulich]

>>> On 21.06.16 at 18:04, <paul.c.lai@intel.com> wrote:
> Cleaning up altp2m code per request of xen-devel mailing list.
> 
> Paul Lai (3):
>   altp2m cleanup work
>   Move altp2m specific functions to altp2m files.
>   Making altp2m struct dynamically allocated.

Much appreciated. But having peeked into patch 1 (as the title didn't
really make clear what kind of cleanup that is) I don't think that's the
cleanup that was mainly requested: Iirc the main work item was to
get rid of the multitude of function parameters the set_entry() hook
takes.

Jan


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

Re: [PATCH v1 Altp2m cleanup 0/3] Cleaning up altp2m code

[Lai, Paul C]

Jan:

Ok... adding back xen-devel.

Thanks for the pointer to the thread.

I'm looking at the existing Xen code and the changes mentioned by George (and supported by Tim) are already in.  The patch suggested by George is
  http://lists.xenproject.org/archives/html/xen-devel/2015-07/txtEYYYL6ATei.txt
from George's post of  http://lists.xenproject.org/archives/html/xen-devel/2015-07/msg01088.html

The current code has the following snippets, which match/mirror the intent of the above referenced patch.

xen/arch/x86/mm/mem_sharing.c:
...
+        bool_t sve;
 
         if ( atomic_read(&d->shr_pages) == 0 )
             break;
-        mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);
+        mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);

xen/arch/x86/mm/p2m-ept.c:
.... 
    if ( sve != -1 )
        new_entry.suppress_ve = !!sve;
    else
        new_entry.suppress_ve = is_epte_valid(&old_entry) ?
                                    old_entry.suppress_ve : 1;
...

[WRT side note: it's easier to see what your referring to if you present what you see.  If I search, I'm guessing at what you're referring to.  Often (at least for me), I end up have two different conversations.]

Regards, -Paul

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@suse.com] 
Sent: Wednesday, June 22, 2016 8:54 AM
To: Lai, Paul C <paul.c.lai@intel.com>
Cc: Sahita, Ravi <ravi.sahita@intel.com>
Subject: RE: [PATCH v1 Altp2m cleanup 0/3] Cleaning up altp2m code

>>> On 22.06.16 at 17:26, <paul.c.lai@intel.com> wrote:
> Dropping Xen-devel.

Not sure why, but anyway.

> Jan/Ravi: 
> 
> Please send me reminder for the " main work item was to get rid of the 
> multitude of function parameters the set_entry()".  This wasn't in the 
> instructions I (if I recall correctly) was given.

See for example
http://lists.xenproject.org/archives/html/xen-devel/2015-07/msg01331.html
and the surrounding thread, as well as replies to later versions of this same patch.

(As a side note, it's not clear to me why you made me search xen-devel for this discussion, when you could have just as easily done this yourself.)

Jan


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