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

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

On 2014/9/4 11:04, Chen, Tiejun wrote:
> 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

Based on that first patch I think I should rename some terms in other 
patches:

s/device reserved memory/reserved device memory
s/DRM/RDM/
s/drm/rdm

Thanks
Tiejun

>
>>> +                                             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
>>
>>
>>
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>