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

Re: [Xen-devel] [PATCH v4 1/2] xen: Switch parameter in get_page_from_gfn to use typesafe gfn

Hi Paul,

Thank you for the review,

On 20/08/2019 09:15, Paul Durrant wrote:
-----Original Message-----
From: Julien Grall <julien.grall@xxxxxxx>
Sent: 19 August 2019 15:27
To: xen-devel@xxxxxxxxxxxxxxxxxxxx
Cc: Julien Grall <julien.grall@xxxxxxx>; Stefano Stabellini 
<sstabellini@xxxxxxxxxx>; Volodymyr
Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper 
<Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
<George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Jan Beulich 
Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; Wei Liu 
Roger Pau Monne <roger.pau@xxxxxxxxxx>; Boris Ostrovsky 
<boris.ostrovsky@xxxxxxxxxx>; Suravee
Suthikulpanit <suravee.suthikulpanit@xxxxxxx>; Brian Woods 
<brian.woods@xxxxxxx>; Paul Durrant
<Paul.Durrant@xxxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>; Kevin Tian 
Subject: [PATCH v4 1/2] xen: Switch parameter in get_page_from_gfn to use 
typesafe gfn

No functional change intended.

Only reasonable clean-ups are done in this patch. The rest will use _gfn
for the time being.

The code in get_page_from_gfn is slightly reworked to also handle a bad
behavior because it is not safe to use mfn_to_page(...) because
mfn_valid(...) succeeds.

Signed-off-by: Julien Grall <julien.grall@xxxxxxx>

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

... with a few suggestions below ...

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 029eea3b85..236bd6ed38 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2168,7 +2168,7 @@ int hvm_set_cr0(unsigned long value, bool may_defer)
      struct vcpu *v = current;
      struct domain *d = v->domain;
-    unsigned long gfn, old_value = v->arch.hvm.guest_cr[0];
+    unsigned long old_value = v->arch.hvm.guest_cr[0];
      struct page_info *page;

      HVM_DBG_LOG(DBG_LEVEL_VMMU, "Update CR0 value = %lx", value);
@@ -2223,7 +2223,8 @@ int hvm_set_cr0(unsigned long value, bool may_defer)
          if ( !paging_mode_hap(d) )
              /* The guest CR3 must be pointing to the guest physical. */
-            gfn = v->arch.hvm.guest_cr[3] >> PAGE_SHIFT;
+            gfn_t gfn = gaddr_to_gfn(v->arch.hvm.guest_cr[3]);
              page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
              if ( !page )
@@ -2315,7 +2316,7 @@ int hvm_set_cr3(unsigned long value, bool may_defer)
          /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
          HVM_DBG_LOG(DBG_LEVEL_VMMU, "CR3 value = %lx", value);
-        page = get_page_from_gfn(v->domain, value >> PAGE_SHIFT,
+        page = get_page_from_gfn(v->domain, gaddr_to_gfn(value),
                                   NULL, P2M_ALLOC);

I think you can reduce the scope of 'page' above in the same way you did with 
'gfn' above

For this one and ...

          if ( !page )
              goto bad_cr3;

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 0060310d74..38bdef0862 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -674,7 +674,7 @@ static int vmx_restore_cr0_cr3(
          if ( cr0 & X86_CR0_PG )
-            page = get_page_from_gfn(v->domain, cr3 >> PAGE_SHIFT,
+            page = get_page_from_gfn(v->domain, gaddr_to_gfn(cr3),
                                       NULL, P2M_ALLOC);

Here also you can reduce the scope of 'page' (although only into the scope just 
outside the 'if')

... this one, we don't change the type of the variable so I don't feel such clean-ups belong to this patch.

              if ( !page )

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 7531406543..f8e2b6f921 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2083,7 +2083,7 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t 
              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(l1e_get_pfn(nl1e)), &p2mt, 

'l1e_get_pfn(nl1e)' is repeated 3 times within this scope AFAICT so probably 
worth a local variable while you're in the neighbourhood, to reduce verbosity.

I can only find 2 use of l1e_get_pfn within mod_l1_entry. But then, this sort of clean-up should be in there own patch.

But I will leave that to the x86 folks as I don't want to be roped in endless clean-up. I know there will be more ;).

              if ( p2m_is_paged(p2mt) )

diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 3a3c15890b..4f3f438614 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -229,7 +229,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 

          ret = -EINVAL;
-        page = get_page_from_gfn(current->domain, info.gmfn, NULL, P2M_ALLOC);
+        page = get_page_from_gfn(current->domain, _gfn(info.gmfn),
+                                 NULL, P2M_ALLOC);

'currd' has actually been defined at the top of the function so if you lose the 
'current->domain' you can re-flow this back onto one line I think.

Sounds reasonable, but there are 3 more characters than the normal, so it will still have to live on 2 lines :(.


Julien Grall

Xen-devel mailing list



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