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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----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 
> <jbeulich@xxxxxxxx>;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; 
> Wei Liu <wl@xxxxxxx>;
> 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 
> <kevin.tian@xxxxxxxxx>
> 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 ...

[snip]
> 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

>          if ( !page )
>              goto bad_cr3;

[snip]
> 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')

>              if ( !page )
>              {

[snip]
> 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 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(l1e_get_pfn(nl1e)), &p2mt, 
> q);
> 

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

>              if ( p2m_is_paged(p2mt) )
>              {

[snip]
> 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) 
> arg)
>              break;
> 
>          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.

>          if ( !page )
>              break;
>          if ( !get_page_type(page, PGT_writable_page) )


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel