Received-SPF: pass (google.com: domain of linux-kernel+bounces-119157-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <010bb8bb-cc48-4b83-b46b-14e5efc753a8@amd.com>
Date: Tue, 26 Mar 2024 10:13:21 -0400
User-Agent: Mozilla Thunderbird
Cc: yazen.ghannam@amd.com, bp@alien8.de, linux-edac@vger.kernel.org,
 tony.luck@intel.com, linux-kernel@vger.kernel.org, avadhut.naik@amd.com,
 muralidhara.mk@amd.com
Subject: Re: [PATCH 2/4] RAS: ATL: Expand helpers for adding and removing base
 and hole
Content-Language: en-US
To: John Allen <john.allen@amd.com>
References: <20240314163527.63321-1-john.allen@amd.com>
 <20240314163527.63321-3-john.allen@amd.com>
 <ff9e4658-18c4-4a36-962a-373c15c337a2@amd.com>
 <ZgHQB2yasP1DeLcy@AUS-L1-JOHALLEN.amd.com>
From: Yazen Ghannam <yazen.ghannam@amd.com>
In-Reply-To: <ZgHQB2yasP1DeLcy@AUS-L1-JOHALLEN.amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?RlZOUWNZWVgyL2V3QzdsUmhXd3JOcmVMN1JDSytwaVpWYUE3djhTUTZRUldw?=
 =?utf-8?B?QXNoeXVxVXdwRTMrVXEyOHJ2SXdWSWJ4bjhYSG5Td1RCVHRKcmtERlRpekV4?=
 =?utf-8?B?ajYxWXlCNG8xL0hrRzl0eUpZbkpCQ2N3L0dFZkFpSEV3QmhCdVZkRUlMRWlY?=
 =?utf-8?B?cW84RjZuOHhCbkd5QlJpc0EwZ2psOVFmUW5FQTlDbGNGWGtSRU4yM2VWZ0NV?=
 =?utf-8?B?bzJudEpCM1RHVVZGR3cyS09admJpdXNGQjJPdUxleHdtb1dXZVF6V1VJaVk4?=
 =?utf-8?B?YzFTMHhTVmdDeEY0ZWpGRXl1TUZaU0FORHUvbUM4aGlDVmkzRkV1M3BSc1dk?=
 =?utf-8?B?UmE0OVJWWEUreG5Cak1PTm1hc0lvTktKQUlGbWlmeUpneHYxMDYyVVlpYlcw?=
 =?utf-8?B?UmMwQ0JpN1gzQjkxelRPTlNYOFBRdEErN1p0VWhvQS8vU0hxMzNDbWk3a281?=
 =?utf-8?B?YzdjRzB3K0tHcVpWUzRJSmpSTHVjNXZvMnpMSXFUZ2RPZXZhdWpCWFdRb21J?=
 =?utf-8?B?WWJqUlRlZnV4NVdCczBFUkVkNnhUVmpYdjNzV3VPbUFodUNTelIrNnBHdDNq?=
 =?utf-8?B?Z2ZzbzVRQXNqd1BRR1lVTmdjaEVmdEJXUnE0aS9qNVJieFE3YmwvczdZaDE4?=
 =?utf-8?B?M2pLZG54Z2VFekx0dHJtTllRYm1aYkRleCs4dUdqSlE3dHo2NTJkRzBJb2Y4?=
 =?utf-8?B?L2M0aVhXZ1NQQXlVclF6STVQekRad1F2Nm15RmhiZlpla3BwcVgxc0ZZcGJL?=
 =?utf-8?B?YU9GTHZ5L2VRbkw1bGNhYWxoeXZVNFVMVFI1TjRwdjZWaXhieTZIVmczb2Zq?=
 =?utf-8?B?YSs3cU82RmpWV0tKd1Qvdk5GZDBhRFVxUjBEUnVPeDFLQWYxODd3ajFmaUdL?=
 =?utf-8?B?RHpMYnJLaW1LendOTTBORDJGczE4eDc0RkJNa0ZNU0VhQmtHSU91eDY5QVA2?=
 =?utf-8?B?ZXhydFY1TTJNS1FpY0g5VEdxSXpSZDhWaFViMlkrSzFPTzBJY240eWQ5bksy?=
 =?utf-8?B?bTVYQ3kydjEwUFZoMit0cHdvbHp1Q0JhUVhVU3lRaTBzYkRVTllYdGtoTk5w?=
 =?utf-8?B?dXBOc0pHc0U4NnlrUDZXN3pZb2RlNWt5dHJmQlp5aFl6S2JWc3UyUXdiMk9H?=
 =?utf-8?B?U2I3a0MwK09BN1UzZXZGa2ZBZFcrYjExQzZPVlE5dXRwZ3hYTzVTVDR2WVNZ?=
 =?utf-8?B?anZmT0thWWZTd1NZYU13YjVFT0VPTi9oanBEV256L2JwVWZLakN3ZjJpY1Jr?=
 =?utf-8?B?WXdBNFF4ZndCZkNhNC83SXJmM0R3Wk5QS1VlRGQvcHhJdUsvSmJxU1ZabmNi?=
 =?utf-8?B?cm05QnI5TW5GZFhGR0h1V3U5WnRrTmZRYndQSFJncVk2WUlhdVhsZEliWkVX?=
 =?utf-8?B?NXVJNG1nelVQOGhRWUJkZjFaNVArdkpvaHF3TnhaaWx5d3lndjNPQjFHdk9V?=
 =?utf-8?B?S3JzRS9aMkJSVXEyZ25ZKzAxQVJ6amZvZ25BbTlkenNWR25jMnZmWkVJckY2?=
 =?utf-8?B?TkgycVJ0YzlQNk4zOVp5WW1VNjMrY3RxSlY3bUVFTm9ZUTlDQUMveDBVT2Jq?=
 =?utf-8?B?RmY0RitWNGdWb0wyQldEYSs1Zy9xcTBnUE1IT0RBTXRZeCtzNk5mc3NVNG42?=
 =?utf-8?B?d0Y0OGZkWWgyMy9obHhCN2I1dlFQUWZ1QW0xT2lHOVA3dHliSGdYOXQxakwz?=
 =?utf-8?B?cDFLeG5GOTc5TnltTEtiOXVVZXBXTFFMZEhLTFBNZEVmMTFmSEZweloycmlr?=
 =?utf-8?B?anlNODlONmIzYXRweGFJdDZPdHVlQWtGSVdzU3M0TFdzWU1xc1ZvbWdrc3Jr?=
 =?utf-8?B?cngvanB1MWh0TEJGYjJsRURuZHlRZDBSMnZ5UjY1c3l1ZDU3alhHYVhYN0tH?=
 =?utf-8?B?MmNQTTdWSjkvZldtNnJBc0gzWFBBTm9NNUo2UFFYRUMyUTV4cXdPUHRmamM0?=
 =?utf-8?B?Q2E1anRsYkpDZmVkSGtObnlYUGpOOHYybm9VbXFBTCs5SjRnYUpra1JIQmQ5?=
 =?utf-8?B?NEVrakVtVTZvRGw0VjhyQ1Nkb2xzbGtKczFNQUEyREplc3ltdDdCckJCV2ZV?=
 =?utf-8?B?akRPMDlGV3h2V3hJT1lPNUxHbDdYR2FUN1NwWE1DdzRWblQ0NE15N3NyNUto?=
 =?utf-8?Q?QRPSpgSBLmwYhMzFmMynTcC/n?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 24343e1a-38df-49fa-894f-08dc4d9ee652
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3109.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Mar 2024 14:13:24.7774
 (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: I3RrmMG1BzQvBEfQFu4HA/EcC2AA56nPpC/rQT58reOMakxMORs8Uc/znKmvrjtyt1K2762/uTEdQDPV6vY6pg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV2PR12MB5991

On 3/25/24 15:27, John Allen wrote:
> On Mon, Mar 18, 2024 at 11:28:48AM -0400, Yazen Ghannam wrote:
>> On 3/14/24 12:35, John Allen wrote:
>>> Data fabric 4.5 denormalization will need to frequently add and remove
>>
>> More specifically, the non-power-of-2 cases will need this.
>>
>>> the base and the legacy MMIO hole. Modify existing helpers to improve DF
>>> 4.5 denormalization flow and add helper to remove the base and hole.
>>
>> Please write the what/context, why/issue, and how/fix information as
>> separate paragraphs even if they're just a single sentence each. I think
>> this helps to find the details more easily.
>>
>>>
>>> Signed-off-by: John Allen <john.allen@amd.com>
>>> ---
>>>    drivers/ras/amd/atl/core.c     | 43 ++++++++++++++++++++++------------
>>>    drivers/ras/amd/atl/internal.h |  3 +++
>>>    2 files changed, 31 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/ras/amd/atl/core.c b/drivers/ras/amd/atl/core.c
>>> index c1710d233adb..cafdfc57d929 100644
>>> --- a/drivers/ras/amd/atl/core.c
>>> +++ b/drivers/ras/amd/atl/core.c
>>> @@ -49,15 +49,26 @@ static bool legacy_hole_en(struct addr_ctx *ctx)
>>>    	return FIELD_GET(DF_LEGACY_MMIO_HOLE_EN, reg);
>>>    }
>>> -static int add_legacy_hole(struct addr_ctx *ctx)
>>> +static u64 add_legacy_hole(struct addr_ctx *ctx, u64 addr)
>>>    {
>>>    	if (!legacy_hole_en(ctx))
>>> -		return 0;
>>> +		return addr;
>>> -	if (ctx->addr >= df_cfg.dram_hole_base)
>>> -		ctx->addr += (BIT_ULL(32) - df_cfg.dram_hole_base);
>>> +	if (addr >= df_cfg.dram_hole_base)
>>> +		addr += (BIT_ULL(32) - df_cfg.dram_hole_base);
>>> -	return 0;
>>> +	return addr;
>>> +}
>>> +
>>> +static u64 remove_legacy_hole(struct addr_ctx *ctx, u64 addr)
>>> +{
>>> +	if (!legacy_hole_en(ctx))
>>> +		return addr;
>>> +
>>> +	if (addr >= df_cfg.dram_hole_base)
>>> +		addr -= (BIT_ULL(32) - df_cfg.dram_hole_base);
>>> +
>>> +	return addr;
>>>    }
>>>    static u64 get_base_addr(struct addr_ctx *ctx)
>>> @@ -72,14 +83,16 @@ static u64 get_base_addr(struct addr_ctx *ctx)
>>>    	return base_addr << DF_DRAM_BASE_LIMIT_LSB;
>>>    }
>>> -static int add_base_and_hole(struct addr_ctx *ctx)
>>> +u64 add_base_and_hole(struct addr_ctx *ctx, u64 addr)
>>>    {
>>> -	ctx->ret_addr += get_base_addr(ctx);
>>> -
>>> -	if (add_legacy_hole(ctx))
>>> -		return -EINVAL;
>>> +	addr += get_base_addr(ctx);
>>> +	return add_legacy_hole(ctx, addr);
>>> +}
>>> -	return 0;
>>> +u64 remove_base_and_hole(struct addr_ctx *ctx, u64 addr)
>>> +{
>>> +	addr -= get_base_addr(ctx);
>>> +	return remove_legacy_hole(ctx, addr);
>>
>> This should be the inverse of the "add" operation, I think. So remove
>> the legacy hole first, then remove the base address.
>>
>>>    }
>>>    static bool late_hole_remove(struct addr_ctx *ctx)
>>> @@ -123,14 +136,14 @@ unsigned long norm_to_sys_addr(u8 socket_id, u8 die_id, u8 coh_st_inst_id, unsig
>>>    	if (denormalize_address(&ctx))
>>>    		return -EINVAL;
>>> -	if (!late_hole_remove(&ctx) && add_base_and_hole(&ctx))
>>> -		return -EINVAL;
>>> +	if (!late_hole_remove(&ctx))
>>> +		ctx.ret_addr = add_base_and_hole(&ctx, ctx.ret_addr);
>>>    	if (dehash_address(&ctx))
>>>    		return -EINVAL;
>>> -	if (late_hole_remove(&ctx) && add_base_and_hole(&ctx))
>>> -		return -EINVAL;
>>> +	if (late_hole_remove(&ctx))
>>> +		ctx.ret_addr = add_base_and_hole(&ctx, ctx.ret_addr);
>>>    	if (addr_over_limit(&ctx))
>>>    		return -EINVAL;
>>> diff --git a/drivers/ras/amd/atl/internal.h b/drivers/ras/amd/atl/internal.h
>>> index 1413c8ddc6c5..05b870fcb24e 100644
>>> --- a/drivers/ras/amd/atl/internal.h
>>> +++ b/drivers/ras/amd/atl/internal.h
>>> @@ -236,6 +236,9 @@ int dehash_address(struct addr_ctx *ctx);
>>>    unsigned long norm_to_sys_addr(u8 socket_id, u8 die_id, u8 coh_st_inst_id, unsigned long addr);
>>>    unsigned long convert_umc_mca_addr_to_sys_addr(struct atl_err *err);
>>> +u64 add_base_and_hole(struct addr_ctx *ctx, u64 addr);
>>> +u64 remove_base_and_hole(struct addr_ctx *ctx, u64 addr);
>>
>> remove_base_and_hole() is only used in denormalize.c, correct? So why
>> not define it there as static? Other than trying to keep the code
>> together and symmetrical, I mean.
> 
> In addition to keeping the two inverse functions together,
> remove_base_and_hole depends on other functions in core.c. So if we
> don't expose remove_base_and_hole in the header, then we would need to
> expose get_base_addr and remove_legacy_hole in the header.
> Alternatively, we could move remove_legacy_hole to denormalize.c and
> expose get_base_addr and legacy_hole_en in the header instead. So
> exposing one function that's the inverse of the other just looks better
> to me than exposing two.
> 

Right, fair point. So what you have is good then.

Thanks,
Yazen