Re: [Xen-devel] [v6][PATCH 6/7] hvmloader: check to reserved device memory maps in e820

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.09.14 at 07:49, <tiejun.chen@xxxxxxxxx> wrote:
> +static unsigned int construct_rdm_e820_maps(unsigned int nr,
> +                                            uint32_t nr_map,
> +                                            struct 
> xen_mem_reserved_device_memory *map,
> +                                            struct e820entry *e820)
> +{
> +    unsigned int i = 0, j = 0, m = 0, sum_nr = nr + nr_map;

Some or all of the initializers here are pointless.

> +    uint64_t start, end, rdm_start, rdm_end;
> +    unsigned int insert = 0, do_insert = 0;
> +
> + do_real_construct:
> +    for ( i = 0; i < nr_map; i++ )
> +    {
> +        rdm_start = map[i].start_pfn << PAGE_SHIFT;
> +        rdm_end = rdm_start + map[i].nr_pages * PAGE_SIZE;

Same comment as for an earlier patch.

> +
> +        for ( j = 0; j < nr - 1; j++ )
> +        {
> +            end = e820[j].addr + e820[j].size;
> +            start = e820[j+1].addr;

So now you don't check the last region at all? You just need to make
this assignment conditional upon whether you're in the last iteration.

> +
> +            /* Between those existing e820 entries. */
> +            if ( (rdm_start > end) && (rdm_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;
> +                    }

You know of the memmove() function, don't you?

> +
> +                    /* Then fill RMRR into that entry. */
> +                    e820[j+1].addr = rdm_start;
> +                    e820[j+1].size = rdm_end - rdm_start;
> +                    e820[j+1].type = E820_RESERVED;
> +                    nr++;
> +                }
> +                insert++;
> +            }
> +            /* Already at the end. */
> +            else if ( (rdm_start > end) && !start )

How would !start represent the end of anything?

> +            {
> +                if ( do_insert )
> +                {
> +                    e820[nr].addr = rdm_start;
> +                    e820[nr].size = rdm_end - rdm_start;
> +                    e820[nr].type = E820_RESERVED;
> +                    nr++;
> +                }
> +                insert++;
> +            }
> +        }
> +    }
> +
> +    /* Just return if done. */
> +    if ( do_insert )
> +        return nr;
> +
> +    /* Fine to construct RDM mappings into e820. */
> +    if ( insert == nr_map )
> +    {
> +        do_insert = 1;
> +        goto do_real_construct;
> +    }
> +    /* Overlap. */
> +    else
> +    {
> +        printf("RDM overlap with those existing e820 entries!\n");
> +        printf("So we don't construct RDM mapping in e820!\n");
> +    }
> +
> +    return nr;
> +}
>  /* Create an E820 table based on memory parameters provided in hvm_info. */

Blank line missing above.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel