Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 9 Feb 2022 22:34:14 +0000
From:   Yazen Ghannam <yazen.ghannam@amd.com>
To:     Naveen Krishna Chatradhi <nchatrad@amd.com>
Cc:     linux-edac@vger.kernel.org, x86@kernel.org,
        linux-kernel@vger.kernel.org, bp@alien8.de, mingo@redhat.com,
        mchehab@kernel.org, Muralidhara M K <muralimk@amd.com>
Subject: Re: [PATCH v7 01/12] EDAC/amd64: Document heterogeneous enumeration
Message-ID: <YgRBZjUN3AoHr7q+@yaz-ubuntu>
References: <20220203174942.31630-1-nchatrad@amd.com>
 <20220203174942.31630-2-nchatrad@amd.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220203174942.31630-2-nchatrad@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MWpmWTNoM3BqS0F5bDhMdkRralJpbG5PZ2ZrY2xQdVZMQ28xZXczNUNIVk1x?=
 =?utf-8?B?aWk4WW5OaFRhQUo2dmJxSjFMR0lPdFlMcHhSRDJCQlhJdGwxa2FpdFliblV6?=
 =?utf-8?B?S3QzV0NZZ2dKbURCWGZqM0l4bFYxY1FPVWxmQ2J3YTZkT0RtWXJIaGtXM2ZW?=
 =?utf-8?B?d1pwcEpCNTJuL2xDSndMUkMya2NtZkRIQkFWa29YRjFrZk9aYlJLSjYvaXQz?=
 =?utf-8?B?SC9udWp1ZWFMYzdVUWR1TWFoVy9yZ0s0alRQUEZ1M0h6NjNRd1NmY3F0VlBC?=
 =?utf-8?B?WGNFZ0daTTl4d0poMWxjdzk2VS9Pb3ZxTHRBcEVyakVOMlcvNC9CWUZWNHU5?=
 =?utf-8?B?MWNsUmVnU2dhMVA3MzVsTmFJcTNGek1JMHp3VkNVYXUzTk83WkRvTzJJTnJh?=
 =?utf-8?B?TlhSUUVFZ0xRYVV4M0VGeWozdDNnVjEzZUhYSWRWRGp0OVgyQ2F6UTdMRVIr?=
 =?utf-8?B?U3FiNmg4WUxvMmdYdGRlM09qalliakI1YnNMYWZiZkhTL3RGR1EyZ0hFbjRS?=
 =?utf-8?B?TnNOd2FaTWdWa1ZkVzhqUjF4NTVVdFE1VTUvcDR1K2Q5RXRDWU9ZN0h3TzZn?=
 =?utf-8?B?a0s1SUNXNXhoWDVWVWVlZjJFOVBCTlgxV2l4RmxEUWRNUEVPcEs1NGFzbDVo?=
 =?utf-8?B?WXIvUlEvL0E3SW5ySXhLVWxiZWRWV1VudzJLVTRvcDRIeGZ0ZGgyUWlPY1Ft?=
 =?utf-8?B?QnlXbnNZMER2eHhYWWZLTTRuaWRUbjlubVkrOThVMWtDRjdoMnA1N1M1UkRR?=
 =?utf-8?B?VW9NTHJ2bjJaMWxvTnBISXFiaW9sSEQxTzFJd2M3QkZNYWlpN1hsODZXRXRC?=
 =?utf-8?B?Y0NOMjhvcld5VjBsd3d3dXJkREVsWFVWcnF2VkVISU9LVnZidk5BYkVpYnlC?=
 =?utf-8?B?TFlsQ2JGTUlyTXU5eFR4NDhkWFYzZGhoMTNJQk9LTWlrNFlLeXBIV2M1WGov?=
 =?utf-8?B?N3U4TnViZmVidlQ4cUVWSGVjQUVmT0FvbUh6alI1K1o4U1FwTjRCTXRpV1Rz?=
 =?utf-8?B?UDV1UjdheUx6K21FMWpUMG1jMXMwYTdGa0NITm1wa1FzYk1EZEdxTXhpNkNP?=
 =?utf-8?B?bTlkWDd1MmlHeUg5RTZrMFF2ZGQxelFuWnRrcW4wTzhveUk5ODVVVk16eWFP?=
 =?utf-8?B?WSs2bzl2Ui8xcy82dTA1eTNBZCtpYjhkMmJ1ajB2OHBKYzRVdmxjejUyUW1D?=
 =?utf-8?B?aGt4clYrVXJ3N3lUbTg1dS95eDhha0htQ3pmQitnM3F2RFU0RlRFTmhKNU5J?=
 =?utf-8?B?TmJKbHFiNVhVREhQNitKbnhJWE9laXovVmEvZHNEUk9UQUp3WHRPR3JrUTFi?=
 =?utf-8?B?MWxlczNFWm9iYVd0K1F1Y1kybWtYR3FvRlZYczNyYzdOUVZaOWZQWlJPaVly?=
 =?utf-8?B?ZUNTMkVUNmlsR3F5NDljWFp1VitIaWNMRXpPOTZ0QVdsckQ5WnllUzh4WG5V?=
 =?utf-8?B?M1BGWVJ5MmVtaFc0OUYvR2JGbm1kdmdpVUx1dFhyaEtWaFQrZ2pYNFpNb0lm?=
 =?utf-8?B?VFgxaWlxdWNTZkdTT0pURUdvUE5vcUdOUkx5dUtGb3lMcjFhcEhOMXdGVGlF?=
 =?utf-8?B?STFGLzlQOEUxRUd0NzB0ekdudkU5d1FwVFBBQytYV0cycFBLMStnNHlaMUNv?=
 =?utf-8?B?MUJZY0dVYlovckhmdGUxdUYwYm04OWs2ZWpqNEFxZ0xwRTlqdHk0NzRScVQ4?=
 =?utf-8?B?R21zNlNsZHVEcXQyVWhYQmZIc2k2YWZvRFFFclNabkhWU3RVR3dOVUtIYVlt?=
 =?utf-8?B?Q08xZzh4SG9PbUxtQ0pzRmxZbUpsSGVFdHRnbVQ3azRjVVdSbkxGc01EeXlQ?=
 =?utf-8?B?S2F5NVpDcG9XSUtFdDZNQ0dneHZ2TzVvT25DNG1uOFZmeHhNY2RCSHBIVHpa?=
 =?utf-8?B?VXVFUDZKRTdpRGlFNWZhbWRhOTdmQkZCTTFLekg4dkV2MUg2QTR2cm94MWpa?=
 =?utf-8?B?QWY4QUQvdUREQlNXQkhPbmo4aUhDeUhvV25lQm5TZjVDa1JYMHkyczNEY0d1?=
 =?utf-8?B?aEhSaWNncHFidHhqYWFBR3pJOVh3QnJLVzlBdFBvVFFRK2xieXFZU2NPeHo3?=
 =?utf-8?B?WHNKWUM5WVRCYkRvc0NtVTNxZ1FaZ015QkNBaEVIMHd5L2tQQWdDT2I3ZFVU?=
 =?utf-8?B?d2FhVHFoOWxrcWNHRTFmaGdYSHFEeFVqRnpqTGRUYUZMMjltcFl0a1Rja1Ra?=
 =?utf-8?Q?OlCZ6y4D9eYBh3tQfL8AYUs=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9ac51102-9607-4178-0c9b-08d9ec1c5269
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3108.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Feb 2022 22:34:23.7991
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3hQ3rLtoP0QDjuL8eeDF5UO/0J10jRZR/l3PfnsM+/2Y4thdVxTUYsYtaOpbESOrCq08hSKflhGd2QiN32muSg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB3963
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Feb 03, 2022 at 11:49:31AM -0600, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> The Documentation notes have been added in amd64_edac.h and will be
> referring to driver-api wherever needed.

