public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Jeremy Linton <jeremy.linton@arm.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	Leif Lindholm <leif.lindholm@linaro.org>,
	Graeme Gregory <graeme.gregory@linaro.org>
Subject: Re: [PATCH edk2-platforms] Silicon/SynQuacer: add PPTT ACPI table to describe cache topology
Date: Thu, 8 Mar 2018 11:50:44 -0600	[thread overview]
Message-ID: <d9aadbe1-2cba-a821-0fc5-971332c19e7b@arm.com> (raw)
In-Reply-To: <CAKv+Gu84ua4q1ChbBO=SR2EzNE46cE0iBzPfuiLA3-YjqucuPw@mail.gmail.com>

Hi,

On 03/08/2018 11:27 AM, Ard Biesheuvel wrote:
> On 8 March 2018 at 17:24, Jeremy Linton <jeremy.linton@arm.com> wrote:
>> Hi,
>>
>>
>> On 03/08/2018 07:13 AM, Ard Biesheuvel wrote:
>>>
>>> Add a ACPI Processor Properties Topology Table (PPTT) to the SynQuacer
>>> builds. This information is used by the OS to tune the scheduler.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>> This produces the following topology after applying Jeremy's patches:
>>>
>>> $ lstopo-no-graphics
>>> Machine (31GB)
>>>     Package L#0 + L3 L#0 (4096KB)
>>>       L2 L#0 (256KB)
>>>         L1d L#0 (32KB) + L1i L#0 (32KB) + Core L#0 + PU L#0 (P#0)
>>>         L1d L#1 (32KB) + L1i L#1 (32KB) + Core L#1 + PU L#1 (P#1)
>>>       L2 L#1 (256KB)
>>>         L1d L#2 (32KB) + L1i L#2 (32KB) + Core L#2 + PU L#2 (P#2)
>>>         L1d L#3 (32KB) + L1i L#3 (32KB) + Core L#3 + PU L#3 (P#3)
>>>       L2 L#2 (256KB)
>>>         L1d L#4 (32KB) + L1i L#4 (32KB) + Core L#4 + PU L#4 (P#4)
>>>         L1d L#5 (32KB) + L1i L#5 (32KB) + Core L#5 + PU L#5 (P#5)
>>>       L2 L#3 (256KB)
>>>         L1d L#6 (32KB) + L1i L#6 (32KB) + Core L#6 + PU L#6 (P#6)
>>>         L1d L#7 (32KB) + L1i L#7 (32KB) + Core L#7 + PU L#7 (P#7)
>>>       L2 L#4 (256KB)
>>>         L1d L#8 (32KB) + L1i L#8 (32KB) + Core L#8 + PU L#8 (P#8)
>>>         L1d L#9 (32KB) + L1i L#9 (32KB) + Core L#9 + PU L#9 (P#9)
>>>       L2 L#5 (256KB)
>>>         L1d L#10 (32KB) + L1i L#10 (32KB) + Core L#10 + PU L#10 (P#10)
>>>         L1d L#11 (32KB) + L1i L#11 (32KB) + Core L#11 + PU L#11 (P#11)
>>>       L2 L#6 (256KB)
>>>         L1d L#12 (32KB) + L1i L#12 (32KB) + Core L#12 + PU L#12 (P#12)
>>>         L1d L#13 (32KB) + L1i L#13 (32KB) + Core L#13 + PU L#13 (P#13)
>>>       L2 L#7 (256KB)
>>>         L1d L#14 (32KB) + L1i L#14 (32KB) + Core L#14 + PU L#14 (P#14)
>>>         L1d L#15 (32KB) + L1i L#15 (32KB) + Core L#15 + PU L#15 (P#15)
>>>       L2 L#8 (256KB)
>>>         L1d L#16 (32KB) + L1i L#16 (32KB) + Core L#16 + PU L#16 (P#16)
>>>         L1d L#17 (32KB) + L1i L#17 (32KB) + Core L#17 + PU L#17 (P#17)
>>>       L2 L#9 (256KB)
>>>         L1d L#18 (32KB) + L1i L#18 (32KB) + Core L#18 + PU L#18 (P#18)
>>>         L1d L#19 (32KB) + L1i L#19 (32KB) + Core L#19 + PU L#19 (P#19)
>>>       L2 L#10 (256KB)
>>>         L1d L#20 (32KB) + L1i L#20 (32KB) + Core L#20 + PU L#20 (P#20)
>>>         L1d L#21 (32KB) + L1i L#21 (32KB) + Core L#21 + PU L#21 (P#21)
>>>       L2 L#11 (256KB)
>>>         L1d L#22 (32KB) + L1i L#22 (32KB) + Core L#22 + PU L#22 (P#22)
>>>         L1d L#23 (32KB) + L1i L#23 (32KB) + Core L#23 + PU L#23 (P#23)
>>>     HostBridge L#0
>>>       PCIBridge
>>>         PCIBridge
>>>           PCI 1b21:0612
>>>             Block(Disk) L#0 "sda"
>>>     HostBridge L#3
>>>       PCI 10de:128b
>>>         GPU L#1 "renderD128"
>>>         GPU L#2 "card0"
>>>         GPU L#3 "controlD64"
>>>
>>>    Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf |   1 +
>>>    Silicon/Socionext/SynQuacer/AcpiTables/Pptt.aslc      | 221
>>> ++++++++++++++++++++
>>
>>
>> So, the above looks good. OTOH, this is yet another manually
>> created/hard-coded ACPI table, subject to change every-time another SOC is
>> released. I have a couple similar ones, but haven't post them because I
>> believe the HiSi folks have done us a favor and created a table generator
>> which does 90% of this work by probing the hardware, and creating a
>> "compressed" representation of the table. Leaving the individual platforms
>> to only fill out LLCs and such which can't be probed.
>>
>> It would be great if the remaining HiSi bits were removed and the whole
>> thing made generic enough to plug in to these individual platforms, so that
>> they only need supply their nonstandard bits and the rest is taken care of.
>>
> 
> Yeah, I am aware of that. But to be honest, for a platform such as
> this one, where the information is 100% static, I'd much rather have a
> single .c file like this that never changes once you check it in.
> 

Maybe, but even if there is never an identical machine with a few cores 
extra (or removed), you have to deal with the possibility that the 
standard is going to be updated (say to add a leaf node flag). If/when 
that happens you now have the technical debt of having to go manually 
touch the tables vs updating the generator code and being done with it 
across all the platforms. Particularly since other people are just going 
to take the same shortcut next time of just copy-pasting this table and 
tweaking it, rather than trying to create a library out of the HiSi code.


  reply	other threads:[~2018-03-08 17:44 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-08 13:13 [PATCH edk2-platforms] Silicon/SynQuacer: add PPTT ACPI table to describe cache topology Ard Biesheuvel
2018-03-08 17:24 ` Jeremy Linton
2018-03-08 17:27   ` Ard Biesheuvel
2018-03-08 17:50     ` Jeremy Linton [this message]
2018-03-08 17:59       ` Ard Biesheuvel
2018-03-08 20:38         ` Ard Biesheuvel
2018-03-08 21:48           ` Jeremy Linton
2018-03-08 22:56             ` Ard Biesheuvel
2018-04-27 10:58 ` Leif Lindholm
2018-04-27 12:12   ` Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d9aadbe1-2cba-a821-0fc5-971332c19e7b@arm.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox