From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820 Date: Fri, 08 Aug 2014 17:28:34 +0800 Message-ID: <53E49842.4000506@intel.com> References: <1407409371-31728-1-git-send-email-tiejun.chen@intel.com> <1407409371-31728-5-git-send-email-tiejun.chen@intel.com> <53E36B10.1050805@citrix.com> <53E431E1.3070200@intel.com> <53E48D8D020000780002A4F7@mail.emea.novell.com> <53E47C86.7060400@intel.com> <53E49BCF020000780002A644@mail.emea.novell.com> <53E48CB6.1090303@intel.com> <53E4ADEF020000780002A7C0@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53E4ADEF020000780002A7C0@mail.emea.novell.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: Jan Beulich Cc: kevin.tian@intel.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, Andrew Cooper , ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2014/8/8 17:01, Jan Beulich wrote: >>>> On 08.08.14 at 10:39, wrote: >> On 2014/8/8 15:43, Jan Beulich wrote: >>> More reasonable, albeit now you lost the fetch-just-once behavior. >>> Furthermore - do you really need this to be a dynamic allocation? >> >> So, >> >> diff --git a/tools/firmware/hvmloader/util.c >> b/tools/firmware/hvmloader/util.c >> index 80d822f..d55773e 100644 >> --- a/tools/firmware/hvmloader/util.c >> +++ b/tools/firmware/hvmloader/util.c >> @@ -766,6 +766,17 @@ struct shared_info *get_shared_info(void) >> return shared_info; >> } >> >> +struct e820map *get_rmrr_map_info(void) >> +{ >> + if ( rmrr_e820map.nr_map ) >> + return &rmrr_e820map; >> + >> + if ( hypercall_memory_op(XENMEM_RMRR_memory_map, &rmrr_e820map) != 0 ) >> + BUG(); >> + >> + return &rmrr_e820map; >> +} > > Still not quite, since now you still re-invoke the hypercall if the first > one didn't return any entries (e.g. on a VT-d-less system). I think we just should focus on if we already call that hypercall. struct e820map *get_rmrr_map_info(void) { static int no_rmrr = 1; if ( no_rmrr == 0 ) return &rmrr_e820map; if ( hypercall_memory_op(XENMEM_RMRR_memory_map, &rmrr_e820map) != 0 ) BUG(); no_rmrr = 0; return &rmrr_e820map; } If rmrr_e820map is void, the subsequent codes always do nothing since rmrr_e820map.nr_map is zero. > >> --- a/tools/firmware/hvmloader/util.h >> +++ b/tools/firmware/hvmloader/util.h >> @@ -236,6 +236,8 @@ unsigned long create_pir_tables(void); >> void smp_initialise(void); >> >> #include "e820.h" >> +struct e820map rmrr_e820map; >> +struct e820map *get_rmrr_map_info(void); >> int build_e820_table(struct e820entry *e820, >> unsigned int lowmem_reserved_base, >> unsigned int bios_image_base); >> >> >>> The structure you use right now is fixed size. Which gets me to >>> another point though: Is it said in any part of the VT-d spec that >>> there is an upper limit to the number of RMRRs? The re-use of the >> >> After look up some relevant info in VT-D specs, I think we have no any >> direct field to limit the number of RMRRs. Just in 8.1 DMA Remapping >> Reporting Structure, there are some fields to be referred here: >> >> #1 Length: >> >> in bytes, of the description table including the length of the >> associated remapping structures. >> >> #2 Remapping Structures[]: >> >> A list of structures. The list will contain one or more DMA Remapping >> Hardware Unit Definition (DRHD) structures, and zero or more Reserved >> Memory Region Reporting (RMRR) and Root Port ATS Capability Reporting >> (ATSR) structures. >> >> Seems no any explicit restriction. > > IOW a fixed-length interface structure isn't a proper fit here. > Yes. Thanks Tiejun