From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8E19B2095D9C2 for ; Fri, 4 Aug 2017 01:48:14 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Aug 2017 01:50:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,320,1498546800"; d="scan'208";a="119664484" Received: from ldtyree-mobl.amr.corp.intel.com (HELO localhost) ([10.255.79.10]) by orsmga002.jf.intel.com with ESMTP; 04 Aug 2017 01:50:22 -0700 MIME-Version: 1.0 To: Laszlo Ersek , edk2-devel-01 Message-ID: <150183662138.26642.8135798941756670502@jljusten-skl> From: Jordan Justen In-Reply-To: <227cab99-7986-8161-4d32-7ad0679bf301@redhat.com> Cc: "Dr. David Alan Gilbert" References: <20170711032231.29280-1-lersek@redhat.com> <20170711032231.29280-2-lersek@redhat.com> <150102801569.20332.10802753116663936503@jljusten-skl> <227cab99-7986-8161-4d32-7ad0679bf301@redhat.com> User-Agent: alot/0.5.1 Date: Fri, 04 Aug 2017 01:50:22 -0700 Subject: Re: [PATCH 1/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Aug 2017 08:48:14 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2017-07-26 09:23:26, Laszlo Ersek wrote: > On 07/26/17 02:13, Jordan Justen wrote: > > On 2017-07-10 20:22:31, Laszlo Ersek wrote: > >> +STATIC > >> +EFI_STATUS > >> +E820HighRamIterate ( > >> + IN E820_HIGH_RAM_ENTRY_CALLBACK Callback, > >> + IN OUT VOID *Context > >> + ) > > = > > I think a simpler option would be: > > = > > STATIC > > EFI_STATUS > > ScanOrAddE820HighRam ( > > IN E820_HIGH_RAM_ENTRY_CALLBACK Callback, > > OUT UINT64 *MaxAddress OPTIONAL > > ) > > > > If MaxAddress !=3D NULL, then scan for it, otherwise add HOBs. > > = > > Do you anticipate future needs where the iterate callback could be > > helpful? > = > Not at the moment. > = > Originally I started with two open-coded loops, but the code duplication > in this case was really ugly. Using callbacks simplified the call sites > very nicely. And, I was a bit concerned that you wouldn't like a > solution that wasn't generic enough :) > = > I can rework the function like suggested if you prefer that. I moved the code of the 2 callbacks into the the function and dropped the callbacks to see how it looked. It seemed a bit clearer and was less code. The callback almost seems like something to consider for fw-cfg lib, except I don't think we really have the need today. > > = > > You might also consider ScanOrAdd64BitE820Ram to somewhat clarify that > > the 'HighRam' is addresses that don't fit in 32 bits. > = > OK. > = > > = > >> + QemuFwCfgSelectItem (FwCfgItem); > >> + for (Processed =3D 0; Processed < FwCfgSize; Processed +=3D sizeof = E820Entry) { > >> + QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry); > >> + DEBUG (( > >> + DEBUG_VERBOSE, > >> + "%a: Base=3D0x%Lx Length=3D0x%Lx Type=3D%u\n", > >> + __FUNCTION__, > >> + E820Entry.BaseAddr, > >> + E820Entry.Length, > >> + E820Entry.Type > >> + )); > >> + if (E820Entry.Type =3D=3D EfiAcpiAddressRangeMemory && > >> + E820Entry.BaseAddr >=3D BASE_4GB) { > > = > > I guess at least for IA32/X64, today, and for the foreseeable future > > the firmware device will cause a break at 4GB. > = > Sorry, I don't understand. What do you mean by "firmware device" and > "break at 4GB"? > = > > It seems like we could just check for the end of the range to be above > > 4GB to easily remove this assumption, right? > = > I wanted to ensure that no RAM range would be included that (for any > unexpected reason) straddled the 4GB mark. > = > Do you mean that the pflash address range is guaranteed to end at 4GB > (exclusive), so no RAM range can straddle the mark? > = > Even so, what expression do you have in mind exactly? As far as I can > imagine, checking the end vs. the base of the E820 entry would only > complicate the above expression, not simplify it. > = > Please elaborate :) Yeah, I meant is there any easy way to handle a situation (unlike qemu piix4/q35 systems) where a RAM region went from below 4GB to above 4GB. If you don't think there is a simple was to handle it, then don't worry about it. -Jordan