Re: [RFC][v3][PATCH 3/6] tools:firmware:hvmloader: reserve RMRR mappings in e820

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

On 2014/8/15 17:58, Andrew Cooper wrote:
> On 15/08/14 09:27, Tiejun Chen wrote:
>> We need to reserve all RMRR mappings in e820 to avoid any
>> potential guest memory conflict.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/firmware/hvmloader/e820.c | 94 +++++++++++++++++++++++++++++++++++++++++
>>   tools/firmware/hvmloader/e820.h |  6 +++
>>   tools/firmware/hvmloader/util.c | 16 +++++++
>>   tools/firmware/hvmloader/util.h |  2 +
>>   4 files changed, 118 insertions(+)
>>
>> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
>> index 2e05e93..7f54eab 100644
>> --- a/tools/firmware/hvmloader/e820.c
>> +++ b/tools/firmware/hvmloader/e820.c
>> @@ -68,12 +68,89 @@ void dump_e820_table(struct e820entry *e820, unsigned int nr)
>>       }
>>   }
>
> Please apply Xen coding style to all of this file - there are a lot of
> errors.

I asked previously what Xen coding style is, and I always try to follow 
some existing code but seems its not enough. Here I still can't find out 
what you're pointing, so please tell me exactly what I should change 
exactly.

And why don't you guy generate a checkpatch.pl file?

>
>>
>> +static unsigned int construct_rmrr_e820_maps(unsigned int nr,
>> +                                     struct rmrr_map *e820_rmrr_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:
>> +    sum_nr = nr + e820_rmrr_map->nr_map;
>> +    for ( i = 0; i < e820_rmrr_map->nr_map; i++ )
>> +    {
>> +        rmrr_start = e820_rmrr_map->map[i].addr;
>> +        rmrr_end = e820_rmrr_map->map[i].addr +
>> +                   e820_rmrr_map->map[i].size + 1;
>> +
>> +        for ( j = 0; j < nr; j++ )
>> +        {
>> +            end = e820[j].addr + e820[j].size;
>> +            start = e820[j+1].addr;
>> +
>> +            /* 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 == e820_rmrr_map->nr_map)
>> +    {
>> +        do_insert = 1;
>> +        goto do_real_construct;
>> +    }
>> +    /* Overlap. */
>> +    else
>> +    {
>> +        printf("RMRR overlap with thoese 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 rmrr_map *rmrr_maps;
>>
>>       if ( !lowmem_reserved_base )
>>               lowmem_reserved_base = 0xA0000;
>> @@ -169,6 +246,23 @@ int build_e820_table(struct e820entry *e820,
>>           nr++;
>>       }
>>
>> +    /* We'd better reserve RMRR mapping for each VM to avoid potential
>> +     * memory conflict.
>> +     */
>> +    rmrr_maps = get_rmrr_map_info();
>> +    if ( rmrr_maps->nr_map )
>> +    {
>> +        if ( (nr + rmrr_maps->nr_map) > E820MAX )
>> +        {
>> +            printf(" No free space to insert all RMRR mapping entry!!\n");
>> +            return nr;
>> +        }
>> +        else
>> +        {
>> +            nr = construct_rmrr_e820_maps(nr, rmrr_maps, e820);
>> +        }
>> +    }
>> +
>>       return nr;
>>   }
>>
>> diff --git a/tools/firmware/hvmloader/e820.h b/tools/firmware/hvmloader/e820.h
>> index b2ead7f..a61b80b 100644
>> --- a/tools/firmware/hvmloader/e820.h
>> +++ b/tools/firmware/hvmloader/e820.h
>> @@ -15,6 +15,12 @@ struct e820entry {
>>       uint32_t type;
>>   } __attribute__((packed));
>>
>> +#define E820MAX 128
>> +
>> +struct rmrr_map {
>> +    unsigned int nr_map;
>> +    struct e820entry map[E820MAX];
>> +};
>>   #endif /* __HVMLOADER_E820_H__ */
>>
>>   /*
>> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
>> index 80d822f..f63434a 100644
>> --- a/tools/firmware/hvmloader/util.c
>> +++ b/tools/firmware/hvmloader/util.c
>> @@ -766,6 +766,22 @@ struct shared_info *get_shared_info(void)
>>       return shared_info;
>>   }
>>
>> +struct rmrr_map *get_rmrr_map_info(void)
>> +{
>> +    static int no_rmrr = 1;
>> +
>> +    if ( no_rmrr == 0 )
>> +        return &rmrr_e820map;
>> +
>> +    if ( hypercall_memory_op(XENMEM_reserved_device_memory_map,
>> +                             &rmrr_e820map) != 0 )
>
> This *only* works because you are not passing a xen_rmrr_memory_map_t as
> per the hypercall definition, and Xen is clobbering your heap in a way
> which matches the hvmloader layout.

I will check this.

Thanks
Tiejun

>
>> +        BUG();
>> +
>> +    no_rmrr = 0;
>> +
>> +    return &rmrr_e820map;
>> +}
>> +
>>   uint16_t get_cpu_mhz(void)
>>   {
>>       struct shared_info *shared_info = get_shared_info();
>> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
>> index a70e4aa..5f48a86 100644
>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -236,6 +236,8 @@ unsigned long create_pir_tables(void);
>>   void smp_initialise(void);
>>
>>   #include "e820.h"
>> +struct rmrr_map rmrr_e820map;
>
> Unless I am mistaken, this will put an rmrr_map object named
> rmrr_e820map in each translation unit which includes util.h  I suspect
> it is not what you actually want to do.
>
> ~Andrew
>
>> +struct rmrr_map *get_rmrr_map_info(void);
>>   int build_e820_table(struct e820entry *e820,
>>                        unsigned int lowmem_reserved_base,
>>                        unsigned int bios_image_base);
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>