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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.12.18 at 14:46, <andrew.cooper3@xxxxxxxxxx> 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)) )

I'd prefer if the comma operator was used really only when there's
no alternative. Here I think

    if ( !mfn_valid(cr3_mfn) ||
         !get_page(cr3_page = mfn_to_page(cr3_mfn), d) )

is quite a bit easier to read.

> @@ -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]));
>  
> -            if ( !cr3_page )
> +            if ( !mfn_valid(cr3_mfn) ||
> +                 !(cr3_page = mfn_to_page(cr3_mfn), get_page(cr3_page, d)) )

Same here and then
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Or wait - in the discussion with Roger you've indicated you'd
switch to get_page_from_mfn() anyway, which would eliminate
the comma expression altogether, and the need for the newly
added initializer as well. The R-b above applies to that
adjustment as well.

Jan


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