From: Yazen Ghannam <yazen.ghannam@amd.com>
To: Borislav Petkov <bp@alien8.de>
Cc: yazen.ghannam@amd.com, linux-edac@vger.kernel.org,
linux-kernel@vger.kernel.org, tony.luck@intel.com,
x86@kernel.org, avadhut.naik@amd.com, john.allen@amd.com,
william.roche@oracle.com, muralidhara.mk@amd.com
Subject: Re: [PATCH v3 1/3] RAS: Introduce AMD Address Translation Library
Date: Tue, 12 Dec 2023 09:33:44 -0500 [thread overview]
Message-ID: <a3305a97-4936-4cfd-b706-10b7792acdad@amd.com> (raw)
In-Reply-To: <20231212132907.GJZXhgIyss9eT1MsNb@fat_crate.local>
On 12/12/2023 8:29 AM, Borislav Petkov wrote:
> On Sun, Dec 10, 2023 at 01:49:30PM -0600, Yazen Ghannam wrote:
>> diff --git a/drivers/ras/amd/atl/dehash.c b/drivers/ras/amd/atl/dehash.c
>> new file mode 100644
>> index 000000000000..84fe9793694e
>> --- /dev/null
>> +++ b/drivers/ras/amd/atl/dehash.c
>> @@ -0,0 +1,446 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * AMD Address Translation Library
>> + *
>> + * dehash.c : Functions to account for hashing bits
>> + *
>> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
>> + * All Rights Reserved.
>> + *
>> + * Author: Yazen Ghannam <Yazen.Ghannam@amd.com>
>> + */
>> +
>> +#include "internal.h"
>> +
>> +static inline bool assert_intlv_bit(struct addr_ctx *ctx, u8 bit1, u8 bit2)
>> +{
>> + if (ctx->map.intlv_bit_pos == bit1 || ctx->map.intlv_bit_pos == bit2)
>> + return false;
>> +
>> + warn_on_assert("%s: Invalid interleave bit: %u",
>> + __func__, ctx->map.intlv_bit_pos);
>> +
>> + return true;
>> +}
>> +
>> +static inline bool assert_num_intlv_dies(struct addr_ctx *ctx, u8 num_intlv_dies)
>> +{
>> + if (ctx->map.num_intlv_dies <= num_intlv_dies)
>> + return false;
>> +
>> + warn_on_assert("%s: Invalid number of interleave dies: %u",
>> + __func__, ctx->map.num_intlv_dies);
>> +
>> + return true;
>> +}
>> +
>> +static inline bool assert_num_intlv_sockets(struct addr_ctx *ctx, u8 num_intlv_sockets)
>> +{
>> + if (ctx->map.num_intlv_sockets <= num_intlv_sockets)
>> + return false;
>> +
>> + warn_on_assert("%s: Invalid number of interleave sockets: %u",
>> + __func__, ctx->map.num_intlv_sockets);
>> +
>> + return true;
>> +}
>> +
>> +static int df2_dehash_addr(struct addr_ctx *ctx)
>> +{
>> + u8 hashed_bit, intlv_bit, intlv_bit_pos;
>> +
>> + /* Assert that interleave bit is 8 or 9. */
>> + if (assert_intlv_bit(ctx, 8, 9))
>> + return -EINVAL;
>
> You don't need those homegrown assertions. Instead, you do this:
>
> diff --git a/drivers/ras/amd/atl/dehash.c b/drivers/ras/amd/atl/dehash.c
> index 84fe9793694e..11634001702e 100644
> --- a/drivers/ras/amd/atl/dehash.c
> +++ b/drivers/ras/amd/atl/dehash.c
> @@ -47,10 +47,12 @@ static inline bool assert_num_intlv_sockets(struct addr_ctx *ctx, u8 num_intlv_s
>
> static int df2_dehash_addr(struct addr_ctx *ctx)
> {
> - u8 hashed_bit, intlv_bit, intlv_bit_pos;
> + u8 hashed_bit, intlv_bit;
> + u8 intlv_bit_pos = ctx->map.intlv_bit_pos;
>
> /* Assert that interleave bit is 8 or 9. */
> - if (assert_intlv_bit(ctx, 8, 9))
> + if (WARN(intlv_bit_pos != 8 && intlv_bit_pos != 9,
> + "Invalid interleave bit: %u\n", intlv_bit_pos))
> return -EINVAL;
>
> /* Assert that die and socket interleaving are disabled. */
> @@ -60,7 +62,6 @@ static int df2_dehash_addr(struct addr_ctx *ctx)
> if (assert_num_intlv_sockets(ctx, 1))
> return -EINVAL;
>
> - intlv_bit_pos = ctx->map.intlv_bit_pos;
> intlv_bit = atl_get_bit(intlv_bit_pos, ctx->ret_addr);
>
> hashed_bit = intlv_bit;
>
> and so on for the other two.
That's mostly how it was in the previous revision. Should I go back to
that then?
>
>> + /* Assert that die and socket interleaving are disabled. */
>> + if (assert_num_intlv_dies(ctx, 1))
>> + return -EINVAL;
>> +
>> + if (assert_num_intlv_sockets(ctx, 1))
>> + return -EINVAL;
>> +
>> + intlv_bit_pos = ctx->map.intlv_bit_pos;
>> + intlv_bit = atl_get_bit(intlv_bit_pos, ctx->ret_addr);
>
> Can we keep it simple please?
>
> intlv_bit = !!(BIT_ULL(intlv_bit_pos) & ctx->ret_addr);
>
> That atl_get_bit() is not necessary.
Okay, will change.
>
>> + hashed_bit = intlv_bit;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(12), ctx->ret_addr);
>> + hashed_bit ^= FIELD_GET(BIT_ULL(18), ctx->ret_addr);
>> + hashed_bit ^= FIELD_GET(BIT_ULL(21), ctx->ret_addr);
>> + hashed_bit ^= FIELD_GET(BIT_ULL(30), ctx->ret_addr);
>> +
>> + if (hashed_bit != intlv_bit)
>> + ctx->ret_addr ^= BIT_ULL(intlv_bit_pos);
>> +
>> + return 0;
>> +}
>
> <---
>
Ack
>> +static int df3_dehash_addr(struct addr_ctx *ctx)
>> +{
>> + bool hash_ctl_64k, hash_ctl_2M, hash_ctl_1G;
>> + u8 hashed_bit, intlv_bit, intlv_bit_pos;
>> +
>> + /* Assert that interleave bit is 8 or 9. */
>> + if (assert_intlv_bit(ctx, 8, 9))
>> + return -EINVAL;
>> +
>> + /* Assert that die and socket interleaving are disabled. */
>> + if (assert_num_intlv_dies(ctx, 1))
>> + return -EINVAL;
>> +
>> + if (assert_num_intlv_sockets(ctx, 1))
>> + return -EINVAL;
>
> Those assertions keep repeating. Extract them into a separate function
> which you call from every *dehash_addr function?
>
Okay, can do. This can drop the assert_*() helpers like the comments above.
>> + hash_ctl_64k = FIELD_GET(DF3_HASH_CTL_64K, ctx->map.ctl);
>> + hash_ctl_2M = FIELD_GET(DF3_HASH_CTL_2M, ctx->map.ctl);
>> + hash_ctl_1G = FIELD_GET(DF3_HASH_CTL_1G, ctx->map.ctl);
>
> I believe without the tabs looks good too:
>
> hash_ctl_64k = FIELD_GET(DF3_HASH_CTL_64K, ctx->map.ctl);
> hash_ctl_2M = FIELD_GET(DF3_HASH_CTL_2M, ctx->map.ctl);
> hash_ctl_1G = FIELD_GET(DF3_HASH_CTL_1G, ctx->map.ctl);
>
Okay.
>> + intlv_bit_pos = ctx->map.intlv_bit_pos;
>> + intlv_bit = atl_get_bit(intlv_bit_pos, ctx->ret_addr);
>> +
>> + hashed_bit = intlv_bit;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(14), ctx->ret_addr);
>> + hashed_bit ^= FIELD_GET(BIT_ULL(18), ctx->ret_addr) & hash_ctl_64k;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(23), ctx->ret_addr) & hash_ctl_2M;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(32), ctx->ret_addr) & hash_ctl_1G;
>> +
>> + if (hashed_bit != intlv_bit)
>> + ctx->ret_addr ^= BIT_ULL(intlv_bit_pos);
>> +
>> + /* Calculation complete for 2 channels. Continue for 4 and 8 channels. */
>> + if (ctx->map.intlv_mode == DF3_COD4_2CHAN_HASH)
>> + return 0;
>> +
>> + intlv_bit = FIELD_GET(BIT_ULL(12), ctx->ret_addr);
>> +
>> + hashed_bit = intlv_bit;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(16), ctx->ret_addr) & hash_ctl_64k;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(21), ctx->ret_addr) & hash_ctl_2M;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(30), ctx->ret_addr) & hash_ctl_1G;
>> +
>> + if (hashed_bit != intlv_bit)
>> + ctx->ret_addr ^= BIT_ULL(12);
>> +
>> + /* Calculation complete for 4 channels. Continue for 8 channels. */
>> + if (ctx->map.intlv_mode == DF3_COD2_4CHAN_HASH)
>> + return 0;
>> +
>> + intlv_bit = FIELD_GET(BIT_ULL(13), ctx->ret_addr);
>> +
>> + hashed_bit = intlv_bit;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(17), ctx->ret_addr) & hash_ctl_64k;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(22), ctx->ret_addr) & hash_ctl_2M;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(31), ctx->ret_addr) & hash_ctl_1G;
>> +
>> + if (hashed_bit != intlv_bit)
>> + ctx->ret_addr ^= BIT_ULL(13);
>> +
>> + return 0;
>> +}
>
> Also, same comments about this function as for df2_dehash_addr(). Below
> too.
>
Okay.
>> +
>> +static int df3_6chan_dehash_addr(struct addr_ctx *ctx)
>> +{
>> + u8 intlv_bit_pos = ctx->map.intlv_bit_pos;
>> + u8 hashed_bit, intlv_bit, num_intlv_bits;
>> + bool hash_ctl_2M, hash_ctl_1G;
>> +
>> + if (ctx->map.intlv_mode != DF3_6CHAN) {
>> + warn_on_bad_intlv_mode(ctx);
>> + return -EINVAL;
>> + }
>> +
>> + num_intlv_bits = ilog2(ctx->map.num_intlv_chan) + 1;
>> +
>> + hash_ctl_2M = FIELD_GET(DF3_HASH_CTL_2M, ctx->map.ctl);
>> + hash_ctl_1G = FIELD_GET(DF3_HASH_CTL_1G, ctx->map.ctl);
>> +
>> + intlv_bit = atl_get_bit(intlv_bit_pos, ctx->ret_addr);
>> +
>> + hashed_bit = intlv_bit;
>> + hashed_bit ^= atl_get_bit((intlv_bit_pos + num_intlv_bits), ctx->ret_addr);
>> + hashed_bit ^= FIELD_GET(BIT_ULL(23), ctx->ret_addr) & hash_ctl_2M;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(32), ctx->ret_addr) & hash_ctl_1G;
>> +
>> + if (hashed_bit != intlv_bit)
>> + ctx->ret_addr ^= BIT_ULL(intlv_bit_pos);
>> +
>> + intlv_bit_pos++;
>> + intlv_bit = atl_get_bit(intlv_bit_pos, ctx->ret_addr);
>> +
>> + hashed_bit = intlv_bit;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(21), ctx->ret_addr) & hash_ctl_2M;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(30), ctx->ret_addr) & hash_ctl_1G;
>> +
>> + if (hashed_bit != intlv_bit)
>> + ctx->ret_addr ^= BIT_ULL(intlv_bit_pos);
>> +
>> + intlv_bit_pos++;
>> + intlv_bit = atl_get_bit(intlv_bit_pos, ctx->ret_addr);
>> +
>> + hashed_bit = intlv_bit;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(22), ctx->ret_addr) & hash_ctl_2M;
>> + hashed_bit ^= FIELD_GET(BIT_ULL(31), ctx->ret_addr) & hash_ctl_1G;
>> +
>> + if (hashed_bit != intlv_bit)
>> + ctx->ret_addr ^= BIT_ULL(intlv_bit_pos);
>> +
>> + return 0;
>> +}
>
> ...
>
Thanks,
Yazen
next prev parent reply other threads:[~2023-12-12 14:33 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-10 19:49 [PATCH v3 0/3] AMD Address Translation Library Yazen Ghannam
2023-12-10 19:49 ` [PATCH v3 1/3] RAS: Introduce " Yazen Ghannam
2023-12-11 14:20 ` Borislav Petkov
2023-12-11 15:28 ` Yazen Ghannam
2023-12-11 19:57 ` Borislav Petkov
2023-12-12 14:23 ` Yazen Ghannam
2023-12-12 15:34 ` Borislav Petkov
2023-12-13 15:35 ` Yazen Ghannam
2023-12-13 16:48 ` Borislav Petkov
2023-12-13 17:04 ` Yazen Ghannam
2023-12-13 17:07 ` Borislav Petkov
2023-12-12 13:29 ` Borislav Petkov
2023-12-12 14:33 ` Yazen Ghannam [this message]
2023-12-12 16:07 ` Borislav Petkov
2023-12-14 10:54 ` Borislav Petkov
2023-12-14 14:30 ` Borislav Petkov
2023-12-10 19:49 ` [PATCH v3 2/3] EDAC/amd64: Use new " Yazen Ghannam
2023-12-10 19:49 ` [PATCH v3 3/3] Documentation: RAS: Add index and address translation section Yazen Ghannam
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-all 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=a3305a97-4936-4cfd-b706-10b7792acdad@amd.com \
--to=yazen.ghannam@amd.com \
--cc=avadhut.naik@amd.com \
--cc=bp@alien8.de \
--cc=john.allen@amd.com \
--cc=linux-edac@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=muralidhara.mk@amd.com \
--cc=tony.luck@intel.com \
--cc=william.roche@oracle.com \
--cc=x86@kernel.org \
/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