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

Re: [Xen-devel] [PATCH 04/18] arm/altp2m: Add altp2m init/teardown routines.





On 04/07/16 17:40, Sergej Proskurin wrote:
On 07/04/2016 05:17 PM, Julien Grall wrote:
On 04/07/16 12:45, Sergej Proskurin wrote:

[...]

+static struct p2m_domain *p2m_init_one(struct domain *d)
+{
+    struct p2m_domain *p2m = xzalloc(struct p2m_domain);
+
+    if ( !p2m )
+        return NULL;
+
+    if ( p2m_initialise(d, p2m) )
+        goto free_p2m;
+
+    return p2m;
+
+free_p2m:
+    xfree(p2m);
+    return NULL;
+}
+
+static void p2m_teardown_hostp2m(struct domain *d)

Why does p2m_teardown_hostp2m not use p2m_teardown_one to teardown the
p2m? Assuming xfree(p2m) is move out of the function.


I believe you mean p2m_free_one: The p2m_teardown_hostp2m might use the
same function but would require the call p2m_free_vmid(d) to be called
outside of p2m_free_one as well. This would require another acquisition
of the p2m->lock. Just to be sure, I did not want to split the teardown
process into two atomic executions. If you believe that it is safe to
do, I will gladly change the code and re-use the code from p2m_free_one
in p2m_teardown_hostp2m.

Looking at the caller of p2m_teardown, I don't think the lock is necessary because nobody can use the P2M anymore when this function is called.

[...]

+        {
+            mfn = _mfn(page_to_mfn(pg));
+            clear_domain_page(mfn);
+            free_domheap_page(pg);
+        }

+    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
+    {
+        mfn = _mfn(page_to_mfn(p2m->root) + i);
+        clear_domain_page(mfn);
+    }
+    free_domheap_pages(p2m->root, P2M_ROOT_ORDER);
       p2m->root = NULL;

       p2m_free_vmid(d);
@@ -1422,7 +1506,7 @@ void p2m_teardown(struct domain *d)
       spin_unlock(&p2m->lock);
   }

-int p2m_init(struct domain *d)
+static int p2m_init_hostp2m(struct domain *d)

Why does p2m_init_hostp2m not use p2m_init_one to initialize the p2m?


We dynamically allocate altp2ms. Also, the initialization of both the
altp2ms and hostp2m slightly differs (see VMID allocation). I could
rewrite the initialization function to be used for both, the hostp2m and
altp2m structs. Especially, if you say that we do not associate domains
with VMIDs (see your upper question).

I always prefer to see the code rewritten rather than duplicating code. The latter makes harder to fix bug when it is spread in multiple place.

[...]

+    uint64_t altp2m_vttbr[MAX_ALTP2M];
   }  __cacheline_aligned;

   struct arch_vcpu
diff --git a/xen/include/asm-arm/hvm/hvm.h
b/xen/include/asm-arm/hvm/hvm.h
index 96c455c..28d5298 100644
--- a/xen/include/asm-arm/hvm/hvm.h
+++ b/xen/include/asm-arm/hvm/hvm.h
@@ -19,6 +19,18 @@
   #ifndef __ASM_ARM_HVM_HVM_H__
   #define __ASM_ARM_HVM_HVM_H__

+struct vttbr_data {

This structure should not be part of hvm.h but processor.h. Also, I
would rename it to simply vttbr.


Ok, I will move it. The struct was named this way to be the counterpart
to struct ept_data. Do you still think, we should introduce naming
differences for basically the same register at this point?

Yes, we are talking about two distinct architectures. If you look at ept_data, it stores more than the page table register. Hence the name.

[...]

+
   #define paddr_bits PADDR_BITS

   /* Holds the bit size of IPAs in p2m tables.  */
@@ -17,6 +20,11 @@ struct domain;

   extern void memory_type_changed(struct domain *);

+typedef enum {
+    p2m_host,
+    p2m_alternate,
+} p2m_class_t;
+
   /* Per-p2m-table state */
   struct p2m_domain {
       /* Lock that protects updates to the p2m */
@@ -66,6 +74,18 @@ struct p2m_domain {
       /* Radix tree to store the p2m_access_t settings as the pte's
don't have
        * enough available bits to store this information. */
       struct radix_tree_root mem_access_settings;
+
+    /* Alternate p2m: count of vcpu's currently using this p2m. */
+    atomic_t active_vcpus;
+
+    /* Choose between: host/alternate */
+    p2m_class_t p2m_class;

Is there any reason to have this field? It is set but never used.


Actually it is used by p2m_is_altp2m and p2m_is_hostp2m (e.g. see assert
in p2m_flush_table).

Right. Sorry, I didn't spot this call.

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®.