From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.103328.1679669801995343825 for ; Fri, 24 Mar 2023 07:56:42 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=dagSPO48; spf=pass (domain: redhat.com, ip: 170.10.129.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1679669800; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=11QcIAvl3qCt51rEGNy84SOm2SvcQCRl/5BqXkJJq5w=; b=dagSPO48H7vDZyREesq8Z8booWGGaAH0orZxxLJ8k2dxuUpWM885NV26lLXRN4qQw8pAnC rOloty5+8qd1jqJiYGtpUjd5nTu7eX2FCp6X/e9aDHv+/RsEJ8Ztq73uMrfgWjm57dMG9/ /WQLprZniYq57J658gWbiFqb4fw7vIk= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-641-8mWiAww_NuWq5y44xfcdAA-1; Fri, 24 Mar 2023 10:56:37 -0400 X-MC-Unique: 8mWiAww_NuWq5y44xfcdAA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E08CB3815EFD; Fri, 24 Mar 2023 14:56:36 +0000 (UTC) Received: from [10.39.194.72] (unknown [10.39.194.72]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A739B40C6E67; Fri, 24 Mar 2023 14:56:35 +0000 (UTC) Message-ID: Date: Fri, 24 Mar 2023 15:56:34 +0100 MIME-Version: 1.0 Subject: Re: [PATCH 1/1] UefiCpuPkg/PiSmmCpuDxeSmm: fix format string To: Gerd Hoffmann , devel@edk2.groups.io Cc: Eric Dong , Oliver Steffen , Rahul Kumar , Ray Ni , Pawel Polawski References: <20230324134157.118321-1-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20230324134157.118321-1-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi Gerd, On 3/24/23 14:41, Gerd Hoffmann wrote: > BufferPages is UINTN, so we need "%Lu" when printing it. > > Fixes: 4f441d024bee ("UefiCpuPkg/PiSmmCpuDxeSmm: fix error handling") > Reported-by: Laszlo Ersek > Signed-off-by: Gerd Hoffmann > --- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > index cf69161caa4b..08663b1b1ab4 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > @@ -880,7 +880,7 @@ PiCpuSmmEntry ( > BufferPages = EFI_SIZE_TO_PAGES (SIZE_32KB + TileSize * (mMaxNumberOfCpus - 1)); > Buffer = AllocateAlignedCodePages (BufferPages, SIZE_4KB); > if (Buffer == NULL) { > - DEBUG ((DEBUG_ERROR, "Failed to allocate %d pages.\n", BufferPages)); > + DEBUG ((DEBUG_ERROR, "Failed to allocate %Lu pages.\n", BufferPages)); > CpuDeadLoop (); > return EFI_OUT_OF_RESOURCES; > } you missed part of my off-list comment; it was: > "BufferPages" has type UINTN, it should not be printed with "%d" (%d > is for INT32). The proper way to print a UINTN value with DEBUG is to > cast the value to UINT64, and print that with %Lu (or %Lx). Refer to "cast the value to UINT64". UINTN is either UINT32 or UINT64. There is no format specifier that's dedicated specifically to UINTN. You can't use fixed %u with UINTN because that will be wrong on 64-bit platforms (%u takes UINT32 but UINTN will be UINT64), and you can't use fixed %Lu with UINTN because that will be wrong on 32-bit platforms (%Lu takes UINT64 but UINTN will be UINT32). The proper way is to (1) cast the UINTN to UINT64 first (which will be a no-op on 64-bit platforms, and it will widen UINT32 to UINT64 on 32-bit platforms, without changing the value), and then (2) print *that* (the now certainly UINT64 value) with %Lu. DEBUG (( DEBUG_ERROR, "Failed to allocate %Lu pages.\n", (UINT64)BufferPages )); ( Here's an idea: > diff --git a/MdePkg/Include/Library/PrintLib.h b/MdePkg/Include/Library/PrintLib.h > index 8d523cac528d..bdacec9777f0 100644 > --- a/MdePkg/Include/Library/PrintLib.h > +++ b/MdePkg/Include/Library/PrintLib.h > @@ -197,6 +197,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #define PREFIX_ZERO 0x20 > #define RADIX_HEX 0x80 > > +#if defined (MDE_CPU_AARCH64) || defined (MDE_CPU_LOONGARCH64) || \ > + defined (MDE_CPU_RISCV64) || defined (MDE_CPU_X64) > +#define PRIuN "%Lu" > +#define PRIxN "%Lx" > +#elif defined (MDE_CPU_ARM) || defined (MDE_CPU_IA32) > +#define PRIuN "%Lu" > +#define PRIxN "%Lx" > +#else > +#error Unknown CPU architecture. > +#endif > + > /** > Produces a Null-terminated Unicode string in an output buffer based on > a Null-terminated Unicode format string and a VA_LIST argument list. and then you could do DEBUG (( DEBUG_ERROR, "Failed to allocate " PRIuN " pages.\n", BufferPages )); (Note the lack of a cast.) Note: I do not cover MDE_CPU_EBC above, in "PrintLib.h". I don't know how EBC (EFI Byte Code) deals with 32-bit vs. 64-bit. The UEFI 2.10 spec says in "22.1.1 Processor Architecture Independence": "64-bit C source code" and "The interpreter handles 32 vs. 64 bit issues". Unfortunately, that doesn't seem to help: it does not help us decide the size of UINTN at compile time. More precisely: if you check "MdePkg/Include/Ebc/ProcessorBind.h", it contains: > /// > /// Signed value of native width. (4 bytes on supported 32-bit processor instructions, > /// 8 bytes on supported 64-bit processor instructions) > /// "long" type scales to the processor native size with EBC compiler > /// > typedef long INTN; > /// > /// The unsigned value of native width. (4 bytes on supported 32-bit processor instructions; > /// 8 bytes on supported 64-bit processor instructions) > /// "long" type scales to the processor native size with the EBC compiler. > /// > typedef unsigned long UINTN; In other words, the size of UINTN *is* chosen at compile time, but the choice cannot be determined from MDE_CPU_EBC. And no other macro that "MdePkg/Include/Ebc/ProcessorBind.h" defines seems to be suitable for investigation with #if preprocessing directives. Anyway: the explicit (UINT64) cast with the open-coded %Lu conversion specifier will work fine on EBC as well, so I suggest sticking with that. ) Laszlo