Re: [RFC][PATCH 4/5] tools:firmware:hvmloader: reserve RMRR mappings in e820

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

On 2014/8/8 14:42, Jan Beulich wrote:
>>>> On 08.08.14 at 04:11, <tiejun.chen@intel.com> 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