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

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

On 2014/9/11 23:57, Jan Beulich wrote:
>>>> On 10.09.14 at 07:49, <tiejun.chen@intel.com> 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.

     unsigned int i, j, m, sum_nr = nr + nr_map;
     uint64_t start, end, rdm_start, rdm_end;
     unsigned int insert, do_insert;

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

         rdm_start = map[i].start_pfn << XC_PAGE_SHIFT;
         rdm_end = rdm_start + map[i].nr_pages << XC_PAGE_SHIFT;

But there's no such definition in tools/firmware/, and this is defined 
in xenctl.h. But if I include xenctl.h here, something else is 
redefined. So here I have to define this, XC_PAGE_SHIFTseparately.

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

I remember we discussed this previously.

"
 > And this would skip the last region - I'm not sure that's correct.
 >

No. Here we just want to know where we should insert a entry, so

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

For example, nr = 3,

     for ( j = 0; j < 2; j++ )

So
#1,
         end = e820[0].addr + e820[0].size;
         start = e820[1].addr;

#2,
         end = e820[1].addr + e820[1].size;
         start = e820[2].addr;
"

I didn't you have further comments there.

>
>> +
>> +            /* 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?

I will try this.

>
>> +
>> +                    /* 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?

end = e820[j].addr + e820[j].size;
start = e820[j+1].addr;

so if end = 0xXXXXXXXX and start = 0, does this mean we already are at 
the end of all valid e820 entries?

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

Will fixed.

Thanks
Tiejun