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

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

On 2014/8/8 15:43, Jan Beulich wrote:
>>>> On 08.08.14 at 09:30, <tiejun.chen@intel.com> wrote:
>> On 2014/8/8 14:42, Jan Beulich wrote:
>>> 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?
>>
>> --- 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;
>> +}
>
> 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;
+}
+
  uint16_t get_cpu_mhz(void)
  {
      struct shared_info *shared_info = get_shared_info();
diff --git a/tools/firmware/hvmloader/util.h 
b/tools/firmware/hvmloader/util.h
index a70e4aa..433be99 100644
--- 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.

Thanks
Tiejun

> E820 interface structure looks pretty odd here (I simply didn't get
> to looking at your patches in full yet).
>
> Jan
>
>
>