From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings Date: Mon, 18 Aug 2014 18:01:16 +0800 Message-ID: <53F1CEEC.1080301@intel.com> References: <1408091238-18364-1-git-send-email-tiejun.chen@intel.com> <1408091238-18364-2-git-send-email-tiejun.chen@intel.com> <53EDD539.90403@citrix.com> <53F1AF19.4080003@intel.com> <53F1CC93.4020205@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53F1CC93.4020205@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , JBeulich@suse.com, ian.jackson@eu.citrix.com, stefano.stabellini@eu.citrix.com, ian.campbell@citrix.com, yang.z.zhang@intel.com, kevin.tian@intel.com Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 >>>> --- >>>> 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 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 >>>> #include >>>> #include >>>> +#include >>>> #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 > >