diff mbox series

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

Message ID 224337b8-98b4-b0f6-a57a-6f508ffa6838@suse.com (mailing list archive)
State Superseded
Headers show
Series x86/mem-paging: misc cleanup | expand

Commit Message

Jan Beulich April 16, 2020, 3:48 p.m. UTC
Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Julien Grall April 18, 2020, 11:14 a.m. UTC | #1
Hi Jan,

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

> @@ -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 the
>    * 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.

Cheers,

[1] 
https://lore.kernel.org/xen-devel/20200322161418.31606-18-julien@xen.org/
Jan Beulich April 20, 2020, 6:03 a.m. UTC | #2
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 the
>>    * 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.

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

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.

Jan
Julien Grall April 20, 2020, 9:57 a.m. UTC | #3
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 the
>>>     * 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@xen.org/
> 
> 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.

Ok.

Cheers,

[2] <2be87441-05a6-6b58-23e3-da467230ffe7@xen.org>

> 
> Jan
>
diff mbox series

Patch

--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -278,7 +278,7 @@  static int set_mem_type(struct domain *d
         if ( p2m_is_paging(t) )
         {
             put_gfn(d, pfn);
-            p2m_mem_paging_populate(d, pfn);
+            p2m_mem_paging_populate(d, _gfn(pfn));
             return -EAGAIN;
         }
 
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1968,7 +1968,7 @@  int hvm_hap_nested_page_fault(paddr_t gp
      * locks in such circumstance.
      */
     if ( paged )
-        p2m_mem_paging_populate(currd, gfn);
+        p2m_mem_paging_populate(currd, _gfn(gfn));
 
     if ( sharing_enomem )
     {
@@ -3199,7 +3199,7 @@  enum hvm_translation_result hvm_translat
     if ( p2m_is_paging(p2mt) )
     {
         put_page(page);
-        p2m_mem_paging_populate(v->domain, gfn_x(gfn));
+        p2m_mem_paging_populate(v->domain, gfn);
         return HVMTRANS_gfn_paged_out;
     }
     if ( p2m_is_shared(p2mt) )
--- 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);
             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);
 
             if ( p2m_is_paged(p2mt) )
             {
                 if ( page )
                     put_page(page);
-                p2m_mem_paging_populate(pg_dom, l1e_get_pfn(nl1e));
+                p2m_mem_paging_populate(pg_dom, _gfn(gfn));
                 return -ENOENT;
             }
 
@@ -3982,7 +3983,7 @@  long do_mmu_update(
                     put_page(page);
                 if ( p2m_is_paged(p2mt) )
                 {
-                    p2m_mem_paging_populate(pt_owner, gmfn);
+                    p2m_mem_paging_populate(pt_owner, _gfn(gmfn));
                     rc = -ENOENT;
                 }
                 else
--- a/xen/arch/x86/mm/hap/guest_walk.c
+++ b/xen/arch/x86/mm/hap/guest_walk.c
@@ -68,7 +68,7 @@  unsigned long hap_p2m_ga_to_gfn(GUEST_PA
         *pfec = PFEC_page_paged;
         if ( top_page )
             put_page(top_page);
-        p2m_mem_paging_populate(p2m->domain, cr3 >> PAGE_SHIFT);
+        p2m_mem_paging_populate(p2m->domain, _gfn(PFN_DOWN(cr3)));
         return gfn_x(INVALID_GFN);
     }
     if ( p2m_is_shared(p2mt) )
@@ -109,7 +109,7 @@  unsigned long hap_p2m_ga_to_gfn(GUEST_PA
         {
             ASSERT(p2m_is_hostp2m(p2m));
             *pfec = PFEC_page_paged;
-            p2m_mem_paging_populate(p2m->domain, gfn_x(gfn));
+            p2m_mem_paging_populate(p2m->domain, gfn);
             return gfn_x(INVALID_GFN);
         }
         if ( p2m_is_shared(p2mt) )
--- a/xen/arch/x86/mm/mem_paging.c
+++ b/xen/arch/x86/mm/mem_paging.c
@@ -36,12 +36,11 @@ 
  * released by the guest. The pager is supposed to drop its reference of the
  * gfn.
  */
-void p2m_mem_paging_drop_page(struct domain *d, unsigned long gfn,
-                              p2m_type_t p2mt)
+void p2m_mem_paging_drop_page(struct domain *d, gfn_t gfn, p2m_type_t p2mt)
 {
     vm_event_request_t req = {
         .reason = VM_EVENT_REASON_MEM_PAGING,
-        .u.mem_paging.gfn = gfn
+        .u.mem_paging.gfn = gfn_x(gfn)
     };
 
     /*
@@ -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 the
  * 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));
         /* Prevent the vcpu from faulting repeatedly on the same gfn */
         if ( v->domain == d )
             vcpu_pause_nosync(v);
@@ -218,13 +216,12 @@  void p2m_mem_paging_resume(struct domain
  * Once the p2mt is changed the page is readonly for the guest.  On success the
  * pager can write the page contents to disk and later evict the page.
  */
-static int nominate(struct domain *d, unsigned long gfn_l)
+static int nominate(struct domain *d, gfn_t gfn)
 {
     struct page_info *page;
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     p2m_type_t p2mt;
     p2m_access_t a;
-    gfn_t gfn = _gfn(gfn_l);
     mfn_t mfn;
     int ret = -EBUSY;
 
@@ -279,12 +276,11 @@  static int nominate(struct domain *d, un
  * could evict it, eviction can not be done either. In this case the gfn is
  * still backed by a mfn.
  */
-static int evict(struct domain *d, unsigned long gfn_l)
+static int evict(struct domain *d, gfn_t gfn)
 {
     struct page_info *page;
     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 ret = -EBUSY;
@@ -346,13 +342,12 @@  static int evict(struct domain *d, unsig
  * mfn if populate was called for  gfn which was nominated but not evicted. In
  * this case only the p2mt needs to be forwarded.
  */
-static int prepare(struct domain *d, unsigned long gfn_l,
+static int prepare(struct domain *d, gfn_t gfn,
                    XEN_GUEST_HANDLE_PARAM(const_uint8) buffer)
 {
     struct page_info *page = NULL;
     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 ret, page_extant = 1;
@@ -405,7 +400,7 @@  static int prepare(struct domain *d, uns
         {
             gdprintk(XENLOG_ERR,
                      "Failed to load paging-in gfn %lx Dom%d bytes left %d\n",
-                     gfn_l, d->domain_id, ret);
+                     gfn_x(gfn), d->domain_id, ret);
             ret = -EFAULT;
             goto out;
         }
@@ -421,7 +416,7 @@  static int prepare(struct domain *d, uns
                                                  : p2m_ram_rw, a);
     if ( !ret )
     {
-        set_gpfn_from_mfn(mfn_x(mfn), gfn_l);
+        set_gpfn_from_mfn(mfn_x(mfn), gfn_x(gfn));
 
         if ( !page_extant )
             atomic_dec(&d->paged_pages);
@@ -465,15 +460,15 @@  int mem_paging_memop(XEN_GUEST_HANDLE_PA
     switch( mpo.op )
     {
     case XENMEM_paging_op_nominate:
-        rc = nominate(d, mpo.gfn);
+        rc = nominate(d, _gfn(mpo.gfn));
         break;
 
     case XENMEM_paging_op_evict:
-        rc = evict(d, mpo.gfn);
+        rc = evict(d, _gfn(mpo.gfn));
         break;
 
     case XENMEM_paging_op_prep:
-        rc = prepare(d, mpo.gfn, mpo.buffer);
+        rc = prepare(d, _gfn(mpo.gfn), mpo.buffer);
         if ( !rc )
             copyback = 1;
         break;
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1835,7 +1835,7 @@  void *map_domain_gfn(struct p2m_domain *
         ASSERT(p2m_is_hostp2m(p2m));
         if ( page )
             put_page(page);
-        p2m_mem_paging_populate(p2m->domain, gfn_x(gfn));
+        p2m_mem_paging_populate(p2m->domain, gfn);
         *pfec = PFEC_page_paged;
         return NULL;
     }
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -848,7 +848,7 @@  int guest_wrmsr_xen(struct vcpu *v, uint
 
             if ( p2m_is_paging(t) )
             {
-                p2m_mem_paging_populate(d, gmfn);
+                p2m_mem_paging_populate(d, _gfn(gmfn));
                 return X86EMUL_RETRY;
             }
 
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -322,7 +322,7 @@  int guest_remove_page(struct domain *d,
 
         put_gfn(d, gmfn);
 
-        p2m_mem_paging_drop_page(d, gmfn, p2mt);
+        p2m_mem_paging_drop_page(d, _gfn(gmfn), p2mt);
 
         return 0;
     }
@@ -1667,7 +1667,7 @@  int check_get_page_from_gfn(struct domai
         if ( page )
             put_page(page);
 
-        p2m_mem_paging_populate(d, gfn_x(gfn));
+        p2m_mem_paging_populate(d, gfn);
         return -EAGAIN;
     }
 #endif
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -732,10 +732,9 @@  static inline void p2m_pod_init(struct p
 int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
 
 /* Tell xenpaging to drop a paged out frame */
-void p2m_mem_paging_drop_page(struct domain *d, unsigned long gfn, 
-                                p2m_type_t p2mt);
+void p2m_mem_paging_drop_page(struct domain *d, gfn_t gfn, p2m_type_t p2mt);
 /* Start populating a paged out frame */
-void p2m_mem_paging_populate(struct domain *d, unsigned long gfn);
+void p2m_mem_paging_populate(struct domain *d, gfn_t gfn);
 /* Resume normal operation (in case a domain was paused) */
 struct vm_event_st;
 void p2m_mem_paging_resume(struct domain *d, struct vm_event_st *rsp);