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

Re: [PATCH 6/6] x86/mem-paging: consistently use gfn_t

Hi Jan,

On 20/04/2020 07:03, Jan Beulich wrote:
On 18.04.2020 13:14, Julien Grall wrote:
On 16/04/2020 16:48, Jan Beulich wrote:
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2151,16 +2151,17 @@ static int mod_l1_entry(l1_pgentry_t *pl
                paging_mode_translate(pg_dom) )
               p2m_type_t p2mt;
+            unsigned long gfn = l1e_get_pfn(nl1e);

How about making gfn a gfn_t directly? This would avoid code churn when...

               p2m_query_t q = l1e_get_flags(nl1e) & _PAGE_RW ?
                               P2M_ALLOC | P2M_UNSHARE : P2M_ALLOC;
   -            page = get_page_from_gfn(pg_dom, l1e_get_pfn(nl1e), &p2mt, q);
+            page = get_page_from_gfn(pg_dom, gfn, &p2mt, q);

... I am going to convert get_page_from_gfn() to use typesafe gfn. See [1].

Ah, yes, I can certainly do so.

@@ -89,16 +88,15 @@ void p2m_mem_paging_drop_page(struct dom
    * already sent to the pager. In this case the caller has to try again until 
    * gfn is fully paged in again.
-void p2m_mem_paging_populate(struct domain *d, unsigned long gfn_l)
+void p2m_mem_paging_populate(struct domain *d, gfn_t gfn)
       struct vcpu *v = current;
       vm_event_request_t req = {
           .reason = VM_EVENT_REASON_MEM_PAGING,
-        .u.mem_paging.gfn = gfn_l
+        .u.mem_paging.gfn = gfn_x(gfn)
       p2m_type_t p2mt;
       p2m_access_t a;
-    gfn_t gfn = _gfn(gfn_l);
       mfn_t mfn;
       struct p2m_domain *p2m = p2m_get_hostp2m(d);
       int rc = vm_event_claim_slot(d, d->vm_event_paging);
@@ -107,7 +105,7 @@ void p2m_mem_paging_populate(struct doma
       if ( rc == -EOPNOTSUPP )
           gdprintk(XENLOG_ERR, "Dom%d paging gfn %lx yet no ring in place\n",
-                 d->domain_id, gfn_l);
+                 d->domain_id, gfn_x(gfn));

Please use PRI_gfn in the format string to match the argument change.

I can do this, but iirc in one of my replies to one of your changes
I've indicated I'm not fully convinced of such changes.

I guess you are referring to [2]. The discussion was quite different, we were arguing whether PRI_mfn could be used for other value than mfn_x(mfn). But then you said you were happy with PRI_xen_pfn.

Aside the return type of gfn_x(gfn) argument, if we use %PRI_gfn then we can finally have a consistent way to print a GFN and easily change it.

[1] https://lore.kernel.org/xen-devel/20200322161418.31606-18-julien@xxxxxxx/

Looking over this I notice (only now) that this patch is not
consistent with its dropping of # in PRI_[gm]fn uses: You
don't drop them in e.g. Viridian's enable_hypercall_page(),
but you do in e.g. guest_wrmsr_xen(). Dropping is The Right
Thing To Do (tm), so please do so uniformly.



[2] <2be87441-05a6-6b58-23e3-da467230ffe7@xxxxxxx>


Julien Grall



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