From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f44.google.com (mail-ej1-f44.google.com [209.85.218.44]) by mx.groups.io with SMTP id smtpd.web10.6688.1680871576771189053 for ; Fri, 07 Apr 2023 05:46:17 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=n2ohWnHd; spf=pass (domain: linaro.org, ip: 209.85.218.44, mailfrom: marcin.juszkiewicz@linaro.org) Received: by mail-ej1-f44.google.com with SMTP id l17so8166175ejp.8 for ; Fri, 07 Apr 2023 05:46:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1680871575; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=fe3NeQjyxXIzl1fOZHFKAc+82O8pydtY9/koO9LP9aQ=; b=n2ohWnHdysGlpwK5s7dnEhE/47LtSXYCvg9GI/Lgre062T2lJOXN04yDibGEcwi5rP SfSr7xIBI48vH+/sHCF0FmwBNbg/zv0+kHf6EFLI7ZeTET2L6cv6UvmzK6DGjCUxdIVm DLF/bOx4EF42QC0e6qcMjg3ZYCpwrlMzLXPI7j+hbtsH1nF9UnZC61YdhEQLZ9NOC2B8 TWl0bfdaYFicrM1HRPjoiBzH7pQoMseLCZh3SIwOoDM+hxLGDBo9ehtRr2Ktuq7OU2yn LyN7e5nFhF1HrtAeOAqeZdIy1ViORNVT6PhpfcMZ8bTu93HtPzRN9yVM3F1cu33rxCxZ KXWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680871575; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=fe3NeQjyxXIzl1fOZHFKAc+82O8pydtY9/koO9LP9aQ=; b=J1dvatFxFkj8Auueik0P49gCTlOusji0rcejux8ti/UexfObVyMa7j7v2cxolno5gx jORGKI9l21Ze80b7YRRDRsLlOcWE0uFPgDbQktQ5HUkumMXEm3KTYhpjJhGCw7FRBnSg FxsMZ997iTXwRFXkcMbXX6otQdRxAboc8j0Ttf21hmtrLoPEjsj3BxsV5hkxrUwKj6Uj ST4s7d2F6XrfwJJP4R1gnbSFlNkVZmLDMZd/upJFT9SfH4QuBz7Ye7XnrwGum2UNVmUZ 4yvQzhDWTsRDyZ9aGaOp2KMSx7ns2mQZg9Sc+ljaABKC2FHF2JEjHGCNZWEvEIdoKLmY gDMQ== X-Gm-Message-State: AAQBX9cJ4Z9xxFPVdW9Eu8A5x5XfLgh/aWiZN0NM5arw9D0WlFGhJKjP UOdXT6uBEHicgyfA3Fm7zu5IMQ== X-Google-Smtp-Source: AKy350YTiNIKHL8edGvBqjkJvci7LD+/OeItGu6Fhm5sJRpEku9Vt2nUa/y1Ujzd/+HY/wATZNjMyQ== X-Received: by 2002:a17:906:5295:b0:8b1:781d:f9a4 with SMTP id c21-20020a170906529500b008b1781df9a4mr2203811ejm.21.1680871575031; Fri, 07 Apr 2023 05:46:15 -0700 (PDT) Return-Path: Received: from [192.168.200.206] (83.11.21.111.ipv4.supernova.orange.pl. [83.11.21.111]) by smtp.gmail.com with ESMTPSA id qp4-20020a170907206400b0093048a8bd31sm2034349ejb.68.2023.04.07.05.46.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 07 Apr 2023 05:46:14 -0700 (PDT) Message-ID: Date: Fri, 7 Apr 2023 14:46:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Subject: Re: [edk2-devel] [PATCH v2] add ArmCpuInfo EFI application To: Ard Biesheuvel , devel@edk2.groups.io Cc: Ard Biesheuvel , Leif Lindholm , Rebecca Cran References: <77a6f44f-cd1f-71a7-0964-c8bc9fc103d5@bsdio.com> <20230407103934.86756-1-marcin.juszkiewicz@linaro.org> From: "Marcin Juszkiewicz" Organization: Linaro In-Reply-To: Content-Language: pl-PL, en-GB, en-HK Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit W dniu 7.04.2023 o 12:55, Ard Biesheuvel pisze: > Hello Marcin, > > Thanks for this - it looks useful. > Some comments below. Thanks. > On Fri, 7 Apr 2023 at 12:40, Marcin Juszkiewicz > wrote: >> >> App goes through ID_AA64*_EL1 system registers and decode their values. >> >> First version which does not use much of current AArch64 support code >> present in EDK2. Written to check what data is there and what can be >> done with it. >> >> Signed-off-by: Marcin Juszkiewicz >> --- >> MdeModulePkg/MdeModulePkg.dsc | 3 +- >> MdeModulePkg/Application/ArmCpuInfo/ArmCpuInfo.inf | 38 + >> MdeModulePkg/Application/ArmCpuInfo/readargs.h | 12 + >> MdeModulePkg/Application/ArmCpuInfo/ArmCpuInfo.c | 2274 ++++++++++++++++++++ >> MdeModulePkg/Application/ArmCpuInfo/readregs.s | 49 + >> 5 files changed, 2375 insertions(+), 1 deletion(-) >> > > Why are you adding this to MdeModulePkg rather than ArmPkg? This is the first time I wrote something for EDK2 so worked to get it built first rather than to get it right. Looked at HelloWorld example and followed. Moved to ArmPkg/Application/ArmCpuInfo/ in current code. >> diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc >> index 1014598f31c3..1ecfb345159f 100644 >> --- a/MdeModulePkg/MdeModulePkg.dsc >> +++ b/MdeModulePkg/MdeModulePkg.dsc >> @@ -216,6 +216,7 @@ [PcdsDynamicExDefault] >> gEfiMdeModulePkgTokenSpaceGuid.PcdRecoveryFileName|L"FVMAIN.FV" >> >> [Components] >> + MdeModulePkg/Application/ArmCpuInfo/ArmCpuInfo.inf >> MdeModulePkg/Application/HelloWorld/HelloWorld.inf >> MdeModulePkg/Application/DumpDynPcd/DumpDynPcd.inf >> MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.inf >> @@ -520,4 +521,4 @@ [Components.X64] >> MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf >> >> [BuildOptions] >> - >> +GCC:*_*_AARCH64_CC_FLAGS = -march=armv9-a > > This belongs in the app's INF, not in the package DSC where it affects > code generation for all components, which may need to run on much > older CPUs Dropped that line as it was not needed in the end. >> diff --git a/MdeModulePkg/Application/ArmCpuInfo/ArmCpuInfo.c b/MdeModulePkg/Application/ArmCpuInfo/ArmCpuInfo.c >> new file mode 100644 >> index 000000000000..ac27902e3533 >> +void print_values(const char* field, const char* bits, const int value, const char* description) >> +{ >> + char binaries[17][5] = {"0000", "0001", "0010", "0011", "0100", "0101", "0110", "0111", >> + "1000", "1001", "1010", "1011", "1100", "1101", "1110", "1111"}; >> + > > STATIC CONST CHAR8[] please, and you can use [] for the first > dimension (and drop the final element) done >> + AsciiPrint(" %-16a | %5a | %5a | %a\n", field, bits, binaries[value], description); > > This should be value & 0xf so you never access outside of the array. done >> +void handle_aa64mmfr0_el1(const UINT64 aa64mmfr0_el1) >> +{ >> + UINT64 value; >> + char* regname = "ID_AA64MMFR0_EL1"; > > STATIC CONST CHAR8 RegName[] = ... done >> + char* description; >> + char* bits; > > CONST It is not const. >> + >> + >> + bits = "3:0 "; >> + value = aa64mmfr0_el1 & 0xf; >> + switch(value) > > Space after switch done >> + if(value == 0b0110) > > Space after if done > etc etc > > There are some other style issues here, which I am sure the CI will > whine about, so you might want to run uncrustify on it. done