Re: [Xen-devel] [PATCH v3 13/14] xen/mm: Convert {s, g}et_gpfn_from_mfn() to use typesafe MFN

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

Sorry, I seem to have missed the comments on this patch while respinning the 
series.

I will look at it and address them in v4.

Cheers,

On 05/06/2019 12:01, Jan Beulich wrote:
> > > > On 03.06.19 at 18:03, <julien.grall@xxxxxxx> wrote:
> > --- a/xen/arch/x86/x86_64/traps.c
> > +++ b/xen/arch/x86/x86_64/traps.c
> > @@ -184,7 +184,8 @@ void vcpu_show_registers(const struct vcpu *v)
> >   
> >   void show_page_walk(unsigned long addr)
> >   {
> > -    unsigned long pfn, mfn = read_cr3() >> PAGE_SHIFT;
> > +    unsigned long pfn;
> > +    mfn_t mfn = maddr_to_mfn(read_cr3());
> Quoting my v1 comment:
>
> "I realize you simply take what has been there and transform it, but
>   maddr_to_mfn() (other than shifting by PAGE_SHIFT) is not truly
>   applicable here: What the CR3 register holds is not a physical address,
>   both the low twelve bits as well as the high twelve ones have different
>   meaning. The shift is correct currently because the high ones are
>   (right now) zero on reads. Please consider AND-ing with
>   X86_CR3_ADDR_MASK (or keeping the shift)."
>
> FOAD by "please consider" I don't mean "leave it as is will be fine as
> well", but to do one of the two possible changes, with a preference
> towards the AND-ing, as that's the ultimately correct approach.
>
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -1416,7 +1416,7 @@ static void free_heap_pages(
> >   
> >           /* This page is not a guest frame any more. */
> >           page_set_owner(&pg[i], NULL); /* set_gpfn_from_mfn snoops pg owner */
> > -        set_gpfn_from_mfn(mfn_x(mfn) + i, INVALID_M2P_ENTRY);
> > +        set_pfn_from_mfn(mfn_add(mfn, + i), INVALID_M2P_ENTRY);
> Quoting my v1 comment again: "Stray leftover + ?"
>
> > --- a/xen/include/asm-x86/mm.h
> > +++ b/xen/include/asm-x86/mm.h
> > @@ -492,22 +492,26 @@ extern struct domain *dom_xen, *dom_io, *dom_cow; /* for 
> > vmcoreinfo */
> >    */
> >   extern bool machine_to_phys_mapping_valid;
> >   
> > -static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn)
> > +static inline void set_pfn_from_mfn(mfn_t mfn, unsigned long pfn)
> >   {
> > -    struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
> > +    const unsigned long mfn_ = mfn_x(mfn);
> > +    struct domain *d = page_get_owner(mfn_to_page(mfn));
> const? (Or is this a place where I've similarly asked on an earlier
> patch already?)
>
> Btw, the cheaper (in terms of code churn) change here would seem to
> be to name the function parameter mfn_, and the local variable mfn.
> That'll also reduce the number of uses of the unfortunate trailing-
> underscore-name.
>
> Jan
--
Julien Grall

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