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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

Hi - just some cosmetic suggestions.

Subject s/Swich/Switch/

> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
> index f73ea4a163..a529ebcc3f 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -760,12 +760,12 @@ static int write_cr(unsigned int reg, unsigned long val,
>      case 3: /* Write CR3 */
>      {
>          struct domain *currd = curr->domain;
> -        unsigned long gfn;
> +        gfn_t gfn;
>          struct page_info *page;
>          int rc;
>  
> -        gfn = !is_pv_32bit_domain(currd)
> -              ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val);
> +        gfn = _gfn(!is_pv_32bit_domain(currd)
> +              ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val));

Please re-indent.

>          page = get_page_from_gfn(currd, gfn, NULL, P2M_ALLOC);
>          if ( !page )
>              break;
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 9471d89022..d967e49432 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -817,13 +817,14 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, 
> uint64_t val)
>  
>              if ( p2m_is_paging(t) )
>              {
> -                p2m_mem_paging_populate(d, gmfn);
> +                p2m_mem_paging_populate(d, gfn_x(gfn));
>                  return X86EMUL_RETRY;
>              }
>  
>              gdprintk(XENLOG_WARNING,
> -                     "Bad GMFN %lx (MFN %#"PRI_mfn") to MSR %08x\n",
> -                     gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN), 
> base);
> +                     "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn") to MSR %08x\n",

GMFN => GFN.

> +                     gfn_x(gfn), mfn_x(page ? page_to_mfn(page) : 
> INVALID_MFN),
> +                     base);
>              return X86EMUL_EXCEPTION;
>          }
>  
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index d08c595887..db1ec37610 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -489,18 +489,21 @@ struct page_info *p2m_get_page_from_gfn(struct 
> p2m_domain *p2m, gfn_t gfn,
>                                          p2m_query_t q);
>  
>  static inline struct page_info *get_page_from_gfn(
> -    struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
> +    struct domain *d, gfn_t gfn, p2m_type_t *t, p2m_query_t q)
>  {
>      struct page_info *page;
> +    mfn_t mfn;
>  
>      if ( paging_mode_translate(d) )
> -        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn), t, NULL, 
> q);
> +        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), gfn, t, NULL, q);
>  
>      /* Non-translated guests see 1-1 RAM / MMIO mappings everywhere */
>      if ( t )
>          *t = likely(d != dom_io) ? p2m_ram_rw : p2m_mmio_direct;
> -    page = mfn_to_page(_mfn(gfn));
> -    return mfn_valid(_mfn(gfn)) && get_page(page, d) ? page : NULL;
> +
> +    mfn = _mfn(gfn_x(gfn));
> +    page = mfn_to_page(mfn);
> +    return mfn_valid(mfn) && get_page(page, d) ? page : NULL;

This looks like it would be cleaner by not splitting mfn out into a
separate variable.

page = mfn_to_page(_mfn(gfn_x(gfn)));

return mfn_valid(mfn) && get_page(page, d) ? page : NULL;

The only reason this looks odd is because of the mfn => gfn equality,
but we are just beside a comment explaining that we are non-translated.

~Andrew

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