From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web08.12205.1619011792164780256 for ; Wed, 21 Apr 2021 06:29:52 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id BD83511FB; Wed, 21 Apr 2021 06:29:50 -0700 (PDT) Received: from [192.168.1.169] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 4807C3F774; Wed, 21 Apr 2021 06:29:49 -0700 (PDT) Subject: Re: [edk2-devel] [edk2-platforms][PATCH V1 1/8] Platform/Sgi: Helper macros for PPTT Table To: Pranav Madhu , "devel@edk2.groups.io" Cc: Ard Biesheuvel , Leif Lindholm , Sami Mujawar , nd References: <20210402091208.16752-1-pranav.madhu@arm.com> <20210402091208.16752-2-pranav.madhu@arm.com> <04572474-09c9-056f-397b-ef38ca06175c@arm.com> From: "PierreGondois" Message-ID: Date: Wed, 21 Apr 2021 14:29:45 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Hi Pranav, @@ -20,6 +20,132 @@ >>> =C3=AF=C2=BF=C2=BD#define EFI_ACPI_ARM_CREATOR_ID SIGNATURE_32('A','R= ','M',' ') >>> =C3=AF=C2=BF=C2=BD#define EFI_ACPI_ARM_CREATOR_REVISION 0x00000099 >>> >>> +#define CORE_COUNT=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2= =BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD FixedPcdGet32 (PcdCoreCount) >>> +#define CLUSTER_COUNT=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD FixedPcdGe= t32 (PcdClusterCount) >>> + >>> +#pragma pack(1) >>> +// PPTT processor core structure >>> +typedef struct { >>> +=C3=AF=C2=BF=C2=BD EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR=C3=AF=C2=BF= =C2=BD Core; =C3=AF=C2=BF=C2=BD >>> >> +UINT32=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2= =BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF= =C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2= =BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD= =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3= =AF=C2=BF >> =C2=BD >>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2= =BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD= =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Offset[2]; >> I think there should be 3 entries (DCache, ICache, L2Cache). Updating = this will >> require updating the other PPTT tables written. > As per ACPI spec 6.4, chapter '5.2.29.2 Cache Type Structure - Type 1',= " Only > the head of the list needs to be listed as a resource by a processor no= de (and > counted toward Number of Private Resources), as the cache node itself > contains a link to the next level of cache." > Here L2 cache is represented as next level of L1, so no need to count i= t. Yes indeed you are right. >> Would it be also possible to rename the field 'PrivateResources' as in= the >> spec ? > Yes, but in actual, it is not the private resource count. This was nit picking, 'Offset' also works for me, and other PPTT tables=20 are calling this field as 'Offset'. > >> Another question: what does 'RD_' stands for ? > RD Stands for Reference Design, it is the convention we follow. > >>> +=C3=AF=C2=BF=C2=BD EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE=C3=AF=C2=BF=C2=BD= =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD = DCache; =C3=AF=C2=BF=C2=BD >>> +EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD= =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD ICache; =C3=AF=C2=BF= =C2=BD >>> +EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD= =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD L2Cache; } >>> +RD_PPTT_CORE; >>> + >>> +// PPTT processor cluster structure >>> +typedef struct { >>> +=C3=AF=C2=BF=C2=BD EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR=C3=AF=C2=BF= =C2=BD Cluster; =C3=AF=C2=BF=C2=BD >>> >> +UINT32=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2= =BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF= =C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2= =BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD= =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3= =AF=C2=BF >> =C2=BD >>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2= =BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD= =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Offset; =C3=AF=C2=BF= =C2=BD >>> +EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD= =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD L3Cache; =C3=AF=C2= =BF=C2=BD >>> +RD_PPTT_CORE Core[CORE_COUNT]; } RD_PPTT_CLUSTER; >>> + >>> +// PPTT processor cluster structure without cache typedef struct { >>> +=C3=AF=C2=BF=C2=BD EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR=C3=AF=C2=BF= =C2=BD Cluster; =C3=AF=C2=BF=C2=BD >>> >> +UINT32=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2= =BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF= =C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2= =BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD= =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3= =AF=C2=BF >> =C2=BD >>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2= =BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD= =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Offset; >> I think there is no need for an offset here. Updating this will requir= e updating >> the other PPTT tables written. > Right. Will update. > >>> +=C3=AF=C2=BF=C2=BD RD_PPTT_CORE Core[CORE_COUNT]; >>> +} RD_PPTT_MINIMAL_CLUSTER; >>> + >>> +// PPTT processor package structure >>> +typedef struct { >>> +=C3=AF=C2=BF=C2=BD EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR=C3=AF=C2=BF= =C2=BD Package; =C3=AF=C2=BF=C2=BD >>> >> +UINT32=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2= =BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF= =C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2= =BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD= =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3= =AF=C2=BF >> =C2=BD >>> +=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2= =BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD= =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Offset; =C3=AF=C2=BF= =C2=BD >>> +EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD= =C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD Slc; =C3=AF=C2=BF=C2= =BD >>> +RD_PPTT_MINIMAL_CLUSTER Cluster[CLUSTER_COUNT]; } >>> +RD_PPTT_SLC_PACKAGE; #pragma pack () >>> + >>> +// >>> +// PPTT processor structure flags for different SoC components as >>> defined in >>> +// ACPI 6.3 specification >>> +// >>> + >> [...] >>> +// EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR >>> +#define EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR_INIT(Length, Flag, >>> Parent,=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2= =BF=C2=BD=C3=AF=C2=BF=C2=BD=C3=AF=C2=BF=C2=BD \ >>> +=C3=AF=C2=BF=C2=BD ACPIProcessorID, NumberOfPrivateResource) \ >> I think it should be possible to remove the 'Length' parameter and com= pute it >> as: >> sizeof (EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR) + >> NumberOfPrivateResource * sizeof >> (EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE) + NumberOfPrivateResource * >> sizeof (UINT32) >> > As per 6.4 specification, table 5.138, the Length is "Length of the loc= al processor > structure in bytes" It is just the length of local processor, not the e= ntire structure. Yes indeed you are right. Thanks for the answer, Pierre