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

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

On 2014/8/8 17:01, Jan Beulich wrote:
>>>> On 08.08.14 at 10:39, <tiejun.chen@intel.com> 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