From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 A9E3321E1DAFF for ; Fri, 4 Aug 2017 13:02:24 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 424C4356F5; Fri, 4 Aug 2017 20:04:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 424C4356F5 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-151.phx2.redhat.com [10.3.116.151]) by smtp.corp.redhat.com (Postfix) with ESMTP id A9F1E5D9C0; Fri, 4 Aug 2017 20:04:35 +0000 (UTC) To: Jordan Justen , edk2-devel-01 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> <150183662138.26642.8135798941756670502@jljusten-skl> From: Laszlo Ersek Message-ID: <45baebf6-51d4-e81f-f2b3-f9a080ced353@redhat.com> Date: Fri, 4 Aug 2017 22:04:34 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <150183662138.26642.8135798941756670502@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 04 Aug 2017 20:04:37 +0000 (UTC) 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 20:02:24 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/04/17 10:50, Jordan Justen wrote: > 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 != 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. OK, I'll do that. > > The callback almost seems like something to consider for fw-cfg lib, > except I don't think we really have the need today. I agree that we don't need it now. > >>> >>> 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 = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) { >>>> + QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry); >>>> + DEBUG (( >>>> + DEBUG_VERBOSE, >>>> + "%a: Base=0x%Lx Length=0x%Lx Type=%u\n", >>>> + __FUNCTION__, >>>> + E820Entry.BaseAddr, >>>> + E820Entry.Length, >>>> + E820Entry.Type >>>> + )); >>>> + if (E820Entry.Type == EfiAcpiAddressRangeMemory && >>>> + E820Entry.BaseAddr >= 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. OK, thanks. I think it's out of scope for now. Thanks Laszlo