DynamicTablesPkg: Add ACPI HMAT table.

[SophiaWang-Google]

Description

DynamicTablesPkg: Add ACPI HMAT table.

• Breaking change?
  N/A
• Impacts security?
  N/A
• Includes tests?
  N/A

How This Was Tested

Use below commands:
acpidump -o acpi.log
acpixtract -s HMAT acpi.log
iasl -d hmat.dat
cat hmat.dsl

to dump the HMAT table, the data looks right.

Integration Instructions

N/A

[mdkinney]

Commit message is missing description and Signed-off-by.

https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format

[pierregondois]

Same comment, this should be a token referencing a CM_ARCH_COMMON_MEMORY_AFFINITY_INFO struct

[pierregondois]

Similar comment, this should be a token I think

[abdattar]

make the HMAT table generator as generic library rather than Arm specific library.
X64 also supports the HMAT table.

[samimujawar]

I think this needs to be an arch common object.

Also, there is a PR at https://github.com/tianocore/edk2-staging/pull/510/files which adds Dynamic SMBIOS support and is very close to being merged in the staging branch. Once merged, it should be possible to implement SMBIOS generators as well. The initial plan would be to have 1 or 2 SMBIOS generators on the staging branch and their corresponding implementation in edk2-platforms staging branch to demonstrate the usage. I think we are very close to getting there.
This means we will have Dynamic SMBIOS support merged in edk2 mainline next.

@gmahadevan Any thoughts on how this would work with Dynamic SMBIOS?

[SophiaWang-Google]

According to ACPI spec, InitiatorProximityDomainList and TargetProximityDomainList are just numbers/indexes of proximity domain.Please refer to https://stevescargall.com/blog/2022/11/linux-numa-distances-explained/

If we use CM_OBJECT_TOKEN to refer to every proximity domain, that means we need to create the big array of CM_ARCH_COMMON_INITIATOR_DOMAIN_LIST, for example: if the system have 400 CPUs, and only have 4 proximity domains, we need to create array of CM_ARCH_COMMON_INITIATOR_DOMAIN_LIST with size of 400, and each cpu points to one element inside the array.

It turns out to be:
cpu 0 ---> proximity domain 0
cpu 1 ---> proximity domain 0
...
cpu 99 ---> proximity domain 0
cpu 100 ---> proximity domain 1
...
cpu 199 ---> proximity domain 1
...
cpu 399 ---> proximity domain 3

Do we really want to create that big size of CM_ARCH_COMMON_TARGET_DOMAIN_LIST array, or we can rely on edk2-platform to fill in proximity numbers (in above example: it is 4)

To avoid hard-coded array size in the original version, we can use separate data structures as shown in current version:
typedef struct CmArchCommonInitiatorProximityDomainList {
UINT32 InitiatorProximityDomainList;
} CM_ARCH_COMMON_INITIATOR_DOMAIN_LIST;

/** A structure that describes the Target Proximity Domain List Info.
ID: EArchCommonObjTargetDomainList
*/
typedef struct CmArchCommonTargetProximityDomainList {
UINT32 TargetProximityDomainList;
} CM_ARCH_COMMON_TARGET_DOMAIN_LIST;

/** A structure that describes the Relative Distance Entry Info.
ID: EArchCommonObjRelativeDistanceEntry
*/
typedef struct CmArchCommonRelativeDistanceEntry
UINT16 RelativeDistanceEntry;
} CM_ARCH_COMMON_RELATIVE_DISTANCE_ENTRY;

And rely on the InitiatorProximityDomainCount
Status = GetEArmObjInitiatorProximityDomain (
CfgMgrProtocol,
CM_NULL_TOKEN,
&InitiatorProximityDomainInfo,
&InitiatorProximityDomainCount
);
to fill in multiple entries of CM_ARCH_COMMON_INITIATOR_DOMAIN_LIST

Please let me know your preference.

[pierregondois]

Hello Sophia,
I think it would be good to use a CM_ARCH_COMMON_OBJ_REF to store the Initiator/Target.
Indeed, we should ideally avoid duplication of the information:

• If the index of a domain changes (for any reason), we would have to find back where this index was used and modify it aswell.
• If the index need to be generated one day, it will be difficult to patch all the places where the index was hand-written
• Ideally, we should actually not need a ProximityDomain field in the CM_ARM_GICC_INFO, we should store a reference to the proximity domain the CPU belongs to. Unfortunately this design was too complex to start with, but IMO we should avoid hand-writing values that can be generated