I don't see the comment in amd64_edac.h referring to driver-api/edac.rst. So
I'm not sure what this sentence is saying.

> 
> Explains how the physical topology is enumerated in the software and
> edac module populates the sysfs ABIs.
>

Also, please make sure the message is imperative, e.g "Add...", "Explain...",
etc.
 
> Signed-off-by: Muralidhara M K <muralimk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> v6->v7:
> * New in v7
> 
>  Documentation/driver-api/edac.rst |   9 +++
>  drivers/edac/amd64_edac.h         | 101 ++++++++++++++++++++++++++++++
>  2 files changed, 110 insertions(+)
> 
> diff --git a/Documentation/driver-api/edac.rst b/Documentation/driver-api/edac.rst
> index b8c742aa0a71..0dd07d0d0e47 100644
> --- a/Documentation/driver-api/edac.rst
> +++ b/Documentation/driver-api/edac.rst
> @@ -106,6 +106,15 @@ will occupy those chip-select rows.
>  This term is avoided because it is unclear when needing to distinguish
>  between chip-select rows and socket sets.
>  
> +* High Bandwidth Memory (HBM)
> +
> +HBM is a new type of memory chip with low power consumption and ultra-wide
> +communication lanes. It uses vertically stacked memory chips (DRAM dies)
> +interconnected by microscopic wires called "through-silicon vias," or TSVs.
> +
> +Several stacks of HBM chips connect to the CPU or GPU through an ultra-fast
> +interconnect called the “interposer". So that HBM’s characteristics are
> +nearly indistinguishable from on-chip integrated RAM.
> 

I think this makes sense.
 
