Re: [Xen-devel] [PATCH for-4.12 v2 8/8] xen: Switch parameter in get_page_from_gfn to use typesafe gfn

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andrew,

On 21/12/2018 14:14, Andrew Cooper wrote:
> On 20/12/2018 19:23, Julien Grall wrote:
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index 32dc4253ff..b462a8513b 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -827,7 +827,7 @@ int arch_set_info_guest(
> >       unsigned long flags;
> >       bool compat;
> >   #ifdef CONFIG_PV
> > -    unsigned long cr3_gfn;
> > +    gfn_t cr3_gfn;
> I've sent an alternative patch which this patch should be rebased over,
> at which point all modifications to arch_set_info_guest() should
> hopefully disappear.
The rest of the series should be merged by end of today (Code freeze for Xen 
Arm). So I will resend this patch separately after my holidays.
> > diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> > index 5d5a746a25..73d2da8441 100644
> > --- a/xen/arch/x86/hvm/domain.c
> > +++ b/xen/arch/x86/hvm/domain.c
> > @@ -297,7 +297,7 @@ int arch_set_info_hvm_guest(struct vcpu *v, const 
> > vcpu_hvm_context_t *ctx)
> >       {
> >           /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
> >           struct page_info *page = get_page_from_gfn(v->domain,
> > -                                 v->arch.hvm.guest_cr[3] >> PAGE_SHIFT,
> > +                                 gaddr_to_gfn(v->arch.hvm.guest_cr[3]),
> >                                    NULL, P2M_ALLOC);
> Can you re-indent while modifying this please?
Sure.

> > diff --git a/xen/arch/x86/hvm/viridian/time.c b/xen/arch/x86/hvm/viridian/time.c
> > index 840a82b457..a718434456 100644
> > --- a/xen/arch/x86/hvm/viridian/time.c
> > +++ b/xen/arch/x86/hvm/viridian/time.c
> > @@ -38,16 +38,16 @@ static void dump_reference_tsc(const struct domain *d)
> >   
> >   static void update_reference_tsc(struct domain *d, bool initialize)
> >   {
> > -    unsigned long gmfn = d->arch.hvm.viridian.reference_tsc.fields.pfn;
> > -    struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
> > +    gfn_t gfn = _gfn(d->arch.hvm.viridian.reference_tsc.fields.pfn);
> > +    struct page_info *page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
> >       HV_REFERENCE_TSC_PAGE *p;
> >   
> >       if ( !page || !get_page_type(page, PGT_writable_page) )
> >       {
> >           if ( page )
> >               put_page(page);
> > -        gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
> > -                 gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
> > +        gdprintk(XENLOG_WARNING, "Bad GFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
> The canonical format for gfns and mfns are just %"PRI_*, without the #
>
> Do you mind fixing this seeing as you're changing the string anyway?
Sure.

> > diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> > index 3304921991..1efbc071c5 100644
> > --- a/xen/include/asm-x86/p2m.h
> > +++ b/xen/include/asm-x86/p2m.h
> > @@ -491,18 +491,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 unfortunately propagates some bad behaviour, because it is not safe
> to use mfn_to_page(mfn); before mfn_valid(mfn) succeeds.  (In practice
> it works because mfn_to_page() is just pointer arithmetic.)
>
> Pleas can you express this as:
>
> return (mfn_valid(mfn) &&
>          (page = mfn_to_page(mfn), get_page(page, d))) ? page : NULL;
>
> which at least gets the order of operations in the correct order from
> C's point of view.
>
> Alternatively, and perhaps easier to follow:
>
> if ( !mfn_valid(mfn) )
>      return NULL;
>
> page = mfn_to_page(mfn);
>
> return get_page(page, d) ? page : NULL;
I am happy to fix that. However, shouldn't this be handled in a separate patch? 
After all, the code is not worst than it currently is.
Cheers,

> ~Andrew
--
Julien Grall

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