From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [RFC][v3][PATCH 1/6] xen:x86: record RMRR mappings Date: Mon, 18 Aug 2014 18:05:47 +0800 Message-ID: <53F1CFFB.7020000@intel.com> References: <1408091238-18364-1-git-send-email-tiejun.chen@intel.com> <1408091238-18364-2-git-send-email-tiejun.chen@intel.com> <53EDD539.90403@citrix.com> <53EE436502000078000BA911@mail.emea.novell.com> <53F1AE4C.5050002@intel.com> <53F1CE0D.7080005@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53F1CE0D.7080005@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , Jan Beulich , ian.campbell@citrix.com, ian.jackson@eu.citrix.com, stefano.stabellini@eu.citrix.com, kevin.tian@intel.com, yang.z.zhang@intel.com Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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, 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 > >