[SophiaWang-Google]

Hi, Pierre,

I totally agree with you to avoid using hand-writing values and store a reference to the proximity domain the CPU belongs to can be the very clean design.

Given it takes time to develop that solution, I can see two options here:

1. in edk2/, using the reference token to connect GICC ProximityDomain and latency BW table's ProximityDomain, that means, if we have 400 CPUs/GICCs, latency BW table will have 400 entries with a bunch of duplicated 0s,1s ... as I mentioned in previous comment.
2. in edk2-platform/, we patch the ProximityDomain information using the formula:
   Initiator ProximityDomain = # of GICC counts / # of GICC counts per domain
   Or other formula to calculate the # of initiator ProximityDomain. In this way, we rely on different manufactures to apply the formula per their HW design.
   The same applied to Target ProximityDomain.

With option 2, we do not hand writing the number, we still use GICC counts to calculate the domains numbers, that means, if any modifications on GICC (CPU) proximity domains, it will be automatically reflected in HMAT.
With option 2, we also avoid to have 400 entries in latency BW table's ProximityDomain list. We only have 4 entries in latency BW table's ProximityDomain if GICC only have 4 initiator domains (per example in previous comment).

Please let me know which option you prefer, so I can implement accordingly.

[pierregondois]

Hello Sophia,
when reading your message, it seems I might have missed/misunderstood something.
Please let me come back to you tomorrow to be sure about the suggestion.

[pierregondois]

Hello Sophia,
The best solution would be to deprecate the CM_ARM_GICC_INFO.ProximityDomain
and CM_ARM_GICC_INFO.ClockDomain fields I think. Same for the CM_X64_LOCAL_APIC_X2APIC_AFFINITY_INFO struct.

Ideally we should:
1-
Add a new CM_ARCH_COMMON_DOMAIN object:

typedef struct CmArchCommonDomain {
  /// Generate Domain Id
  /// TRUE if the DynamicTablesPkg framework should
  /// generate the Domain Id
  BOOLEAN GenerateDomainId;

  /// If GenerateDomainId is FALSE, use this value
  /// for the DomainId;
  UINT32 DomainId;
} CM_ARCH_COMMON_DOMAIN;

2-
Add new fields in the CM_ARM_GICC_INFO structure:

• ProximityDomainToken: Token referencing a CM_ARCH_COMMON_DOMAIN
• ClockDomainToken: Token referencing a CM_ARCH_COMMON_DOMAIN

3-
Deprecate the following fields in CM_ARM_GICC_INFO:

• ProximityDomain
• ClockDomain
  The SratGenerator is the only consumer of these fields. We will have to either:
• Check if the ProximityDomainToken|ClockDomainToken are populated. If yes, use them to generate the Srat table, otherwise use the previous ProximityDomain|ClockDomain fields
• Create a second SratGenerator for the new users and don't modify the initial SratGenerator

4-
The diagram would look like that:

CM_ARCH_COMMON_MEMORY_LAT_BW_INFO.TargetProximityDomainListToken-------+
CM_ARCH_COMMON_MEMORY_LAT_BW_INFO.InitiatorProximityDomainListToken-+  |
                                                                    |  |
                                                                    |  |
              +-----------------------------------------------------+  |
              |                                                        |
              v                                                        |
+-------------------------+                                            |
|  CM_ARCH_COMMON_OBJ_REF |                                            |
|  (this is an array)     |                                            |
|-------------------------|                                            |
|          Token[0]-------+----------------+                           |
|          Token[1]-+     |                |                           |
+-------------------|-----+                |                           |
                    |                      |                           |
                    v                      v                           v
+-------------------------+   +-------------------------+   +-------------------------+
|   CM_ARCH_COMMON_DOMAIN |   |   CM_ARCH_COMMON_DOMAIN |   |   CM_ARCH_COMMON_DOMAIN |
|     (Proximity Domain)  |   |     (Proximity Domain)  |   |     (Proximity Domain)  |
|-------------------------|   |-------------------------|   |-------------------------|
| GenerateDomainId::FALSE |   | GenerateDomainId::FALSE |   | GenerateDomainId::FALSE |
| DomainId::0             |   | DomainId::1             |   | DomainId::2             |
+-------------------------+   +-------------------------+   +-------------------------+
         ^ ^                                  ^ ^
         | |                                  | |
         | +--------------------------------+ | |
         +--------------------------------+ | | |
                                          | | | |
