Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/08/14 09:27, Tiejun Chen wrote:
> We will expose RMRR mappings to VM so need to record all
> RMRR mappings firstly.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  xen/arch/x86/e820.c                |  2 ++
>  xen/drivers/passthrough/vtd/dmar.c | 14 ++++++++++++++
>  xen/include/asm-x86/e820.h         |  3 +++
>  3 files changed, 19 insertions(+)
>
> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> index 55fe0d6..db44afd 100644
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -34,6 +34,8 @@ static bool_t __initdata e820_verbose;
>  boolean_param("e820-verbose", e820_verbose);
>  
>  struct e820map e820;
> +/* Used to record RMRR mapping. */
> +rmrr_maps_t rmrr_maps;
>  
>  /*
>   * This function checks if the entire range <start,end> is mapped with type.
> diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
> index 1152c3a..24321e3 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -29,6 +29,7 @@
>  #include <xen/pci.h>
>  #include <xen/pci_regs.h>
>  #include <asm/string.h>
> +#include <asm/e820.h>
>  #include "dmar.h"
>  #include "iommu.h"
>  #include "extern.h"
> @@ -80,6 +81,18 @@ static int __init acpi_register_rmrr_unit(struct acpi_rmrr_unit *rmrr)
>      return 0;
>  }
>  
> +/* Record RMRR mapping to ready expose VM. */
> +static int __init rmrr_maps_register(struct acpi_rmrr_unit *rmrr)
> +{

You absolutely need some protection against calling this function more
than 128 times, or you need to do dynamic allocation of the storage.

> +    rmrr_maps.map[rmrr_maps.nr_map].addr = rmrr->base_address;
> +    rmrr_maps.map[rmrr_maps.nr_map].size = rmrr->end_address -
> +                                           rmrr->base_address;
> +    rmrr_maps.map[rmrr_maps.nr_map].type = E820_RESERVED;
> +    rmrr_maps.nr_map++;
> +
> +    return 0;
> +}
> +
>  static void __init disable_all_dmar_units(void)
>  {
>      struct acpi_drhd_unit *drhd, *_drhd;
> @@ -675,6 +688,7 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>                          " end_address %"PRIx64"\n",
>                          rmrru->base_address, rmrru->end_address);
>              acpi_register_rmrr_unit(rmrru);
> +            rmrr_maps_register(rmrru);
>          }
>      }
>  
> diff --git a/xen/include/asm-x86/e820.h b/xen/include/asm-x86/e820.h
> index 71a804c..74262c9 100644
> --- a/xen/include/asm-x86/e820.h
> +++ b/xen/include/asm-x86/e820.h
> @@ -23,6 +23,8 @@ struct e820map {
>      struct e820entry map[E820MAX];
>  };
>  
> +typedef struct e820map rmrr_maps_t;

This type is a single map of RMRR regions, not multiple maps. 
rmrr_map_t please.

~Andrew

> +
>  extern int e820_all_mapped(u64 start, u64 end, unsigned type);
>  extern int reserve_e820_ram(struct e820map *e820, uint64_t s, uint64_t e);
>  extern int e820_change_range_type(
> @@ -32,6 +34,7 @@ extern int e820_add_range(
>      struct e820map *, uint64_t s, uint64_t e, uint32_t type);
>  extern unsigned long init_e820(const char *, struct e820entry *, int *);
>  extern struct e820map e820;
> +extern rmrr_maps_t rmrr_maps;
>  
>  /* These symbols live in the boot trampoline. */
>  extern struct e820entry e820map[];