Re: [Xen-devel] [v7][PATCH 05/16] hvmloader: get guest memory map into memory_map[]

[George Dunlap <dunlapg@xxxxxxxxx>]

On Thu, Jul 9, 2015 at 6:33 AM, Tiejun Chen <tiejun.chen@xxxxxxxxx> wrote:
> Now we get this map layout by call XENMEM_memory_map then
> save them into one global variable memory_map[]. It should
> include lowmem range, rdm range and highmem range. Note
> rdm range and highmem range may not exist in some cases.
>
> And here we need to check if any reserved memory conflicts with
> [RESERVED_MEMORY_DYNAMIC_START - 1, RESERVED_MEMORY_DYNAMIC_END].
> This range is used to allocate memory in hvmloder level, and
> we would lead hvmloader failed in case of conflict since its
> another rare possibility in real world.
>
> CC: Keir Fraser <keir@xxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
> v5 ~ v7:
>
> * Nothing is changed.
>
> v4:
>
> * Move some codes related to e820 to that specific file, e820.c.
>
> * Consolidate "printf()+BUG()" and "BUG_ON()"
>
> * Avoid another fixed width type for the parameter of get_mem_mapping_layout()
>
>  tools/firmware/hvmloader/e820.c      | 35 +++++++++++++++++++++++++++++++++++
>  tools/firmware/hvmloader/e820.h      |  7 +++++++
>  tools/firmware/hvmloader/hvmloader.c |  2 ++
>  tools/firmware/hvmloader/util.c      | 26 ++++++++++++++++++++++++++
>  tools/firmware/hvmloader/util.h      | 12 ++++++++++++
>  5 files changed, 82 insertions(+)
>
> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
> index 2e05e93..3e53c47 100644
> --- a/tools/firmware/hvmloader/e820.c
> +++ b/tools/firmware/hvmloader/e820.c
> @@ -23,6 +23,41 @@
>  #include "config.h"
>  #include "util.h"
>
> +struct e820map memory_map;
> +
> +void memory_map_setup(void)
> +{
> +    unsigned int nr_entries = E820MAX, i;
> +    int rc;
> +    uint64_t alloc_addr = RESERVED_MEMORY_DYNAMIC_START - 1;
> +    uint64_t alloc_size = RESERVED_MEMORY_DYNAMIC_END - alloc_addr;

Why START-1 rather than just START?

It looks like RESERVED_MEMORY_DYNAMIC_START is set to 0xFC001000.  In
the code the way it is, if there is an RMRR from 0xFC000000 of size
0x1000, it looks like check_overlap() below will fail and hvmloader
will BUG().

Is that really what we want?  Why can we not have an RMRR range that
goes right up to the edge of the reserved range?

Other than that this patch looks good.

 -George

> +
> +    rc = get_mem_mapping_layout(memory_map.map, &nr_entries);
> +
> +    if ( rc || !nr_entries )
> +    {
> +        printf("Get guest memory maps[%d] failed. (%d)\n", nr_entries, rc);
> +        BUG();
> +    }
> +
> +    memory_map.nr_map = nr_entries;
> +
> +    for ( i = 0; i < nr_entries; i++ )
> +    {
> +        if ( memory_map.map[i].type == E820_RESERVED )
> +        {
> +            if ( check_overlap(alloc_addr, alloc_size,
> +                               memory_map.map[i].addr,
> +                               memory_map.map[i].size) )
> +            {
> +                printf("Fail to setup memory map due to conflict");
> +                printf(" on dynamic reserved memory range.\n");
> +                BUG();
> +            }
> +        }
> +    }
> +}
> +
>  void dump_e820_table(struct e820entry *e820, unsigned int nr)
>  {
>      uint64_t last_end = 0, start, end;
> diff --git a/tools/firmware/hvmloader/e820.h b/tools/firmware/hvmloader/e820.h
> index b2ead7f..8b5a9e0 100644
> --- a/tools/firmware/hvmloader/e820.h
> +++ b/tools/firmware/hvmloader/e820.h
> @@ -15,6 +15,13 @@ struct e820entry {
>      uint32_t type;
>  } __attribute__((packed));
>
> +#define E820MAX        128
> +
> +struct e820map {
> +    unsigned int nr_map;
> +    struct e820entry map[E820MAX];
> +};
> +
>  #endif /* __HVMLOADER_E820_H__ */
>
>  /*
> diff --git a/tools/firmware/hvmloader/hvmloader.c 
> b/tools/firmware/hvmloader/hvmloader.c
> index 25b7f08..84c588c 100644
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -262,6 +262,8 @@ int main(void)
>
>      init_hypercalls();
>
> +    memory_map_setup();
> +
>      xenbus_setup();
>
>      bios = detect_bios();
> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> index 80d822f..122e3fa 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -27,6 +27,17 @@
>  #include <xen/memory.h>
>  #include <xen/sched.h>
>
> +/*
> + * Check whether there exists overlap in the specified memory range.
> + * Returns true if exists, else returns false.
> + */
> +bool check_overlap(uint64_t start, uint64_t size,
> +                   uint64_t reserved_start, uint64_t reserved_size)
> +{
> +    return (start + size > reserved_start) &&
> +            (start < reserved_start + reserved_size);
> +}
> +
>  void wrmsr(uint32_t idx, uint64_t v)
>  {
>      asm volatile (
> @@ -368,6 +379,21 @@ uuid_to_string(char *dest, uint8_t *uuid)
>      *p = '\0';
>  }
>
> +int get_mem_mapping_layout(struct e820entry entries[], uint32_t *max_entries)
> +{
> +    int rc;
> +    struct xen_memory_map memmap = {
> +        .nr_entries = *max_entries
> +    };
> +
> +    set_xen_guest_handle(memmap.buffer, entries);
> +
> +    rc = hypercall_memory_op(XENMEM_memory_map, &memmap);
> +    *max_entries = memmap.nr_entries;
> +
> +    return rc;
> +}
> +
>  void mem_hole_populate_ram(xen_pfn_t mfn, uint32_t nr_mfns)
>  {
>      static int over_allocated;
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index f99c0f19..1100a3b 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -4,8 +4,10 @@
>  #include <stdarg.h>
>  #include <stdint.h>
>  #include <stddef.h>
> +#include <stdbool.h>
>  #include <xen/xen.h>
>  #include <xen/hvm/hvm_info_table.h>
> +#include "e820.h"
>
>  #define __STR(...) #__VA_ARGS__
>  #define STR(...) __STR(__VA_ARGS__)
> @@ -222,6 +224,9 @@ int hvm_param_set(uint32_t index, uint64_t value);
>  /* Setup PCI bus */
>  void pci_setup(void);
>
> +/* Setup memory map  */
> +void memory_map_setup(void);
> +
>  /* Prepare the 32bit BIOS */
>  uint32_t rombios_highbios_setup(void);
>
> @@ -249,6 +254,13 @@ void perform_tests(void);
>
>  extern char _start[], _end[];
>
> +int get_mem_mapping_layout(struct e820entry entries[],
> +                           unsigned int *max_entries);
> +
> +extern struct e820map memory_map;
> +bool check_overlap(uint64_t start, uint64_t size,
> +                   uint64_t reserved_start, uint64_t reserved_size);
> +
>  #endif /* __HVMLOADER_UTIL_H__ */
>
>  /*
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel