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

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

On 2014/8/18 20:31, Jan Beulich wrote:
>>>>> Andrew Cooper <andrew.cooper3@citrix.com> 08/18/14 11:57 AM >>>
>> 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:
>>>>>> --- 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.
>
> E280-like yes, but ...
>
>>>> 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
>>> 	) 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(...
>> }
>
> ... as said before, I don't think using the E820 structure as-is is the right
> approach: Neither do we need byte-granular fields, nor do we need a type
> here.
>

Please don't say simply that e820entry is not suitable, what's your 
preferred structure here?

Looks you are saying something like,

struct __packed rmrr_entry {
     uint64_t addr;
     uint64_t size;
};

but compare that to the existing e820entry,

struct __packed e820entry {
     uint64_t addr;
     uint64_t size;
     uint32_t type;
};

Anyway, please show me your ideal structure then I'd like to follow-up 
that since it's no big deal.

Thanks
Tiejun