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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.08.14 at 13:02, <tiejun.chen@xxxxxxxxx> wrote:
> +static unsigned int construct_rmrr_e820_maps(unsigned int nr,
> +                                             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;
> +    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).

> +    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).

> +    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).

> +
> +            /* Between those existing e820 entries. */
> +            if ( (rmrr_start > end) && (rmrr_end < start) )
> +            {
> +                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)
> +    {
> +        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");
> +    }
> +
> +    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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel