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

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

On 2014/8/19 10:14, Chen, Tiejun wrote:
> 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;
> };
>

Another concern is that we always use xen_memory_map for the hypercall,

struct xen_memory_map {
     /*
      * On call the number of entries which can be stored in buffer. On
      * return the number of entries which have been stored in
      * buffer.
      */
     unsigned int nr_entries;

     /*
      * Entries in the buffer are in the same format as returned by the
      * BIOS INT 0x15 EAX=0xE820 call.
      */
     XEN_GUEST_HANDLE(void) buffer;
};

As it comments above, theoretical e820 is expected in buffer.

Thanks
Tiejun

> Anyway, please show me your ideal structure then I'd like to follow-up
> that since it's no big deal.
>
> Thanks
> Tiejun
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>