Re: [v5][PATCH 09/10] tools:firmware:hvmloader: check to reserve RMRR mappings in e820

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

On 2014/9/2 16:47, Jan Beulich wrote:
>>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>> +static unsigned int construct_rmrr_e820_maps(unsigned int nr,

s/construct_rmrr_e820_maps/construct_drm_e820_maps

>> +                                             uint32_t nr_map,
>> +                                             struct reserved_device_memory *map,
>> +                                             struct e820entry *e820)
>> +{
>> +    unsigned int i = 0, j = 0, m = 0, sum_nr = 0;
>> +    uint64_t start, end, rmrr_start, rmrr_end;

I rename this pair of variables in this whole function,

s/rmrr_start/drm_start
s/rmrr_end/drm_end

>> +    unsigned int insert = 0, do_insert = 0;
>> +
>> +do_real_construct:
>
> Please indent labels by at least one space (and there are further
> coding style issues to address elsewhere).

Is this necessary?

static struct tmem_object_root * obj_find(struct tmem_pool *pool, struct 
oid *oidp)
{
     struct rb_node *node;
     struct tmem_object_root *obj;

restart_find:
     read_lock(&pool->pool_rwlock);
     ...
                     read_unlock(&pool->pool_rwlock);
                     goto restart_find;

>
>> +    sum_nr = nr + nr_map;
>
> Afaict neither nr nor nr_map change before you get here the second
> (and last) time. Hence the calculation can be moved up (ideally into
> the initializer of the variable).

+    unsigned int i = 0, j = 0, m = 0, sum_nr = nr + nr_map;

>
>> +    for ( i = 0; i < nr_map; i++ )
>> +    {
>> +        rmrr_start = map[i].start_pfn << PAGE_SHIFT;
>> +        rmrr_end = rmrr_start + map[i].nr_pages * PAGE_SIZE;
>> +
>> +        for ( j = 0; j < nr; j++ )
>> +        {
>> +            end = e820[j].addr + e820[j].size;
>> +            start = e820[j+1].addr;
>
> This is not valid when j == nr - 1 (last iteration).
>
-        for ( j = 0; j < nr; j++ )
+        for ( j = 0; j < nr - 1; j++ )

>> +
>> +            /* Between those existing e820 entries. */
>> +            if ( (rmrr_start > end) && (rmrr_end < start) )
>> +            {
>> +                if (do_insert)

-                if (do_insert)
+                if ( do_insert )

>> +                {
>> +                    /* Move to free this entry. */
>> +                    for ( m = sum_nr - 1; m > j; m-- )
>> +                    {
>> +                        e820[m].addr = e820[m-1].addr;
>> +                        e820[m].size = e820[m-1].size;
>> +                        e820[m].type = e820[m-1].type;
>> +                    }
>> +
>> +                    /* Then fill RMRR into that entry. */
>> +                    e820[j+1].addr = rmrr_start;
>> +                    e820[j+1].size = rmrr_end - rmrr_start;
>> +                    e820[j+1].type = E820_RESERVED;
>> +                    nr++;
>> +                }
>> +                insert++;
>> +            }
>> +            /* Already at the end. */
>> +            else if ( (rmrr_start > end) && !start )
>> +            {
>> +                if ( do_insert )
>> +                {
>> +                    e820[nr].addr = rmrr_start;
>> +                    e820[nr].size = rmrr_end - rmrr_start;
>> +                    e820[nr].type = E820_RESERVED;
>> +                    nr++;
>> +                }
>> +                insert++;
>> +            }
>> +        }
>> +    }
>> +
>> +    /* Just return if done. */
>> +    if ( do_insert )
>> +        return nr;
>> +
>> +    /* Fine to construct RMRR mappings into e820. */
>> +    if ( insert == nr_map)

-    if ( insert == nr_map)
+    if ( insert == nr_map )

>> +    {
>> +        do_insert = 1;
>> +        goto do_real_construct;
>> +    }
>> +    /* Overlap. */
>> +    else
>> +    {
>> +        printf("RMRR overlap with those existing e820 entries!\n");
>> +        printf("So we don't construct RMRR mapping in e820!\n");

s/RMRR/DRM

Thanks
Tiejun

>> +    }
>> +
>> +    return nr;
>> +}
>>   /* Create an E820 table based on memory parameters provided in hvm_info. */
>>   int build_e820_table(struct e820entry *e820,
>>                        unsigned int lowmem_reserved_base,
>>                        unsigned int bios_image_base)
>>   {
>>       unsigned int nr = 0;
>> +    struct reserved_device_memory *map = NULL;
>> +    int rc;
>>
>>       if ( !lowmem_reserved_base )
>>               lowmem_reserved_base = 0xA0000;
>> @@ -169,6 +247,29 @@ int build_e820_table(struct e820entry *e820,
>>           nr++;
>>       }
>>
>> +    /* We'd better reserve RMRR mapping for each VM to avoid potential
>> +     * memory conflict.
>> +     */
>> +    if ( hvm_info->nr_reserved_device_memory_map )
>> +    {
>> +        if ( !map )
>> +            map = mem_alloc(hvm_info->nr_reserved_device_memory_map *
>> sizeof(struct reserved_device_memory), 0);
>> +        if ( map )
>> +        {
>> +            rc = get_reserved_device_memory_map(map,
>> +
>> hvm_info->nr_reserved_device_memory_map);
>> +            if ( !rc )
>> +            {
>> +                nr = construct_rmrr_e820_maps(nr,
>> +
>> hvm_info->nr_reserved_device_memory_map,
>> +                                              map,
>> +                                              e820);
>> +            }
>> +        }
>> +        else
>> +            printf("No space to get reserved_device_memory_map!\n");
>> +    }
>> +
>>       return nr;
>>   }
>>
>> --
>> 1.9.1
>
>
>
>