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 15:30:14 +0800 Message-ID: <53E47C86.7060400@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53E48D8D020000780002A4F7@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 14:42, Jan Beulich wrote: >>>> On 08.08.14 at 04:11, wrote: >> On 2014/8/7 20:03, Andrew Cooper wrote: >>> On 07/08/14 12:02, Tiejun Chen wrote: >>>> --- a/tools/firmware/hvmloader/e820.h >>>> +++ b/tools/firmware/hvmloader/e820.h >>>> @@ -15,6 +15,12 @@ struct e820entry { >>>> uint32_t type; >>>> } __attribute__((packed)); >>>> >>>> +#define E820MAX 128 >>>> + >>>> +struct e820map { >>>> + int nr_map; >>> >>> This value should be unsigned. >> >> I'm not sure if we need to change this since here I just copy this from >> the xen/include/asm-x86/e820.h file, >> >> struct e820map { >> int nr_map; >> struct e820entry map[E820MAX]; >> }; > > While it be welcome for you to (in a separate patch) also change this > one, it is not considered okay to copy existing mistakes: We've been > slowly switching over to put more attention on correct signed-ness > (and const-ness) - variables/fields that can't ever be negative > shouldn't have signed types. I'm always afraid I'm missing something magic behind this signed type but with your clarification I already send one small patch to address this, and I also will update this point in this thread. > >>>> --- a/tools/firmware/hvmloader/util.c >>>> +++ b/tools/firmware/hvmloader/util.c >>>> @@ -766,6 +766,19 @@ struct shared_info *get_shared_info(void) >>>> return shared_info; >>>> } >>>> >>>> +struct e820map *get_rmrr_map_info(void) >>>> +{ >>>> + static struct e820map *e820_map = NULL; >>>> + >>>> + if ( e820_map != NULL ) >>>> + return e820_map; >>>> + >>>> + if ( hypercall_memory_op(XENMEM_RMRR_memory_map, e820_map) != 0 ) >>>> + BUG(); >>> >>> This instructs Xen to clobber the memory starting at 0, and works >>> because HVMLoader is in protected, non-paging mode at this point. I >>> don't think this is what you meant to do, and will repeatedly make the >> >> Sorry I can't understand this explicitly. Here I just want a way to get >> RMRR mapping info under hvmloader circumstance. >> >> Could you elaborate what you mean? Or show me a proper way I should do >> as you expect. > > You never allocate memory for the map, i.e. you invoke the > hypercall with a NULL second argument. This just happens to work, > but is very unlikely what you intended to do. > Looks scratch_alloc() should be used to allocate in hvmloader, so what about this? diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c index 80d822f..90011fa 100644 --- a/tools/firmware/hvmloader/util.c +++ b/tools/firmware/hvmloader/util.c @@ -766,6 +766,16 @@ struct shared_info *get_shared_info(void) return shared_info; } +struct e820map *get_rmrr_map_info(void) +{ + struct e820map *e820_map = scratch_alloc(sizeof(struct e820map), 0); + + if ( hypercall_memory_op(XENMEM_RMRR_memory_map, e820_map) != 0 ) + BUG(); + + return e820_map; +} + uint16_t get_cpu_mhz(void) { struct shared_info *shared_info = get_shared_info(); Thanks Tiejun