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.133.124]) by mx.groups.io with SMTP id smtpd.web10.104011.1679669905913450077 for ; Fri, 24 Mar 2023 07:58:26 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=XC8SkVXc; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1679669905; 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=bbKvtkNKqqq5aX+Nir9wQ/9+TWrzd8DnVnVYldlMKlw=; b=XC8SkVXc9leDLflZkRcwZ20CqYC8j8vAB8Z8D2+hsgSs8ekUvyNdM+JcUr6pel8NioB+Hc E0pHgcnuNjRvitD9KdG8e/vGLUSykAmP1v4wCWTVra9hNZFgb+M2wurCoiXWI6e8ybLwTn l9mf1Qg4UoFMza26u8ORNkFgwzlRyYc= 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-290-fKaqVDK6Mg-w-1cn3qZawg-1; Fri, 24 Mar 2023 10:58:22 -0400 X-MC-Unique: fKaqVDK6Mg-w-1cn3qZawg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 683233C10160; Fri, 24 Mar 2023 14:58:22 +0000 (UTC) Received: from [10.39.194.72] (unknown [10.39.194.72]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3860D2A68; Fri, 24 Mar 2023 14:58:21 +0000 (UTC) Message-ID: Date: Fri, 24 Mar 2023 15:58:20 +0100 MIME-Version: 1.0 Subject: Re: [PATCH 1/1] UefiCpuPkg/PiSmmCpuDxeSmm: fix format string From: "Laszlo Ersek" 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> In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 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 On 3/24/23 15:56, Laszlo Ersek wrote: > 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" aargh, how annoying. I'm in a rush, sorry. I over-edited this code and forgot to remove the "L" from the latter two. Laszlo >> +#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