Re: [Xen-devel] [PATCH v5 4/8] mm: introduce a helper to get the memory type of a page

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.08.18 at 15:43, <roger.pau@xxxxxxxxxx> wrote:
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1181,6 +1181,12 @@ int page_is_ram_type(unsigned long mfn, unsigned long 
> mem_type)
>      return 0;
>  }
>  
> +int page_get_type(unsigned long mfn)
> +{
> +    ASSERT_UNREACHABLE();
> +    return -1;
> +}

With the assertion, why is the function needed in the first place?

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -430,6 +430,40 @@ int page_is_ram_type(unsigned long mfn, unsigned long 
> mem_type)
>      return 0;
>  }
>  
> +int page_get_type(unsigned long mfn)

Considering we already have get_page_type(), this function name
needs to change. At the very least page_get_ram_type(), looking
at the set of values it may return.

> +{
> +    uint64_t maddr = pfn_to_paddr(mfn);
> +    unsigned int i;
> +
> +    for ( i = 0; i < e820.nr_map; i++ )
> +    {
> +        /* Test the range. */
> +        if ( (e820.map[i].addr <= maddr) &&
> +             ((e820.map[i].addr + e820.map[i].size) >= (maddr + PAGE_SIZE)) )

Would you mind inverting the condition and using continue, both
to reduce indentation of the switch and to make it very clear that
no braces are needed / wanted?

> +            switch ( e820.map[i].type )
> +            {
> +            case E820_RAM:
> +                return RAM_TYPE_CONVENTIONAL;
> +
> +            case E820_RESERVED:
> +                return RAM_TYPE_RESERVED;
> +
> +            case E820_UNUSABLE:
> +                return RAM_TYPE_UNUSABLE;
> +
> +            case E820_ACPI:
> +            case E820_NVS:
> +                return RAM_TYPE_ACPI;
> +
> +            default:
> +                ASSERT_UNREACHABLE();
> +                return -1;
> +            }
> +    }
> +
> +    return -1;
> +}

One more case to consider: What about a page part of which is
a given type, and the other part of which is simply missing from
the E820 table? I'm uncertain whether in that case it might be a
good idea in general to report it as having the given type; for the
specific purpose you want the function for, that would imo be
quite helpful.

Jan



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