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

Re: [Xen-devel] [PATCH 08/15] xen/x86: p2m: Use typesafe gfn for the P2M callbacks get_entry and set_entry



Hi,

On 13/09/2017 20:08, Razvan Cojocaru wrote:
@@ -142,7 +142,7 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
                           vm_event_request_t **req_ptr)
 {
     struct vcpu *v = current;
-    unsigned long gfn = gpa >> PAGE_SHIFT;
+    gfn_t gfn = gaddr_to_gfn(gpa);
     struct domain *d = v->domain;
     struct p2m_domain *p2m = NULL;
     mfn_t mfn;
@@ -215,7 +215,7 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
         *req_ptr = req;

         req->reason = VM_EVENT_REASON_MEM_ACCESS;
-        req->u.mem_access.gfn = gfn;
+        req->u.mem_access.gfn = gfn_x(gfn);
         req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1);
         if ( npfec.gla_valid )
         {
@@ -247,7 +247,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct 
p2m_domain *hp2m,
     unsigned long gfn_l = gfn_x(gfn);

I'd drop gfn_l completely here and just use gfn_x(gfn) in the two places
below where it's used. It would get rid of a line of code in this
function. But I wouldn't hold the patch over this, so if nobody else has
comments it's fine as is.

Technically more clean-up could be done in every functions. If you noticed it, I kept the changes minimal because this is already a boring but necessary job and I really don't want to explore all the possible clean-up in each single function... Feel free to do more clean-up.

     int rc;

-    mfn = ap2m->get_entry(ap2m, gfn_l, &t, &old_a, 0, NULL, NULL);
+    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);

     /* Check host p2m if no valid entry in alternate */
     if ( !mfn_valid(mfn) )
@@ -264,16 +264,16 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct 
p2m_domain *hp2m,
         if ( page_order != PAGE_ORDER_4K )
         {
             unsigned long mask = ~((1UL << page_order) - 1);
-            unsigned long gfn2_l = gfn_l & mask;
+            gfn_t gfn2 = _gfn(gfn_l & mask);
             mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);

-            rc = ap2m->set_entry(ap2m, gfn2_l, mfn2, page_order, t, old_a, 1);
+            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
             if ( rc )
                 return rc;
         }
     }

-    return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
+    return ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a,
                          (current->domain != d));

If you need to send V2, could you please also indent the last line so
that '(' is under the 'a' in 'ap2m'? You don't have to, but it'd be a
coding style fix coming in on top of this patch.

Will do.

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 23c0518733..dff214cf7b 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -674,11 +674,12 @@ bool_t ept_handle_misconfig(uint64_t gpa)
  * Returns: 0 for success, -errno for failure
  */
 static int
-ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
+ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_t, mfn_t mfn,
               unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma,
               int sve)
 {
     ept_entry_t *table, *ept_entry = NULL;
+    unsigned long gfn = gfn_x(gfn_t);

Should we call this gfn_l, as done above?

     unsigned long gfn_remainder = gfn;
     unsigned int i, target = order / EPT_TABLE_ORDER;
     unsigned long fn_mask = !mfn_eq(mfn, INVALID_MFN) ? (gfn | mfn_x(mfn)) : 
gfn;
@@ -910,11 +911,12 @@ out:

 /* Read ept p2m entries */
 static mfn_t ept_get_entry(struct p2m_domain *p2m,
-                           unsigned long gfn, p2m_type_t *t, p2m_access_t* a,
+                           gfn_t gfn_t, p2m_type_t *t, p2m_access_t* a,
                            p2m_query_t q, unsigned int *page_order,
                            bool_t *sve)
 {
     ept_entry_t *table = 
map_domain_page(_mfn(pagetable_get_pfn(p2m_get_pagetable(p2m))));
+    unsigned long gfn = gfn_x(gfn_t);

gfn_l here too?

[...]

diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 0e63d6ed11..57878b1886 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -479,12 +479,13 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)

 /* Returns: 0 for success, -errno for failure */
 static int
-p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
+p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_t, mfn_t mfn,
                  unsigned int page_order, p2m_type_t p2mt, p2m_access_t p2ma,
                  int sve)
 {
     /* XXX -- this might be able to be faster iff current->domain == d */
     void *table;
+    unsigned long gfn = gfn_x(gfn_t);

gfn_l?

     unsigned long i, gfn_remainder = gfn;
     l1_pgentry_t *p2m_entry, entry_content;
     /* Intermediate table to free if we're replacing it with a superpage. */
@@ -731,11 +732,12 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long 
gfn, mfn_t mfn,
 }

 static mfn_t
-p2m_pt_get_entry(struct p2m_domain *p2m, unsigned long gfn,
+p2m_pt_get_entry(struct p2m_domain *p2m, gfn_t gfn_t,
                  p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
                  unsigned int *page_order, bool_t *sve)
 {
     mfn_t mfn;
+    unsigned long gfn = gfn_x(gfn_t);

gfn_l?

If you do that you would have to rename all gfn to gfn_l in the function increase a bit more the size of this boring patch. The right solution would be to make the rest of the code typesafe, but that's not my plan. I have done enough typesafe clean-up myself, so I will leave it with

unsigned long gfn = gfn_x(gfn_);

Until someone step up and decide to do further clean-up.

Cheers,

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