Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings

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

On 2014/8/16 0:29, Jan Beulich wrote:
>>>> On 15.08.14 at 11:39, <andrew.cooper3@citrix.com> wrote:
>>> @@ -80,6 +81,18 @@ static int __init acpi_register_rmrr_unit(struct acpi_rmrr_unit *rmrr)
>>>       return 0;
>>>   }
>>>
>>> +/* Record RMRR mapping to ready expose VM. */
>>> +static int __init rmrr_maps_register(struct acpi_rmrr_unit *rmrr)
>>> +{
>>
>> You absolutely need some protection against calling this function more
>> than 128 times, or you need to do dynamic allocation of the storage.
>
> This together with ...
>
>>> --- a/xen/include/asm-x86/e820.h
>>> +++ b/xen/include/asm-x86/e820.h
>>> @@ -23,6 +23,8 @@ struct e820map {
>>>       struct e820entry map[E820MAX];
>>>   };
>>>
>>> +typedef struct e820map rmrr_maps_t;
>>
>> This type is a single map of RMRR regions, not multiple maps.
>> rmrr_map_t please.
>
> ... this once again stresses what I stated previously: Piggybacking
> on the E820 handling here is just the wrong approach. There's
> really no correlation with E820 other than us wanting to use the
> gathered information for (among other things) adjusting the guest
> E820 table. But that doesn't in any way require any re-use of
> non-suitable data structures.

Why are you saying this is not suitable?

We need a structure to represent a RMRR entry including three fields, 
start, size and type, and especially, essentially RMRR entry belongs to 
e820 table as one entry.

>
> In fact I don't see the need for this first patch anyway, as RMRRs
> are already being put on a linked list as they get found. I.e. the

Yes, that list, acpi_rmrr_unit, can be exposed here. But before you copy 
to guest, don't you need to grab those fields from that list then 
convert them as a suitable structure (mostly this is still same as 
e820entry) to be copied into a buffer?

Thanks
Tiejun

> patch here just clones existing information.
>
> Jan
>
>