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

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

On 2014/8/18 17:57, Andrew Cooper wrote:
> On 18/08/14 08:42, Chen, Tiejun wrote:
>> 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.
>
> Not in Xen.  Only as reported to guests, in which case an e820-like
> structure is most appropriate.
>
>>
>>>
>>> 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?
>
> Yes, but the hypercall handler can do this which avoids all need to
> store an intermediate representation in Xen.
>
> list_for_each_entry(rmrr, &acpi_rmrr_units, list)
> {
>      e820entry e;
>
>      e.start = ...
>
>      copy_to_guest_offset(...
> }
>

This is just what I mean. You always need to grab all info from 
acpi_rmrr_units to covert as a e820entry to copy.

So I guess you guys mean we should avoid duplicating RMRR many times in 
global. acpi_rmrr_units is fine enough, so I'd like to do as you expect.

Thanks
Tiejun

> But with appropriate error checking.
>
> ~Andrew
>
>