[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 01/25] arm/altp2m: Add first altp2m HVMOP stubs.





On 04/08/16 17:22, Sergej Proskurin wrote:

On 08/04/2016 06:04 PM, Julien Grall wrote:


On 04/08/16 17:01, Sergej Proskurin wrote:
Hi Julien,


On 08/03/2016 06:54 PM, Julien Grall wrote:
Hello Sergej,

On 01/08/16 18:10, Sergej Proskurin wrote:
This commit moves 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.

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 <proskurin@xxxxxxxxxxxxx>
---
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>
---
v2: Removed altp2m command-line option: Guard through
HVM_PARAM_ALTP2M.
    Removed not used altp2m helper stubs in altp2m.h.
---
 xen/arch/arm/hvm.c           | 79
++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/altp2m.h |  4 +--
 xen/include/asm-arm/domain.h |  3 ++
 3 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index d999bde..eb524ae 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -32,6 +32,81 @@

 #include <asm/hypercall.h>

+#include <asm/altp2m.h>
+
+static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)
+{
+    struct xen_hvm_altp2m_op a;
+    struct domain *d = NULL;
+    int rc = 0;
+
+    if ( copy_from_guest(&a, 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;
+
+    if ( (a.cmd != HVMOP_altp2m_get_domain_state) &&
+         (a.cmd != HVMOP_altp2m_set_domain_state) &&
+         !d->arch.altp2m_active )

Why not using altp2m_active(d) here?


I have already changed that within the next patch version. Thank you.

Also this check looks quite racy. What does prevent another CPU to
disable altp2m at the same time? How the code would behave?


Thank you. I will protect this part with the altp2m_lock.

I have noticed that you use the altp2m_lock (it is a spinlock) in
multiple places. So you will serialize a lot of code. Is it fine for you?


I would need to move the lock from altp2m_init_by_id to the outside.
This would not lock much more code as it already does. Apart from that,
since activating/deactivating altp2m on a specific domain should be used
very rarely (including the first time when no altp2m structures are
initialized), it is fine to me. Unless, you would like me to use a
different lock instead?

I don't know, it was an open question. There are a couple of places where you may need to add lock atlp2m_lock (such altp2m_copy_lazy), so you would serialize all the access. If you care about performance, then you may want to use a different lock or method of locking (such as read-write-lock).

Regards,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.