+---------------------------------+       | | | |
|        CM_ARM_GICC_INFO         |       | | | |
|  (this is an array)             |       | | | |
|---------------------------------|       | | | |
| GicC[0].ProximityDomainToken----+-------+ | | |
| GicC[1].ProximityDomainToken----+---------+ | |
| GicC[2].ProximityDomainToken----+-----------+ |
| GicC[3].ProximityDomainToken----+-------------+
+---------------------------------+

---

---

---

And to depict the relative distance between domains:
1-
Add a strucure:

typedef struct CmArchCommonRelativeDistanceEntry {
  CM_OBJECT_TOKEN FirstDomain;
  CM_OBJECT_TOKEN SecondDomain;
  UINT16          RelativeDistance;
} CM_ARCH_COMMON_RELATIVE_DISTANCE_ENTRY;

4-
The diagram would look like that:

+-------------------------+   +-------------------------+   +-------------------------+
|   CM_ARCH_COMMON_DOMAIN |   |   CM_ARCH_COMMON_DOMAIN |   |   CM_ARCH_COMMON_DOMAIN |
|     (Proximity Domain)  |   |     (Proximity Domain)  |   |     (Proximity Domain)  |
|-------------------------|   |-------------------------|   |-------------------------|
| GenerateDomainId::FALSE |   | GenerateDomainId::FALSE |   | GenerateDomainId::FALSE |
| DomainId::0             |   | DomainId::1             |   | DomainId::2             |
+-------------------------+   +-------------------------+   +-------------------------+
            ^                                    ^                      ^   ^
            |                                    |                      |   |
            |                                    |                      |   |
            +-------------------------------+    |                      |   |
                                            |    |                      |   |
+----------------------------------------+  |    |                      |   |
| CM_ARCH_COMMON_RELATIVE_DISTANCE_ENTRY |  |    |                      |   |
|    (This is an array)                  |  |    |                      |   |
+----------------------------------------+  |    |                      |   |
|  (Index0)                              |  |    |                      |   |
|  FirstDomain---------------------------+--+    |                      |   |
|  SecondDomain--------------------------+-------|----------------------+   |
|  RelativeDistance::4 (distance of 4)   |       |                          |
+----------------------------------------+       |                          |
|  (Index1)                              |       |                          |
|  FirstDomain---------------------------+-------+                          |
|  SecondDomain--------------------------+----------------------------------+
|  RelativeDistance::6 (distance of 6)   |
+----------------------------------------+
                ^
                |
                +---------------------------------------------+
                                                              |
CM_ARCH_COMMON_MEMORY_LAT_BW_INFO.RelativeDistanceEntryToken--+

@samimujawar , would it be possible to also have a look if you're ok with this design ?
@SophiaWang-Google If the design seems ok to you and Sami, I can help regarding the rework, as you are mainly interested in the HmatGenerator I assume,

Regards,
Pierre

[SophiaWang-Google]

Pierre,

Thanks for the detailed explanation.

Now I fully understand your design and indeed it will enable upstream's capability of generating values that can be re-used across generators.

The design looks good and I will park my HMAT generator work until your new design landed in upstream.

Please let me know anything else I can do to help.

[SophiaWang-Google]

Pierre,

Can you please give rough estimate when this re-architecture work will be done?

Given HMAT generator code is ready with current code base, can we review and get the code merged for now and change the code later with your newly re-architecture work?

Please kindly let me know.

[pierregondois]

Hello Sophia,
I think it would be better (at least for me) to use the correct objects in the first place. Would it be possible to rebase your work on this branch and use the CmObj that are there ?

Normally there should not need more information than that. This will require a bit of juggling though...

https://github.com/pierregondois/edk2/tree/pg/proximity_domain_rework

[SophiaWang-Google]

Hello Pierre,

Thanks for working on proximity_domain_rework, I appreciate the speed.

I just updated ArchCommonNameSpaceObjects.h by using CmObj.

For now, I will pause the update of this CL until https://github.com/pierregondois/edk2/tree/pg/proximity_domain_rework is merged.

Please kindly let me know when HMAT generator work can be resumed.

Best,
Sophia

[pierregondois]

Hello Sophia,
ok yes understood

[github-actions]

This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

[mergify]

PR can not be merged due to conflict. Please rebase and resubmit

[github-actions]

This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

[github-actions]

This pull request has been automatically been closed because it did not have any activity in 60 days and no follow up within 7 days after being marked stale. Thank you for your contributions.