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

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

On 2014/8/18 17:51, Andrew Cooper wrote:
> On 18/08/14 08:45, Chen, Tiejun wrote:
> > 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.
> There is, as far as I am aware, no upper bound to the number of RMRRs
> reported by the firmware.  rmrr_maps.map is an array of length E820_MAX,
As you know all RMRR can be listed in a e820 table so its not possible 
to be over 128.
> or 128.  If this function is called more than 128 times, you will start
> clobbering Xen data.
And this is just a static function here, we also don't call that with >128.

Anyway, I can add something to deliver warning message like this,

+
+    if ( rmrr_map.nr_map >= E820MAX )
+        printk(XENLOG_WARNING
+               "Overflow RMRR mappings! Failed to record RMRR 
mappings.\n");
> But as Jan pointed out in the other fork of this thread, this entire
> function is not needed, and can be removed.
I'm a little bit disagreed here. Please see my reply to Jan.

Thanks
Tiejun
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel