From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 E13338218A for ; Tue, 21 Feb 2017 17:31:16 -0800 (PST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga105.fm.intel.com with ESMTP; 21 Feb 2017 17:31:16 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,191,1484035200"; d="scan'208";a="51040734" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga002.jf.intel.com with ESMTP; 21 Feb 2017 17:31:16 -0800 Received: from fmsmsx119.amr.corp.intel.com (10.18.124.207) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 21 Feb 2017 17:31:16 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX119.amr.corp.intel.com (10.18.124.207) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 21 Feb 2017 17:31:15 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.88]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.132]) with mapi id 14.03.0248.002; Wed, 22 Feb 2017 09:31:13 +0800 From: "Fan, Jeff" To: Laszlo Ersek , "edk2-devel@ml01.01.org" CC: "Yao, Jiewen" , "Tian, Feng" , "Kinney, Michael D" Thread-Topic: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: Copy two functions from PciHostBridge Thread-Index: AQHSjDFIZmkL9Zerh06d/t6QUM/I4KF0PZVg Date: Wed, 22 Feb 2017 01:31:12 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4C542900@shsmsx102.ccr.corp.intel.com> References: <20170221030822.19548-1-jeff.fan@intel.com> <20170221030822.19548-2-jeff.fan@intel.com> <736649ee-27b8-067f-fea3-bff21cd6e3c6@redhat.com> In-Reply-To: <736649ee-27b8-067f-fea3-bff21cd6e3c6@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNjY5ZDUyNjYtZTgzYy00ZWNhLThiZDktNWE0NDdkZmIzNmVjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6InRHWEtiK1drRkFwYmd1ZysyTUcrYmN1YXJGamVuYk1odmNZTXUyZEVJcWs9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: Copy two functions from PciHostBridge X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Feb 2017 01:31:17 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, For 1), I will update this typo when pushing the serial of patches. Thanks! For2), I will do clean up on EFI_D to DEBUG_ in CpuDxe driver after Jiewen = & my patches pushed. Thanks! Jeff -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com]=20 Sent: Tuesday, February 21, 2017 6:57 PM To: Fan, Jeff; edk2-devel@ml01.01.org Cc: Yao, Jiewen; Tian, Feng; Kinney, Michael D Subject: Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: Copy two functions from PciH= ostBridge I have two suggestions for this patch: On 02/21/17 04:08, Jeff Fan wrote: > Copy AddMemoryMappedIoSpace() and AddMemoryMappedIoSpace() from=20 > MdeModulePkg\Bus\Pci\PciHostBridgeDxe\PciHostBridge.c. (1) One of the two "AddMemoryMappedIoSpace" strings should be replaced with= "IntersectMemoryDescriptor". This can be done when you commit / push the patch; no need to repost just b= ecause of it. (2) This is more of a "followup" suggestion: since this is new code, I reco= mmend that the EFI_D_ macros be replced with DEBUG_ macros. However, I see value in keeping the first patch as a pristine copy from the= PciHostBridgeDxe driver, so I don't recommend doing the above change in pa= tch #1. I think it can be patch #3, or a completely separate patch (later). Series Reviewed-by: Laszlo Ersek Regression-tested-by: Laszlo Ersek Thanks! Laszlo >=20 > https://bugzilla.tianocore.org/show_bug.cgi?id=3D390 >=20 > Cc: Laszlo Ersek > Cc: Jiewen Yao > Cc: Feng Tian > Cc: Michael D Kinney > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan > --- > UefiCpuPkg/CpuDxe/CpuDxe.c | 148=20 > +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 148 insertions(+) >=20 > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c=20 > index 9fb6d76..925c50f 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.c > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c > @@ -887,6 +887,154 @@ IdleLoopEventCallback ( > CpuSleep (); > } > =20 > +/** > + Ensure the compatibility of a memory space descriptor with the MMIO ap= erture. > + > + The memory space descriptor can come from the GCD memory space map,=20 > + or it can represent a gap between two neighboring memory space=20 > + descriptors. In the latter case, the GcdMemoryType field is=20 > + expected to be EfiGcdMemoryTypeNonExistent. > + > + If the memory space descriptor already has type =20 > + EfiGcdMemoryTypeMemoryMappedIo, and its capabilities are a superset=20 > + of the required capabilities, then no action is taken -- it is by=20 > + definition compatible with the aperture. > + > + Otherwise, the intersection of the memory space descriptor is=20 > + calculated with the aperture. If the intersection is the empty set=20 > + (no overlap), no action is taken; the memory space descriptor is compa= tible with the aperture. > + > + Otherwise, the type of the descriptor is investigated again. If the=20 > + type is EfiGcdMemoryTypeNonExistent (representing a gap, or a=20 > + genuine descriptor with such a type), then an attempt is made to=20 > + add the intersection as MMIO space to the GCD memory space map,=20 > + with the specified capabilities. This ensures continuity for the=20 > + aperture, and the descriptor is deemed compatible with the aperture. > + > + Otherwise, the memory space descriptor is incompatible with the=20 > + MMIO aperture. > + > + @param[in] Base Base address of the aperture. > + @param[in] Length Length of the aperture. > + @param[in] Capabilities Capabilities required by the aperture. > + @param[in] Descriptor The descriptor to ensure compatibility with th= e > + aperture for. > + > + @retval EFI_SUCCESS The descriptor is compatible. The GCD m= emory > + space map may have been updated, for > + continuity within the aperture. > + @retval EFI_INVALID_PARAMETER The descriptor is incompatible. > + @return Error codes from gDS->AddMemorySpace(). > +**/ > +EFI_STATUS > +IntersectMemoryDescriptor ( > + IN UINT64 Base, > + IN UINT64 Length, > + IN UINT64 Capabilities, > + IN CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Descriptor > + ) > +{ > + UINT64 IntersectionBase; > + UINT64 IntersectionEnd; > + EFI_STATUS Status; > + > + if (Descriptor->GcdMemoryType =3D=3D EfiGcdMemoryTypeMemoryMappedIo && > + (Descriptor->Capabilities & Capabilities) =3D=3D Capabilities) { > + return EFI_SUCCESS; > + } > + > + IntersectionBase =3D MAX (Base, Descriptor->BaseAddress); =20 > + IntersectionEnd =3D MIN (Base + Length, > + Descriptor->BaseAddress + Descriptor->Length); =20 > + if (IntersectionBase >=3D IntersectionEnd) { > + // > + // The descriptor and the aperture don't overlap. > + // > + return EFI_SUCCESS; > + } > + > + if (Descriptor->GcdMemoryType =3D=3D EfiGcdMemoryTypeNonExistent) { > + Status =3D gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo, > + IntersectionBase, IntersectionEnd - IntersectionBase= , > + Capabilities); > + > + DEBUG ((EFI_ERROR (Status) ? EFI_D_ERROR : EFI_D_VERBOSE, > + "%a: %a: add [%Lx, %Lx): %r\n", gEfiCallerBaseName, __FUNCTION__, > + IntersectionBase, IntersectionEnd, Status)); > + return Status; > + } > + > + DEBUG ((EFI_D_ERROR, "%a: %a: desc [%Lx, %Lx) type %u cap %Lx conflict= s " > + "with aperture [%Lx, %Lx) cap %Lx\n", gEfiCallerBaseName, __FUNCTION= __, > + Descriptor->BaseAddress, Descriptor->BaseAddress + Descriptor->Lengt= h, > + (UINT32)Descriptor->GcdMemoryType, Descriptor->Capabilities, > + Base, Base + Length, Capabilities)); > + return EFI_INVALID_PARAMETER; > +} > + > +/** > + Add MMIO space to GCD. > + The routine checks the GCD database and only adds those which are > + not added in the specified range to GCD. > + > + @param Base Base address of the MMIO space. > + @param Length Length of the MMIO space. > + @param Capabilities Capabilities of the MMIO space. > + > + @retval EFI_SUCCES The MMIO space was added successfully. > +**/ > +EFI_STATUS > +AddMemoryMappedIoSpace ( > + IN UINT64 Base, > + IN UINT64 Length, > + IN UINT64 Capabilities > + ) > +{ > + EFI_STATUS Status; > + UINTN Index; > + UINTN NumberOfDescriptors; > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemorySpaceMap; > + > + Status =3D gDS->GetMemorySpaceMap (&NumberOfDescriptors,=20 > + &MemorySpaceMap); if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a: %a: GetMemorySpaceMap(): %r\n", > + gEfiCallerBaseName, __FUNCTION__, Status)); > + return Status; > + } > + > + for (Index =3D 0; Index < NumberOfDescriptors; Index++) { > + Status =3D IntersectMemoryDescriptor (Base, Length, Capabilities, > + &MemorySpaceMap[Index]); > + if (EFI_ERROR (Status)) { > + goto FreeMemorySpaceMap; > + } > + } > + > + DEBUG_CODE ( > + // > + // Make sure there are adjacent descriptors covering [Base, Base + L= ength). > + // It is possible that they have not been merged; merging can be pre= vented > + // by allocation and different capabilities. > + // > + UINT64 CheckBase; > + EFI_STATUS CheckStatus; > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR Descriptor; > + > + for (CheckBase =3D Base; > + CheckBase < Base + Length; > + CheckBase =3D Descriptor.BaseAddress + Descriptor.Length) { > + CheckStatus =3D gDS->GetMemorySpaceDescriptor (CheckBase, &Descrip= tor); > + ASSERT_EFI_ERROR (CheckStatus); > + ASSERT (Descriptor.GcdMemoryType =3D=3D EfiGcdMemoryTypeMemoryMapp= edIo); > + ASSERT ((Descriptor.Capabilities & Capabilities) =3D=3D Capabiliti= es); > + } > + ); > + > +FreeMemorySpaceMap: > + FreePool (MemorySpaceMap); > + > + return Status; > +} > =20 > /** > Initialize the state information for the CPU Architectural Protocol. >=20