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

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2014/8/15 17:39, Andrew Cooper wrote:
> 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@xxxxxxxxx>
> > ---
> >   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
Could you show this assumed scenario?

IMO, this should never be happened since we always use a e820 table to 
cover RMRR range/entries.
> 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.
Okay.

Thanks
Tiejun

> ~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[];
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel