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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 06 November 2018 20:12
> To: Julien Grall <julien.grall@arm.com>; sstabellini@kernel.org; xen-
> devel@lists.xenproject.org
> Cc: George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Jan Beulich <jbeulich@suse.com>; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>; Wei
> Liu <wei.liu2@citrix.com>; Boris Ostrovsky <boris.ostrovsky@oracle.com>;
> Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Brian Woods
> <brian.woods@amd.com>; Paul Durrant <Paul.Durrant@citrix.com>; Jun
> Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>;
> Julien Grall <julie.grall@arm.com>
> Subject: Re: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use
> typesafe gfn
> 
> 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;

^^ er... how's that mfn_valid() going to work? You'd need mfn_valid(page_to_mfn(pahge)), or somesuch.

  Paul

> 
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel