From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id F0AFDC7618A for ; Mon, 20 Mar 2023 11:04:52 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 5EF4B84CFA; Mon, 20 Mar 2023 12:04:49 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="FYtOJTo9"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 90F8385B0A; Mon, 20 Mar 2023 12:04:47 +0100 (CET) Received: from mail-ed1-x536.google.com (mail-ed1-x536.google.com [IPv6:2a00:1450:4864:20::536]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id A03B084CFA for ; Mon, 20 Mar 2023 12:04:44 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-ed1-x536.google.com with SMTP id o12so45022960edb.9 for ; Mon, 20 Mar 2023 04:04:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1679310284; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=cbQBz2NRyFUMiCvUpPyg9hpaCzcERaAkrVjKO4gwk64=; b=FYtOJTo9KCVs8yijj+Io5+m7DkcqRnQzTdKoVBuhni9mCTgAM+0RgvIxjzJkwEWrim tD9lZ24KZbYmNGanwtASW6TfsQDA8Fc+YjGxt0yQ6wlRsdSS/opJHvUzG6WB/wiiygyj gwYzEePzttOCn7Ejt7GllwLXwArIIAxwuFtYtk47dFqf3+o/shYaF5SHilaANgZUrSsW sZJ1ZevyDMFpfJoDcCH6+UavrRuahmWEkp7aI+pQdAnZcSqvQHnf644rZnKdXF1Fz8Um GP7I7CBU2BapWx5g1Rxn4+0d3RRmV4+QgX7v/rOSaUu9z7kpWA99/kiW+Z1GBsOOHQe4 oc7Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679310284; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=cbQBz2NRyFUMiCvUpPyg9hpaCzcERaAkrVjKO4gwk64=; b=b7GJbphNMjf+2y+BInfifgdiWZk0+JVtQ/YPLsK59zo18zNEfJpxmISTMdOqu4LPtr 5VoyvCjfayNDgbhpj7opPccqKU3I11djQ+GGsEumclDEv17sqSJXGeb65Vqeys4HstZI PU2wot9HWKIhoGcnYLLCtoNu6OJRGUQvmTcchnjO6seLxY9yan8ivq239fUmSMv3YJDe RvkCrzHzq41g4F9DZ7vYuZ97ocHrDKgqWh7BpihQIapW/S3CWcdaa4KzEzoYBcPcG0Oy /MdEHN7+6OJGN3jpcL1gYAi5RXZGjaTBQYqCcwwbL3IHEaynGXj8aUc+odkm1zD+IVPA dHZg== X-Gm-Message-State: AO0yUKVVu9ALUxc2vAPJJVlbusznvcs4CaTCqY/SxadL7JG1HW8fs5mg kuRzdQEkHj6F6kQv+LLJSiP9Ug== X-Google-Smtp-Source: AK7set8NrgT8SWscF2sEMl+xP5BW6y6nyZY5cISArLbjpTWaQonik//AKwC/YUJOZSfsd/wDMDqcNw== X-Received: by 2002:a17:906:f0d3:b0:930:a7c9:e45 with SMTP id dk19-20020a170906f0d300b00930a7c90e45mr8701916ejb.8.1679310284132; Mon, 20 Mar 2023 04:04:44 -0700 (PDT) Received: from hera (ppp176092130041.access.hol.gr. [176.92.130.41]) by smtp.gmail.com with ESMTPSA id hy14-20020a1709068a6e00b009351546fb54sm665591ejc.28.2023.03.20.04.04.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Mar 2023 04:04:43 -0700 (PDT) Date: Mon, 20 Mar 2023 13:04:41 +0200 From: Ilias Apalodimas To: Heinrich Schuchardt Cc: u-boot@lists.denx.de Subject: Re: [PATCH 1/2] efi_loader: move dp_alloc() to efi_alloc() Message-ID: References: <20230319082023.97558-1-heinrich.schuchardt@canonical.com> <20230319082023.97558-2-heinrich.schuchardt@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean On Mon, Mar 20, 2023 at 10:15:21AM +0100, Heinrich Schuchardt wrote: > On 3/20/23 08:38, Ilias Apalodimas wrote: > > Hi Heinrich, > > > > On Sun, Mar 19, 2023 at 09:20:22AM +0100, Heinrich Schuchardt wrote: > > > The incumbent function efi_alloc() is unused. > > > > > > Replace dp_alloc() by a new function efi_alloc() that we can use more > > > widely. > > > > [...] > > > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -533,27 +536,6 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, > > > return EFI_SUCCESS; > > > } > > > > > > -/** > > > - * efi_alloc() - allocate memory pages > > > - * > > > - * @len: size of the memory to be allocated > > > - * @memory_type: usage type of the allocated memory > > > - * Return: pointer to the allocated memory area or NULL > > > - */ > > > -void *efi_alloc(uint64_t len, int memory_type) > > > -{ > > > - uint64_t ret = 0; > > > - uint64_t pages = efi_size_in_pages(len); > > > - efi_status_t r; > > > - > > > - r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type, pages, > > > - &ret); > > > - if (r == EFI_SUCCESS) > > > - return (void*)(uintptr_t)ret; > > > - > > > - return NULL; > > > -} > > > - > > > /** > > > * efi_free_pages() - free memory pages > > > * > > > @@ -672,6 +654,28 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, > > > return r; > > > } > > > > > > +/** > > > + * efi_alloc() - allocate boot services data pool memory > > > + * > > > + * Allocate memory from pool and zero it out. > > > + * > > > + * @size: number of bytes to allocate > > > + * Return: pointer to allocated memory or NULL > > > + */ > > > +void *efi_alloc(size_t size) > > > > All our allocation related functions require the memory type to be passed. > > If we want to default this to 'EFI_BOOT_SERVICES_DATA' I think we need to > > change the name a bit to indicate that. > > We have 13 instances of allocating EFI_BOOT_SERVICES_DATA from the pool and > only 2 instances (see below) of other memory types. So this is the only case > worth factoring out. I would prefer to keep identifiers short. Ok, then at least please add the EFI_BOOT_SERVICES_DATA part in the function documentation, or rename it to efi_alloc_bsd() > > > > > > +{ > > > + void *buf; > > > + > > > + if (efi_allocate_pool(EFI_BOOT_SERVICES_DATA, size, &buf) != > > > > Is there a reason we are using efi_allocate_pool instead of > > efi_allocate_pages? > > The UEFI specification explicitly requires pool memory in most places where > the caller has to free memory. Freeing pages requires to know the size of > the memory area while freeing from pool does not. Furthermore when freeing > from pool we can check if the memory was ever allocated. > > In future we may want to make small allocations from pool more efficient by > not consuming a minimum of one page. > > We could rethink why we are allocating configuration tables from pool in the > following instances: > > lib/efi_loader/efi_tcg2.c:1703 > create_final_event() > > lib/efi_loader/efi_runtime.c:116 > efi_init_runtime_supported() I think they can both change to efi_alloc() now. As far as the TCG is concerned there no particular reason of using pool allocations. In any case you can add my Reviewed-by: Ilias Apalodimas Regards /Ilias > > Best regards > > Heinrich > > > > > > + EFI_SUCCESS) { > > > + log_err("out of memory"); > > > + return NULL; > > > + } > > > + memset(buf, 0, size); > > > + > > > + return buf; > > > +} > > > + > > > /** > > > * efi_free_pool() - free memory from pool > > > * > > > -- > > > 2.39.2 > > > > > > > Thanks > > /Ilias >