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 E3EF182212 for ; Tue, 21 Feb 2017 02:57:20 -0800 (PST) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5F73461B91; Tue, 21 Feb 2017 10:57:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-47.phx2.redhat.com [10.3.116.47]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1LAvJk1020333; Tue, 21 Feb 2017 05:57:19 -0500 To: Jeff Fan , edk2-devel@ml01.01.org References: <20170221030822.19548-1-jeff.fan@intel.com> <20170221030822.19548-2-jeff.fan@intel.com> Cc: Jiewen Yao , Feng Tian , Michael D Kinney From: Laszlo Ersek Message-ID: <736649ee-27b8-067f-fea3-bff21cd6e3c6@redhat.com> Date: Tue, 21 Feb 2017 11:57:17 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <20170221030822.19548-2-jeff.fan@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 21 Feb 2017 10:57:21 +0000 (UTC) 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: Tue, 21 Feb 2017 10:57:21 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit I have two suggestions for this patch: On 02/21/17 04:08, Jeff Fan wrote: > Copy AddMemoryMappedIoSpace() and AddMemoryMappedIoSpace() from > 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 because of it. (2) This is more of a "followup" suggestion: since this is new code, I recommend 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 patch #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 > > https://bugzilla.tianocore.org/show_bug.cgi?id=390 > > 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 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 148 insertions(+) > > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c > index 9fb6d76..925c50f 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.c > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c > @@ -887,6 +887,154 @@ IdleLoopEventCallback ( > CpuSleep (); > } > > +/** > + Ensure the compatibility of a memory space descriptor with the MMIO aperture. > + > + The memory space descriptor can come from the GCD memory space map, or it can > + represent a gap between two neighboring memory space descriptors. In the > + latter case, the GcdMemoryType field is expected to be > + EfiGcdMemoryTypeNonExistent. > + > + If the memory space descriptor already has type > + EfiGcdMemoryTypeMemoryMappedIo, and its capabilities are a superset of the > + required capabilities, then no action is taken -- it is by definition > + compatible with the aperture. > + > + Otherwise, the intersection of the memory space descriptor is calculated with > + the aperture. If the intersection is the empty set (no overlap), no action is > + taken; the memory space descriptor is compatible with the aperture. > + > + Otherwise, the type of the descriptor is investigated again. If the type is > + EfiGcdMemoryTypeNonExistent (representing a gap, or a genuine descriptor with > + such a type), then an attempt is made to add the intersection as MMIO space > + to the GCD memory space map, with the specified capabilities. This ensures > + continuity for the aperture, and the descriptor is deemed compatible with the > + aperture. > + > + Otherwise, the memory space descriptor is incompatible with the 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 the > + aperture for. > + > + @retval EFI_SUCCESS The descriptor is compatible. The GCD memory > + 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 == EfiGcdMemoryTypeMemoryMappedIo && > + (Descriptor->Capabilities & Capabilities) == Capabilities) { > + return EFI_SUCCESS; > + } > + > + IntersectionBase = MAX (Base, Descriptor->BaseAddress); > + IntersectionEnd = MIN (Base + Length, > + Descriptor->BaseAddress + Descriptor->Length); > + if (IntersectionBase >= IntersectionEnd) { > + // > + // The descriptor and the aperture don't overlap. > + // > + return EFI_SUCCESS; > + } > + > + if (Descriptor->GcdMemoryType == EfiGcdMemoryTypeNonExistent) { > + Status = 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 conflicts " > + "with aperture [%Lx, %Lx) cap %Lx\n", gEfiCallerBaseName, __FUNCTION__, > + Descriptor->BaseAddress, Descriptor->BaseAddress + Descriptor->Length, > + (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 = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemorySpaceMap); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a: %a: GetMemorySpaceMap(): %r\n", > + gEfiCallerBaseName, __FUNCTION__, Status)); > + return Status; > + } > + > + for (Index = 0; Index < NumberOfDescriptors; Index++) { > + Status = IntersectMemoryDescriptor (Base, Length, Capabilities, > + &MemorySpaceMap[Index]); > + if (EFI_ERROR (Status)) { > + goto FreeMemorySpaceMap; > + } > + } > + > + DEBUG_CODE ( > + // > + // Make sure there are adjacent descriptors covering [Base, Base + Length). > + // It is possible that they have not been merged; merging can be prevented > + // by allocation and different capabilities. > + // > + UINT64 CheckBase; > + EFI_STATUS CheckStatus; > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR Descriptor; > + > + for (CheckBase = Base; > + CheckBase < Base + Length; > + CheckBase = Descriptor.BaseAddress + Descriptor.Length) { > + CheckStatus = gDS->GetMemorySpaceDescriptor (CheckBase, &Descriptor); > + ASSERT_EFI_ERROR (CheckStatus); > + ASSERT (Descriptor.GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo); > + ASSERT ((Descriptor.Capabilities & Capabilities) == Capabilities); > + } > + ); > + > +FreeMemorySpaceMap: > + FreePool (MemorySpaceMap); > + > + return Status; > +} > > /** > Initialize the state information for the CPU Architectural Protocol. >