From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lj1-f178.google.com (mail-lj1-f178.google.com [209.85.208.178]) by mx.groups.io with SMTP id smtpd.web11.9939.1682001738973290395 for ; Thu, 20 Apr 2023 07:42:19 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@linaro.org header.s=google header.b=MK1E1oud; spf=pass (domain: linaro.org, ip: 209.85.208.178, mailfrom: marcin.juszkiewicz@linaro.org) Received: by mail-lj1-f178.google.com with SMTP id 38308e7fff4ca-2a8c30ac7e3so5359241fa.1 for ; Thu, 20 Apr 2023 07:42:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1682001737; x=1684593737; h=content-transfer-encoding:in-reply-to:organization:from :content-language:references:to:subject:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=L4mIbKNoEfRLYNLvdeMBFxj4kgBFfiFU6JgmyUze9TM=; b=MK1E1oudtUOwWdYxu77gUmBFKwfPjLmn0OTt+jRyTRVK1V6Yug5KoRt7JmihbxXaEP Hu7+iI/Q86Nxqc1mTqv4x+rXRDDagexWQSn3A2WuFGJbnzDCwH2JM/p0elrlDhAmYhsH VL6lY814w1qnbmLWK+8/Y7vAdiBr1BUr15iywFzDSavL2FShKy13mfs9YM2DCcS5kbez LrwSGRJ4FlMnD+Q9RMkKz1iNGfEnwmAQUmNNK4a3qzZS+MJO9RnV1xa5WhFC1SLjPy9y Cw/VM9wNpqBQlFelllIATzRKitC+KFCLXD0MfTyK6qoHwKzUjKwmAjxnFwkdqnZBibcS XNYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682001737; x=1684593737; h=content-transfer-encoding:in-reply-to:organization:from :content-language:references:to:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=L4mIbKNoEfRLYNLvdeMBFxj4kgBFfiFU6JgmyUze9TM=; b=EKlGJsqZmlVJx85uTV24ZlA4rrfH28CYQqjVOcPNY/paWbP9D+SZJn3T+ZRA+IznQm Bp6Usb+177IcqByMIXYvuf7fFQpWm/2kvmndVnXEZgs2J93iGWUjV5I+imbNsZNbPGQp 9tFu49+Ap4+ewZqw+47vxVrxZivkFDy2N9InSq//8nyMmqFrc0NIjh3HxvWVQv+hL6im L/7348+j7GEYeDiVGu7f7XcREakn0ile0/rf4tj5w7sUTUJ/2PRJxF2p+4Hdrgov8Zi1 oOM9PmBqEhdz4az/RbcI/bj6UWeWlpC3QX/yGRjzl1eLaMx4/+OhowNpLI9X61X7bNTA sRIQ== X-Gm-Message-State: AAQBX9ddW6x8pnCFArUmwNMW7POAoixymjOdX9h/Zy3GTcIxaU3WZucP wN0+PZb0wWABoThU3rx9q0C1DWtERM4UQQ2mVtVidA== X-Google-Smtp-Source: AKy350bUKCyJ7IRuhAMExgnaiuJVX30hmMXYVoOhz9Cby/UE63BTCoJ1PaDV2BJMja9ZoQfYrYxdCA== X-Received: by 2002:ac2:5109:0:b0:4d8:5096:1c95 with SMTP id q9-20020ac25109000000b004d850961c95mr503452lfb.43.1682001736826; Thu, 20 Apr 2023 07:42:16 -0700 (PDT) Return-Path: Received: from [192.168.200.206] (83.11.213.14.ipv4.supernova.orange.pl. [83.11.213.14]) by smtp.gmail.com with ESMTPSA id b10-20020ac25e8a000000b004edcc8d27b5sm236946lfq.290.2023.04.20.07.42.16 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 20 Apr 2023 07:42:16 -0700 (PDT) Message-ID: Date: Thu, 20 Apr 2023 16:42:15 +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 v5 2/2] add ArmCpuInfo EFI application To: devel@edk2.groups.io, quic_llindhol@quicinc.com References: <1753ABF1A296B040.11304@groups.io> <20230407152957.157494-1-marcin.juszkiewicz@linaro.org> <20230407152957.157494-3-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 20.04.2023 o 14:43, Leif Lindholm pisze: > On Fri, Apr 07, 2023 at 17:29:57 +0200, Marcin Juszkiewicz wrote: >> +# >> +# This flag specifies whether HII resource section is generated into PE image. >> +# >> + UEFI_HII_RESOURCE_SECTION = TRUE > > The above stanza, and its comment, can be dropped. > This relates to native language support, which there isn't any in this app. done >> +VOID >> +PrintText ( >> + CONST CHAR8 *field, > > For coding style, name should be "*Field". done, in all places >> + CONST CHAR8 *Bits, >> + CONST UINT8 Value, >> + CONST CHAR8 *Description >> + ) >> +{ >> + STATIC CONST CHAR8 binaries[][5] = { > > Could I propose renaming "binaries" to "Nibbles", since this is an > array holding string values for all possible nibbles? done >> + "0000", "0001", "0010", "0011", "0100", "0101", "0110", "0111", >> + "1000", "1001", "1010", "1011", "1100", "1101", "1110", "1111" >> + }; >> + switch (Value) { >> + case 0b0000: > > I agree with Pedro's concern w.r.t. binary literals being a GCC > extension. But I also think this is the most appropriate > representation since this is how it's documented in the ARM ARM. > > A bit hacky, but could we do > > enum { > b0000, > b0001, > ... > b1111 > }; > > and then use those instead? > > (you can build with -pedantic to verify you catch them all) done, passed with -pedantic >> + // only valid for BigEnd != 0b0000 > > Could we possibly reword as > // If mixed-endian support is present, check whether supported at EL0 done >> + case 0b0001: // add FEAT_LPA2 check > > That's sounds like a TODO. And we don't want TODOs in code. > Can we either drop the comment, add the check, or skip the test? > (That is my order of preference - we shouldn't need to be verifying > architectural compliance.) dropped >> + case 0b0010: // add FEAT_LPA2 check > > Same comment as for 4k. dropped as well >> + Bits = "59:56"; >> + Value = (Aa64Pfr0 >> 56) & 0xf; >> + switch (Value) { >> + case 0b0000: >> + Description = "no info is FEAT_CSV2 implemented."; > > Consider rewording. > Is the text from the ARM ARM too long? > "The implementation does not disclose whether FEAT_CSV2 is > implemented." > if so, maybe > "Not disclosed whether FEAT_CSV2 is implemented."? thx, done >> + PrintValues (RegName, Bits, Value, Description); >> + >> + // 35:32 is CSV2_frac > > That's a TODO. Is it more info why those bits are not handled here. Same with 15:12 for RAS_frac and 19:16 for MPAM_frac. They are not reserved like 63:40 are. And CSV2_frac bits are used earlier, in PRF0 handling. If we have CSV2 implemented then CSV2_frac says do we have CSV2_1p1 or CSV2_1p2 implemented. MPAM_frac and RAS_frac are used in PRF0 handling as well. >> +EFI_STATUS >> +EFIAPI >> +UefiMain ( > > Stray space before '(' here too. Is it an editor setting? fixed