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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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(...
}

But with appropriate error checking.

~Andrew