Re: [Xen-devel] [PATCH] x86/pv: Clean up cr3 handling in arch_set_info_guest()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Dec 21, 2018 at 01:46:05PM +0000, Andrew Cooper wrote:
> All of this code lives inside CONFIG_PV which means gfn == mfn, and the
> fill_ro_mpt() calls clearly show that the value is used untranslated.
> 
> Change cr3_gfn to a suitably typed cr3_mfn, and replace get_page_from_gfn()
> with a straight mfn_to_page/get_page sequence, to avoid the implication that
> translation is going on.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> 
> Julien: This should simplify your "xen: Switch parameter in get_page_from_gfn
> to use typesafe gfn" patch.  In particular, I did a doubletake at
> fill_ro_mpt(_mfn(gfn_x(cr3_gfn))); when reviewing it.
> ---
>  xen/arch/x86/domain.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 32dc4253..da94ab4 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -827,8 +827,8 @@ int arch_set_info_guest(
>      unsigned long flags;
>      bool compat;
>  #ifdef CONFIG_PV
> -    unsigned long cr3_gfn;
> -    struct page_info *cr3_page;
> +    mfn_t cr3_mfn;
> +    struct page_info *cr3_page = NULL;
>      unsigned long cr4;
>      int rc = 0;
>  #endif
> @@ -1091,12 +1091,12 @@ int arch_set_info_guest(
>      set_bit(_VPF_in_reset, &v->pause_flags);
>  
>      if ( !compat )
> -        cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[3]);
> +        cr3_mfn = _mfn(xen_cr3_to_pfn(c.nat->ctrlreg[3]));
>      else
> -        cr3_gfn = compat_cr3_to_pfn(c.cmp->ctrlreg[3]);
> -    cr3_page = get_page_from_gfn(d, cr3_gfn, NULL, P2M_ALLOC);
> +        cr3_mfn = _mfn(compat_cr3_to_pfn(c.cmp->ctrlreg[3]));
>  
> -    if ( !cr3_page )
> +    if ( !mfn_valid(cr3_mfn) ||
> +         !(cr3_page = mfn_to_page(cr3_mfn), get_page(cr3_page, d)) )

This is kind of an open-coded version of get_page_from_gfn with just
the non-paging bits, IMO I would use get_page_from_gfn, or introduce a
get_page_from_mfn helper?

The more that you use the same construct below.

>          rc = -EINVAL;
>      else if ( paging_mode_refcounts(d) )
>          /* nothing */;
> @@ -1122,7 +1122,7 @@ int arch_set_info_guest(
>          case 0:
>              if ( !compat && !VM_ASSIST(d, m2p_strict) &&
>                   !paging_mode_refcounts(d) )
> -                fill_ro_mpt(_mfn(cr3_gfn));
> +                fill_ro_mpt(cr3_mfn);
>              break;
>          default:
>              if ( cr3_page == current->arch.old_guest_table )
> @@ -1137,10 +1137,10 @@ int arch_set_info_guest(
>          v->arch.guest_table = pagetable_from_page(cr3_page);
>          if ( c.nat->ctrlreg[1] )
>          {
> -            cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[1]);
> -            cr3_page = get_page_from_gfn(d, cr3_gfn, NULL, P2M_ALLOC);
> +            cr3_mfn = _mfn(xen_cr3_to_pfn(c.nat->ctrlreg[1]));

I assume this is something PV specific, but calling xen_cr3_to_pfn on
ctrlreg[1] seems wrong at first sight. And the xen_cr3_to_pfn and
xen_pfn_to_cr3 helpers seem quite pointless, since it's just PFN_DOWN
or pfn_to_paddr.

Thanks, Roger.

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