Re: [Xen-devel] [PATCH v4 02/39] arm/p2m: Add first altp2m HVMOP stubs

[Julien Grall]

Hi Sergej,

On 30/08/17 19:32, Sergej Proskurin wrote:

This commit copies and extends the altp2m-related code from x86 to ARM.
Functions that are no yet supported notify the caller or print a BUG
message stating their absence.


I am still concerned on the locking differing between x86 and Arm 
(likely the former is wrong) and for maintain POV in the future.


Last year you said you were working on getting do_altp2m_op common 
between x86 and Arm. What's the status?



diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index a56b3fe3fb..042bdda979 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -31,6 +31,99 @@
  
  #include 
  
+#include 

+
+static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+struct xen_hvm_altp2m_op a;
+struct domain *d = NULL;
+uint64_t mode;
+int rc = 0;
+
+if ( copy_from_guest(, arg, 1) )
+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) )


That exactly support my view above. You resent a series after a year and 
don't even look at what changed in x86.


I don't expect any better improvement as people will add more features 
in each side.


[...]


diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 8dfc1d1ec2..0991a0a79d 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -145,6 +145,9 @@ struct arch_domain
  struct {
  uint8_t privileged_call_enabled : 1;
  } monitor;
+
+/* altp2m: allow multiple copies of the host's p2m */
+bool altp2m_active;


Does it have to be in arch_domain? Can't this be done in common code?


  }  __cacheline_aligned;
  
  struct arch_vcpu




Cheers,

--
Julien Grall

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

[Xen-devel] [PATCH v4 02/39] arm/p2m: Add first altp2m HVMOP stubs

[Sergej Proskurin]

This commit copies and extends the altp2m-related code from x86 to ARM.
Functions that are no yet supported notify the caller or print a BUG
message stating their absence.

Currently, we prohibit concurrent access of the altp2m interface by
locking the entire domain. As stated in the provided TODO statement,
future implementation should determine, which HVMOPs can be executed
concurrently.

Also, the struct arch_domain is extended with the altp2m_active
attribute, representing the current altp2m activity configuration of the
domain.

Signed-off-by: Sergej Proskurin 
---
Cc: Stefano Stabellini 
Cc: Julien Grall 
---
v2: Removed altp2m command-line option: Guard through HVM_PARAM_ALTP2M.
Removed not used altp2m helper stubs in altp2m.h.

v3: Cosmetic fixes.

Added domain lock in "do_altp2m_op" to avoid concurrent execution of
altp2m-related HVMOPs.

Added check making sure that HVM_PARAM_ALTP2M is set before
execution of altp2m-related HVMOPs.

v4: Cosmetic fixes.

Added a TODO proposing to determine, which HVMOPs can be executed
concurrently instead of locking the entire domain and hence
prohibiting concurrent access of the altp2m interface.

Adjust to the current code base by explicitly checking whether
altp2m is disabled.

Change the type bool_t to bool of the field altp2m_active in struct
arch_domain.
---
 xen/arch/arm/hvm.c   | 97 
 xen/include/asm-arm/altp2m.h |  4 +-
 xen/include/asm-arm/domain.h |  3 ++
 3 files changed, 102 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index a56b3fe3fb..042bdda979 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -31,6 +31,99 @@
 
 #include 
 
+#include 
+
+static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+struct xen_hvm_altp2m_op a;
+struct domain *d = NULL;
+uint64_t mode;
+int rc = 0;
+
+if ( copy_from_guest(, arg, 1) )
+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) )
+return -EINVAL;
+
+d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ?
+rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain();
+
+if ( d == NULL )
+return -ESRCH;
+
+/*
+ * TODO: We prohibit concurrent access of the altp2m interface by locking
+ * the entire domain. Determine which HVMOPs can be executed concurrently.
+ */
+
+/* Prevent concurrent execution of the following HVMOPs. */
+domain_lock(d);
+
+if ( (a.cmd != HVMOP_altp2m_get_domain_state) &&
+ (a.cmd != HVMOP_altp2m_set_domain_state) &&
+ !altp2m_active(d) )
+{
+rc = -EOPNOTSUPP;
+goto out;
+}
+
+mode = d->arch.hvm_domain.params[HVM_PARAM_ALTP2M];
+
+if ( XEN_ALTP2M_disabled == mode )
+{
+rc = -EINVAL;
+goto out;
+}
+
+if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d, mode, a.cmd)) )
+goto out;
+
+switch ( a.cmd )
+{
+case HVMOP_altp2m_get_domain_state:
+rc = -EOPNOTSUPP;
+break;
+
+case HVMOP_altp2m_set_domain_state:
+rc = -EOPNOTSUPP;
+break;
+
+case HVMOP_altp2m_vcpu_enable_notify:
+rc = -EOPNOTSUPP;
+break;
+
+case HVMOP_altp2m_create_p2m:
+rc = -EOPNOTSUPP;
+break;
+
+case HVMOP_altp2m_destroy_p2m:
+rc = -EOPNOTSUPP;
+break;
+
+case HVMOP_altp2m_switch_p2m:
+rc = -EOPNOTSUPP;
+break;
+
+case HVMOP_altp2m_set_mem_access:
+rc = -EOPNOTSUPP;
+break;
+
+case HVMOP_altp2m_change_gfn:
+rc = -EOPNOTSUPP;
+break;
+}
+
+out:
+domain_unlock(d);
+rcu_unlock_domain(d);
+
+return rc;
+}
+
 long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
 long rc = 0;
@@ -79,6 +172,10 @@ long do_hvm_op(unsigned long op, 
XEN_GUEST_HANDLE_PARAM(void) arg)
 rc = -EINVAL;
 break;
 
+case HVMOP_altp2m:
+rc = do_altp2m_op(arg);
+break;
+
 default:
 {
 gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op);
diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
index a87747a291..0711796123 100644
--- a/xen/include/asm-arm/altp2m.h
+++ b/xen/include/asm-arm/altp2m.h
@@ -2,6 +2,7 @@
  * Alternate p2m
  *
  * Copyright (c) 2014, Intel Corporation.
+ * Copyright (c) 2016, Sergej Proskurin .
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -24,8 +25,7 @@
 /* Alternate p2m on/off per domain */
 static inline bool_t altp2m_active(const struct domain *d)
 {
-/* Not implemented on ARM. */
-return 0;
+