Messages in this thread | | | Date | Tue, 12 Dec 2023 09:33:44 -0500 | Subject | Re: [PATCH v3 1/3] RAS: Introduce AMD Address Translation Library | From | Yazen Ghannam <> |
| |
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
| |