> -----Original Message-----
> From: Andrew Cooper
> Sent: 06 November 2018 20:12
> To: Julien Grall <julien.gr...@arm.com>; sstabell...@kernel.org; xen-
> de...@lists.xenproject.org
> Cc: George Dunlap <george.dun...@citrix.com>; Ian Jackson
> <ian.jack...@citrix.com>; Jan Beulich <jbeul...@suse.com>; Konrad
> Rzeszutek Wilk <konrad.w...@oracle.com>; Tim (Xen.org) <t...@xen.org>; Wei
> Liu <wei.l...@citrix.com>; Boris Ostrovsky <boris.ostrov...@oracle.com>;
> Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>; Brian Woods
> <brian.wo...@amd.com>; Paul Durrant <paul.durr...@citrix.com>; Jun
> Nakajima <jun.nakaj...@intel.com>; Kevin Tian <kevin.t...@intel.com>;
> Julien Grall <julie.gr...@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
