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

[Paul Durrant <xadimgnik@gmail.com>]

> -----Original Message-----
> From: julien@xen.org <julien@xen.org>
> Sent: 22 March 2020 16:14
> To: xen-devel@lists.xenproject.org
> Cc: julien@xen.org; Julien Grall <julien.grall@arm.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper <andrew.cooper3@citrix.com>; George
> Dunlap <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Paul Durrant
> <paul@xen.org>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>; Tim Deegan
> <tim@xen.org>
> Subject: [PATCH 17/17] xen: Switch parameter in get_page_from_gfn to use typesafe gfn
> 
> From: Julien Grall <julien.grall@arm.com>
> 
> No functional change intended.
> 
> Only reasonable clean-ups are done in this patch. The rest will use _gfn
> for the time being.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Definitely an improvement so...

Reviewed-by: Paul Durrant <paul@xen.org>

But a couple of things I noticed...

[snip]
> diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> index 5d5a746a25..3c29ff86be 100644
> --- a/xen/arch/x86/hvm/domain.c
> +++ b/xen/arch/x86/hvm/domain.c
> @@ -296,8 +296,10 @@ int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
>      if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) )
>      {
>          /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
> -        struct page_info *page = get_page_from_gfn(v->domain,
> -                                 v->arch.hvm.guest_cr[3] >> PAGE_SHIFT,
> +        struct page_info *page;
> +
> +        page = get_page_from_gfn(v->domain,
> +                                 gaddr_to_gfn(v->arch.hvm.guest_cr[3]),

Should this be cr3_to_gfn?

>                                   NULL, P2M_ALLOC);
>          if ( !page )
>          {
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index a3d115b650..9f720e7aa1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2216,7 +2216,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);
> @@ -2271,7 +2271,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]);
> +

Same here.

>              page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
>              if ( !page )
>              {
> @@ -2363,7 +2364,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, cr3_to_gfn(value),
>                                   NULL, P2M_ALLOC);
>          if ( !page )
>              goto bad_cr3;
> @@ -3191,7 +3192,7 @@ enum hvm_translation_result hvm_translate_get_page(
>           && hvm_mmio_internal(gfn_to_gaddr(gfn)) )
>          return HVMTRANS_bad_gfn_to_mfn;
> 
> -    page = get_page_from_gfn(v->domain, gfn_x(gfn), &p2mt, P2M_UNSHARE);
> +    page = get_page_from_gfn(v->domain, gfn, &p2mt, P2M_UNSHARE);
> 
>      if ( !page )
>          return HVMTRANS_bad_gfn_to_mfn;
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 32d8d847f2..a9abd6d3f1 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -299,7 +299,7 @@ static int svm_vmcb_restore(struct vcpu *v, struct hvm_hw_cpu *c)
>      {
>          if ( c->cr0 & X86_CR0_PG )
>          {
> -            page = get_page_from_gfn(v->domain, c->cr3 >> PAGE_SHIFT,
> +            page = get_page_from_gfn(v->domain, cr3_to_gfn(c->cr3),
>                                       NULL, P2M_ALLOC);
>              if ( !page )
>              {
> @@ -2230,9 +2230,9 @@ nsvm_get_nvmcb_page(struct vcpu *v, uint64_t vmcbaddr)
>          return NULL;
> 
>      /* Need to translate L1-GPA to MPA */
> -    page = get_page_from_gfn(v->domain,
> -                            nv->nv_vvmcxaddr >> PAGE_SHIFT,
> -                            &p2mt, P2M_ALLOC | P2M_UNSHARE);
> +    page = get_page_from_gfn(v->domain,
> +                             gaddr_to_gfn(nv->nv_vvmcxaddr),
> +                             &p2mt, P2M_ALLOC | P2M_UNSHARE);
>      if ( !page )
>          return NULL;
> 
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> index 977c1bc54f..3d75a0f133 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -242,16 +242,16 @@ static void dump_hypercall(const struct domain *d)
> 
>  static void enable_hypercall_page(struct domain *d)
>  {
> -    unsigned long gmfn = d->arch.hvm.viridian->hypercall_gpa.pfn;
> -    struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
> +    gfn_t gfn = _gfn(d->arch.hvm.viridian->hypercall_gpa.pfn);
> +    struct page_info *page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
>      uint8_t *p;
> 
>      if ( !page || !get_page_type(page, PGT_writable_page) )
>      {
>          if ( page )
>              put_page(page);
> -        gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
> -                 gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
> +        gdprintk(XENLOG_WARNING, "Bad GFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
> +                 gfn_x(gfn), mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
>          return;
>      }
> 
> @@ -719,13 +719,13 @@ void viridian_dump_guest_page(const struct vcpu *v, const char *name,
> 
>  void viridian_map_guest_page(struct domain *d, struct viridian_page *vp)
>  {
> -    unsigned long gmfn = vp->msr.pfn;
> +    gfn_t gfn = _gfn(vp->msr.pfn);
>      struct page_info *page;
> 
>      if ( vp->ptr )
>          return;
> 
> -    page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
> +    page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
>      if ( !page )
>          goto fail;
> 
> @@ -746,8 +746,8 @@ void viridian_map_guest_page(struct domain *d, struct viridian_page *vp)
>      return;
> 
>   fail:
> -    gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
> -             gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
> +    gdprintk(XENLOG_WARNING, "Bad GFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
> +             gfn_x(gfn), mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
>  }
> 
>  void viridian_unmap_guest_page(struct viridian_page *vp)
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index a1e3a19c0a..f1898c63c5 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -681,7 +681,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),

And here.

>                                       NULL, P2M_ALLOC);
>              if ( !page )
>              {
> @@ -1321,7 +1321,7 @@ static void vmx_load_pdptrs(struct vcpu *v)
>      if ( (cr3 & 0x1fUL) && !hvm_pcid_enabled(v) )
>          goto crash;
> 
> -    page = get_page_from_gfn(v->domain, cr3 >> PAGE_SHIFT, &p2mt, P2M_UNSHARE);
> +    page = get_page_from_gfn(v->domain, gaddr_to_gfn(cr3), &p2mt, P2M_UNSHARE);

And here.

  Paul


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