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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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



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