>  Memory Controllers
>  ------------------
> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
> index 6f8147abfa71..6a112270a84b 100644
> --- a/drivers/edac/amd64_edac.h
> +++ b/drivers/edac/amd64_edac.h
> @@ -559,3 +559,104 @@ static inline u32 dct_sel_baseaddr(struct amd64_pvt *pvt)
>  	}
>  	return (pvt)->dct_sel_lo & 0xFFFFF800;
>  }
> +
> +/*
> + * AMD Heterogeneous system support on EDAC subsystem
> + * --------------------------------------------------
> + *
> + * An AMD heterogeneous system built by connecting the data fabrics of both CPUs
> + * and GPUs via custom xGMI links. So, the Data Fabric on the GPU nodes can be
> + * accessed the same way as the Data Fabric on CPU nodes.
> + *
> + * An Aldebaran GPUs has 2 Data Fabrics, each GPU DF contains four Unified
> + * Memory Controllers (UMC). Each UMC contains eight Channels. Each UMC Channel
> + * controls one 128-bit HBM2e (2GB) channel (equivalent to 8 X 2GB ranks),
> + * this creates a total of 4096-bits of DRAM data bus.
> + *
> + * While UMC is interfacing a 16GB (8H X 2GB DRAM) HBM stack, each UMC channel is

What is "8H"? Is that 8 "high"?

> + * interfacing 2GB of DRAM (represented as rank).
> + *
> + * Memory controllers on AMD GPU nodes can be represented in EDAC is as below:
> + *       GPU DF / GPU Node -> EDAC MC
> + *       GPU UMC           -> EDAC CSROW
> + *       GPU UMC channel   -> EDAC CHANNEL
> + *
> + * Eg: An heterogeneous system with 1 AMD CPU is connected to 4 Aldebaran GPUs using xGMI.
> + *
> + * AMD GPU Nodes are enumerated in sequential order based on the PCI hierarchy, and the
> + * first GPU node is assumed to have an "Node ID" value after CPU Nodes are fully
> + * populated.
> + *
> + * $ ls /sys/devices/system/edac/mc/
> + *	mc0   - CPU MC node 0
> + *	mc1  |
> + *	mc2  |- GPU card[0] => node 0(mc1), node 1(mc2)
> + *	mc3  |
> + *	mc4  |- GPU card[1] => node 0(mc3), node 1(mc4)
> + *	mc5  |
> + *	mc6  |- GPU card[2] => node 0(mc5), node 1(mc6)
> + *	mc7  |
> + *	mc8  |- GPU card[3] => node 0(mc7), node 1(mc8)
> + *
> + * sysfs entries will be populated as below:
> + *
> + *	CPU			# CPU node
> + *	├── mc 0
> + *
> + *	GPU Nodes are enumerated sequentially after CPU nodes are populated
> + *	GPU card 1		# Each Aldebaran GPU has 2 nodes/mcs
> + *	├── mc 1		# GPU node 0 == mc1, Each MC node has 4 UMCs/CSROWs
> + *	│   ├── csrow 0		# UMC 0
> + *	│   │   ├── channel 0	# Each UMC has 8 channels
> + *	│   │   ├── channel 1   # size of each channel is 2 GB, so each UMC has 16 GB
> + *	│   │   ├── channel 2
> + *	│   │   ├── channel 3
> + *	│   │   ├── channel 4
> + *	│   │   ├── channel 5
> + *	│   │   ├── channel 6
> + *	│   │   ├── channel 7
> + *	│   ├── csrow 1		# UMC 1
> + *	│   │   ├── channel 0
> + *	│   │   ├── ..
> + *	│   │   ├── channel 7
> + *	│   ├── ..		..
> + *	│   ├── csrow 3		# UMC 3
> + *	│   │   ├── channel 0
> + *	│   │   ├── ..
> + *	│   │   ├── channel 7
> + *	│   ├── rank 0
> + *	│   ├── ..		..
> + *	│   ├── rank 31		# total 32 ranks/dimms from 4 UMCs
> + *	├
> + *	├── mc 2		# GPU node 1 == mc2
> + *	│   ├── ..		# each GPU has total 64 GB
> + *
> + *	GPU card 2
> + *	├── mc 3
> + *	│   ├── ..
> + *	├── mc 4
> + *	│   ├── ..
> + *
> + *	GPU card 3
> + *	├── mc 5
> + *	│   ├── ..
> + *	├── mc 6
> + *	│   ├── ..
> + *
> + *	GPU card 4
> + *	├── mc 7
> + *	│   ├── ..
> + *	├── mc 8
> + *	│   ├── ..
> + *
> + *
> + * Heterogeneous hardware details for above context as below:
> + * - The CPU UMC (Unified Memory Controller) is mostly the same as the GPU UMC.
> + *   They have chip selects (csrows) and channels. However, the layouts are different
> + *   for performance, physical layout, or other reasons.
> + * - CPU UMCs use 1 channel. So we say UMC = EDAC Channel. This follows the
> + *   marketing speak, example. CPU has X memory channels, etc.
> + * - CPU UMCs use up to 4 chip selects. So we say UMC chip select = EDAC CSROW.
> + * - GPU UMCs use 1 chip select. So we say UMC = EDAC CSROW.
> + * - GPU UMCs use 8 channels. So we say UMC Channel = EDAC Channel.
> + */
> --

This makes sense to me. I'm interested to see if there's any feedback from
others though.

Please fix up the commit message. Otherwise, I think this looks good.

Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>

Thanks,
Yazen