From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org 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 350112034BBF8 for ; Thu, 9 Nov 2017 04:15:59 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 494855D9E4; Thu, 9 Nov 2017 12:20:01 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-19.rdu2.redhat.com [10.10.120.19]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0C39961F41; Thu, 9 Nov 2017 12:19:59 +0000 (UTC) To: "Yao, Jiewen" , "Wang, Jian J" , "edk2-devel@lists.01.org" Cc: "Dong, Eric" References: <20171103005729.7856-1-jian.j.wang@intel.com> <82c64ab0-25b3-5f7d-cf99-c0d2f87e99da@redhat.com> <74D8A39837DF1E4DA445A8C0B3885C503AA15BD3@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <8a2fce3c-bd64-7899-8f57-cb4ebb45f6dd@redhat.com> Date: Thu, 9 Nov 2017 13:19:58 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503AA15BD3@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 09 Nov 2017 12:20:01 +0000 (UTC) Subject: Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map 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: Thu, 09 Nov 2017 12:16:00 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/09/17 02:48, Yao, Jiewen wrote: > FED00000 is HPET region. It is MMIO. Right, we add it in OvmfPkg/PlatformPei/Platform.c: // // address purpose size // ------------ -------- ------------------------- ... // 0xFED00000 HPET 1 KB ... AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB); Because this is MMIO and not system memory, I think the 1KB size is valid, as far as a resource descriptor HOB is concerned. This goes on to support my earlier suggestion to check the GCD memory space entry more strictly, for its type. Anyway, the latest v3 (v4?) approach can handle the entry just as well, so I don't insist on modifying the entry type check. I was mainly curious if we did something wrong in OVMF. I believe the above HOB is valid though. Thanks Laszlo >> -----Original Message----- >> From: Wang, Jian J >> Sent: Thursday, November 9, 2017 8:42 AM >> To: Laszlo Ersek ; edk2-devel@lists.01.org >> Cc: Yao, Jiewen ; Dong, Eric >> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of >> RT_CODE in memory map >> >> Hi Laszlo, >> >> The memory range is 00000000FED00000 - 00000000FED003FF. Actually I don't >> know if it's for MMIO or not. I might be mess it with other things. >> >> Thanks, >> Jian >>> -----Original Message----- >>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>> Sent: Wednesday, November 08, 2017 10:17 PM >>> To: Wang, Jian J ; edk2-devel@lists.01.org >>> Cc: Yao, Jiewen ; Dong, Eric >>> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of >>> RT_CODE in memory map >>> >>> On 11/08/17 01:10, Wang, Jian J wrote: >>>> Hi Laszlo, >>>> >>>>> -----Original Message----- >>>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>>> Sent: Wednesday, November 08, 2017 1:14 AM >>>>> To: Wang, Jian J ; edk2-devel@lists.01.org >>>>> Cc: Yao, Jiewen ; Dong, Eric >>>>> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of >>>>> RT_CODE in memory map >>>>> >>>>> sorry about the late response >>>>> >>>>> On 11/03/17 01:57, Jian J Wang wrote: >>>>>>> v2 >>>>>>> a. Fix an issue which will cause setting capability failure if size is smaller >>>>>>> than a page. >>>>>> >>>>>> More than one entry of RT_CODE memory might cause boot problem for >>>>> some >>>>>> old OSs. This patch will fix this issue to keep OS compatibility as much >>>>>> as possible. >>>>>> >>>>>> More detailed information, please refer to >>>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=753 >>>>>> >>>>>> Cc: Eric Dong >>>>>> Cc: Jiewen Yao >>>>>> Cc: Laszlo Ersek >>>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>>> Signed-off-by: Jian J Wang >>>>>> --- >>>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++---- >>>>>> 1 file changed, 14 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c >>>>> b/UefiCpuPkg/CpuDxe/CpuPageTable.c >>>>>> index d312eb66f8..4a7827ebc9 100644 >>>>>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c >>>>>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c >>>>>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging ( >>>>>> PageLength = 0; >>>>>> >>>>>> for (Index = 0; Index < NumberOfDescriptors; Index++) { >>>>>> - if (MemorySpaceMap[Index].GcdMemoryType == >>>>> EfiGcdMemoryTypeNonExistent) { >>>>>> + if (MemorySpaceMap[Index].GcdMemoryType == >>>>> EfiGcdMemoryTypeNonExistent >>>>>> + || (MemorySpaceMap[Index].BaseAddress & >> EFI_PAGE_MASK) != 0 >>>>>> + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) >> { >>>>>> continue; >>>>>> } >>>>> >>>>> When exactly do the new conditions match? >>>>> >>>>> I thought the base addresses and the lengths in the GCD memory space >> map >>>>> are all page aligned. Is that not the case? >>>>> >>>>> If these conditions are just a sanity check (i.e. we never expect them >>>>> to fire), then should we perpahs turn them into ASSERT()s? >>>>> >>>> >>>> I found that there's a mmio entry in memory map on OVMF which has size >>>> less than a page. I didn't encounter this before. Maybe some recent changes >>>> in other part of EDKII caused this situation. So ASSERT is not enough. >>> >>> Can you describe the base address and size of this MMIO entry? >>> >>> I don't see how it is possible to add such an entry in the first place >>> -- if you try to add it in PEI, via a HOB, then IIRC HobLib will >>> ASSERT(). If you try to add it with gDS->AddMemorySpace() in DXE, then >>> the call should fail. >>> >>> I believe it might be useful to investigate this entry more closely. >>> Knowing the base address could help us. >>> >>> Thanks! >>> Laszlo