From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::242; helo=mail-io0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x242.google.com (mail-io0-x242.google.com [IPv6:2607:f8b0:4001:c06::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 34BA12035BB04 for ; Tue, 21 Nov 2017 08:28:08 -0800 (PST) Received: by mail-io0-x242.google.com with SMTP id h205so19318946iof.5 for ; Tue, 21 Nov 2017 08:32:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=N70vQKCQXg44xGS9TtfT7USyWuPcnj+lAe5jqCNkY/8=; b=EDnVBIrq4kLtFnv0PSqNGsMXMrIdA09M2InXYLjrU5JZ+Y7DJLWQJ1jT82clV1TnMr DTrFcZoq7Uq0A42x6wazf2ZUi52RsbDD3dBMm3Ndjr5RUXamYTjRSkiSvrKeQW1Gm3p9 xU0ExnyD3aWT/6Xmfo2xhT1g5LMU9YUXKw8UI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=N70vQKCQXg44xGS9TtfT7USyWuPcnj+lAe5jqCNkY/8=; b=VWD511PI/q1SmP4opvcvr7ZPoWiflmWbDAjG5+kcoitRCMjtg7946McHH53qIouVKw rBtJdvSJP79/KSoJcR3jA4E4/3SjVGIydK6Bf7uPHd3LmeLelcM1+CgUGluqNyVjeEaY 4O+s5SmgDmh+uXCinhC5z2ijIQ70wE7j5i71aiOFO1HH8xfScBe9sAkwoiQS3YeNVd5r 5wG2ysjwNNiZZAL56YGBQeKjPsA2hzV72UBKKOP+9rYUT9HUWF20DyrAhxzaU0ECB+M1 jPI6dFHW9Coe55IHIgn31xGLlJOPnVFwK6tPA0U1S7WREXX//o2Z0VOYuQmX1euOk/9c GPCQ== X-Gm-Message-State: AJaThX5Ahf+UicPZMhUjj87ZaDwjvxw03MPfFdbopMdO1rU8RoZ06c41 OnfWGaNuLdHH/rq0Y2gt9m49T6EzQEduelH8MSuONw== X-Google-Smtp-Source: AGs4zMYyZkhxzRhHrwKiN8AmhykNnsJUqBlj2qs5PcrjVjpvN+s6xTUEZSYkvvueOsCBQup9Mf//ieSKIz1VUK2OJl0= X-Received: by 10.107.174.222 with SMTP id n91mr16650977ioo.43.1511281943390; Tue, 21 Nov 2017 08:32:23 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Tue, 21 Nov 2017 08:32:22 -0800 (PST) In-Reply-To: <428456f8-237a-89aa-2531-18ce18bb4c97@redhat.com> References: <20171117160913.17292-1-ard.biesheuvel@linaro.org> <20171117160913.17292-11-ard.biesheuvel@linaro.org> <428456f8-237a-89aa-2531-18ce18bb4c97@redhat.com> From: Ard Biesheuvel Date: Tue, 21 Nov 2017 16:32:22 +0000 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm Subject: Re: [PATCH 10/15] ArmVirtPkg: introduce ArmVirtMemInfoLib library class 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: Tue, 21 Nov 2017 16:28:09 -0000 Content-Type: text/plain; charset="UTF-8" On 21 November 2017 at 16:27, Laszlo Ersek wrote: > On 11/21/17 17:23, Laszlo Ersek wrote: >> On 11/17/17 17:09, Ard Biesheuvel wrote: >>> As part of the effort to get rid of ArmPlatformLib (which incorporates >>> far too many duties in a single library), introduce ArmVirtMemInfoLib >>> which will be invoked by our ArmVirtMemoryInitPeiLib implementation to >>> get a description of the virtual address space. This will allow us to >>> remove this functionality from ArmPlatformLib later, or, in the case of >>> ArmVirtXen and ArmVirtQemuKernel, drop ArmPlatformLib altogether. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Ard Biesheuvel >>> --- >>> ArmVirtPkg/ArmVirtPkg.dec | 3 ++ >>> ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h | 39 ++++++++++++++++++++ >>> 2 files changed, 42 insertions(+) >>> >>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec >>> index a8603e1b80e5..8f656fd2739d 100644 >>> --- a/ArmVirtPkg/ArmVirtPkg.dec >>> +++ b/ArmVirtPkg/ArmVirtPkg.dec >>> @@ -30,6 +30,9 @@ [Defines] >>> [Includes.common] >>> Include # Root include for the package >>> >>> +[LibraryClasses] >>> + ArmVirtMemInfoLib|Include/Library/ArmVirtMemInfoLib.h >>> + >>> [Guids.common] >>> gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E, 0x2E, 0x36, 0x5B, 0x80, 0x63, 0x66 } } >>> gEarlyPL011BaseAddressGuid = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80, 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } } >>> diff --git a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h >>> new file mode 100644 >>> index 000000000000..65be2cbd8082 >>> --- /dev/null >>> +++ b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h >>> @@ -0,0 +1,39 @@ >>> +/** @file >>> + >>> + Copyright (c) 2011-2013, ARM Limited. All rights reserved. >>> + Copyright (c) 2017, Linaro, Ltd. All rights reserved. >>> + >>> + This program and the accompanying materials are licensed and made available >>> + under the terms and conditions of the BSD License which accompanies this >>> + distribution. The full text of the license may be found at >>> + http://opensource.org/licenses/bsd-license.php >>> + >>> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >>> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >>> + >>> +**/ >>> + >>> +#ifndef _ARM_VIRT_MEMINFO_LIB_H_ >>> +#define _ARM_VIRT_MEMINFO_LIB_H_ >>> + >>> +#include >>> +#include >>> + >>> +/** >>> + Return the Virtual Memory Map of your platform >>> + >>> + This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU >>> + on your platform. >>> + >>> + @param[out] VirtualMemoryMap Array of ARM_MEMORY_REGION_DESCRIPTOR >>> + describing a Physical-to-Virtual Memory >>> + mapping. This array must be ended by a >>> + zero-filled entry >>> + >>> +**/ >>> +VOID >>> +ArmVirtGetMemoryMap ( >>> + OUT ARM_MEMORY_REGION_DESCRIPTOR **VirtualMemoryMap >>> + ); >>> + >>> +#endif >>> >> >> (1) Since this is a library API, please add EFIAPI to the declaration. >> >> (This will affect the instance(s) too.) >> >> >> (2) If it's not overly restrictive, then please mention in the >> "VirtualMemoryMap" param comment that the map is supposed to be >> allocated dynamically within the function, using the phase-matching >> MemoryAllocationLib instance. >> >> (Judged from the AllocatePages() call in >> "ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/QemuVirtMem.c".) > > Looking at the patch right after this one, dynamic memory allocation > appears wrong to spell out in the library interface. > > Then I guess the right thing to say would be, "the returned array is > never supposed to be freed; it is released at the latest when the OS > takes control". > >> With those addressed, >> >> Reviewed-by: Laszlo Ersek > > My R-b stands, just please clarify the expected lifetime of the array > returned, one way or another. > Thanks. Simply not freeing it is the current practice everywhere, given that PrePi and PEI MemoryAllocationLib implementations don't do FreePages() in the first place. But I agree it should be mentioned explicitly.