Re: [PATCH 1/2] x86/mm: Use mfn_t for new_guest_cr3()

[George Dunlap <dunlapg@umich.edu>]

On Wed, Aug 30, 2017 at 1:19 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> No functional change (as confirmed by diffing the assembly).
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
>
> For some reason best known to GCC, there is one single change:
>
> @@ -145835,7 +145835,7 @@
>  ffff82d0802864f8:      85 c0                   test   %eax,%eax
>  ffff82d0802864fa:      75 e0                   jne    ffff82d0802864dc <new_guest_cr3+0x7c>
>  ffff82d0802864fc:      4c 8b ad 08 0b 00 00    mov    0xb08(%rbp),%r13
> -ffff82d080286503:      49 39 dd                cmp    %rbx,%r13
> +ffff82d080286503:      4c 39 eb                cmp    %r13,%rbx
>  ffff82d080286506:      0f 84 74 01 00 00       je     ffff82d080286680 <new_guest_cr3+0x220>
>  ffff82d08028650c:      41 f6 84 24 19 06 00    testb  $0x8,0x619(%r12)
>  ffff82d080286513:      00 08
>
> This is from the mfn_eq() alteration, and must be a side effect from using an
> inline function.  The net result is still correct, as only the zero flag is
> checked.
> ---
>  xen/arch/x86/mm.c              | 35 ++++++++++++++++++-----------------
>  xen/arch/x86/pv/emul-priv-op.c |  2 +-
>  xen/include/asm-x86/mm.h       |  2 +-
>  3 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 1f23470..dc07b4f 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2772,23 +2772,23 @@ int vcpu_destroy_pagetables(struct vcpu *v)
>      return rc != -EINTR ? rc : -ERESTART;
>  }
>
> -int new_guest_cr3(unsigned long mfn)
> +int new_guest_cr3(mfn_t mfn)
>  {
>      struct vcpu *curr = current;
>      struct domain *d = curr->domain;
>      int rc;
> -    unsigned long old_base_mfn;
> +    mfn_t old_base_mfn;
>
>      if ( is_pv_32bit_domain(d) )
>      {
> -        unsigned long gt_mfn = pagetable_get_pfn(curr->arch.guest_table);
> -        l4_pgentry_t *pl4e = map_domain_page(_mfn(gt_mfn));
> +        mfn_t mmfn = pagetable_get_mfn(curr->arch.guest_table);
> +        l4_pgentry_t *pl4e = map_domain_page(mmfn);
>
>          rc = mod_l4_entry(pl4e,
> -                          l4e_from_pfn(mfn,
> +                          l4e_from_mfn(mfn,
>                                         (_PAGE_PRESENT | _PAGE_RW |
>                                          _PAGE_USER | _PAGE_ACCESSED)),
> -                          gt_mfn, 0, curr);
> +                          mfn_x(mmfn), 0, curr);
>          unmap_domain_page(pl4e);
>          switch ( rc )
>          {
> @@ -2800,7 +2800,7 @@ int new_guest_cr3(unsigned long mfn)
>          default:
>              gdprintk(XENLOG_WARNING,
>                       "Error while installing new compat baseptr %" PRI_mfn "\n",
> -                     mfn);
> +                     mfn_x(mfn));
>              return rc;
>          }
>
> @@ -2814,20 +2814,20 @@ int new_guest_cr3(unsigned long mfn)
>      if ( unlikely(rc) )
>          return rc;
>
> -    old_base_mfn = pagetable_get_pfn(curr->arch.guest_table);
> +    old_base_mfn = pagetable_get_mfn(curr->arch.guest_table);
>      /*
>       * This is particularly important when getting restarted after the
>       * previous attempt got preempted in the put-old-MFN phase.
>       */
> -    if ( old_base_mfn == mfn )
> +    if ( mfn_eq(old_base_mfn, mfn) )
>      {
>          write_ptbase(curr);
>          return 0;
>      }
>
>      rc = paging_mode_refcounts(d)
> -         ? (get_page_from_mfn(_mfn(mfn), d) ? 0 : -EINVAL)
> -         : get_page_and_type_from_mfn(_mfn(mfn), PGT_root_page_table, d, 0, 1);
> +         ? (get_page_from_mfn(mfn, d) ? 0 : -EINVAL)
> +         : get_page_and_type_from_mfn(mfn, PGT_root_page_table, d, 0, 1);
>      switch ( rc )
>      {
>      case 0:
> @@ -2837,22 +2837,23 @@ int new_guest_cr3(unsigned long mfn)
>          return -ERESTART;
>      default:
>          gdprintk(XENLOG_WARNING,
> -                 "Error while installing new baseptr %" PRI_mfn "\n", mfn);
> +                 "Error while installing new baseptr %" PRI_mfn "\n",
> +                 mfn_x(mfn));
>          return rc;
>      }
>
>      invalidate_shadow_ldt(curr, 0);
>
>      if ( !VM_ASSIST(d, m2p_strict) && !paging_mode_refcounts(d) )
> -        fill_ro_mpt(_mfn(mfn));
> -    curr->arch.guest_table = pagetable_from_pfn(mfn);
> +        fill_ro_mpt(mfn);
> +    curr->arch.guest_table = pagetable_from_mfn(mfn);
>      update_cr3(curr);
>
>      write_ptbase(curr);
>
> -    if ( likely(old_base_mfn != 0) )
> +    if ( likely(mfn_x(old_base_mfn) != 0) )
>      {
> -        struct page_info *page = mfn_to_page(_mfn(old_base_mfn));
> +        struct page_info *page = mfn_to_page(old_base_mfn);
>
>          if ( paging_mode_refcounts(d) )
>              put_page(page);
> @@ -3180,7 +3181,7 @@ long do_mmuext_op(
>              else if ( unlikely(paging_mode_translate(currd)) )
>                  rc = -EINVAL;
>              else
> -                rc = new_guest_cr3(op.arg1.mfn);
> +                rc = new_guest_cr3(_mfn(op.arg1.mfn));
>              break;
>
>          case MMUEXT_NEW_USER_BASEPTR: {
> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
> index af1624a..54a63c2 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -774,7 +774,7 @@ static int priv_op_write_cr(unsigned int reg, unsigned long val,
>          page = get_page_from_gfn(currd, gfn, NULL, P2M_ALLOC);
>          if ( !page )
>              break;
> -        rc = new_guest_cr3(mfn_x(page_to_mfn(page)));
> +        rc = new_guest_cr3(page_to_mfn(page));
>          put_page(page);
>
>          switch ( rc )
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index ec7ce3c..4c03a33 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -539,7 +539,7 @@ void audit_domains(void);
>
>  #endif
>
> -int new_guest_cr3(unsigned long pfn);
> +int new_guest_cr3(mfn_t mfn);
>  void make_cr3(struct vcpu *v, unsigned long mfn);
>  void update_cr3(struct vcpu *v);
>  int vcpu_destroy_pagetables(struct vcpu *);
> --
> 2.1.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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