On Fri, 19 Jan 2024 at 20:47, Max Filippov <jcmvb...@gmail.com> wrote:
>
> Whether TLB ways 5 and 6 are variable is not a property of the TLB
> instance or a TLB entry instance, it's a property of the xtensa core
> configuration.
> Remove 'varway56' field from the xtensa_tlb structure and remove
> 'variable' field from the xtensa_tlb_entry structure. Add
> 'tlb_variable_way' array to the XtensaConfig and use it instead of
> removed fields.
>
> Signed-off-by: Max Filippov <jcmvb...@gmail.com>
> ---
> target/xtensa/cpu.h | 3 +--
> target/xtensa/mmu_helper.c | 38 ++++++++++--------------------------
> target/xtensa/overlay_tool.h | 15 ++++++++++++--
> 3 files changed, 24 insertions(+), 32 deletions(-)
>
> diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
> index 497325466397..24d3f15ea1bf 100644
> --- a/target/xtensa/cpu.h
> +++ b/target/xtensa/cpu.h
> @@ -316,13 +316,11 @@ typedef struct xtensa_tlb_entry {
> uint32_t paddr;
> uint8_t asid;
> uint8_t attr;
> - bool variable;
> } xtensa_tlb_entry;
>
> typedef struct xtensa_tlb {
> unsigned nways;
> const unsigned way_size[10];
> - bool varway56;
> unsigned nrefillentries;
> } xtensa_tlb;
>
> @@ -493,6 +491,7 @@ typedef struct XtensaConfig {
>
> xtensa_tlb itlb;
> xtensa_tlb dtlb;
> + bool tlb_variable_way[16];
>
> uint32_t mpu_align;
> unsigned n_mpu_fg_segments;
> diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c
> index d9f845e7fb6f..414c2f5ef669 100644
> --- a/target/xtensa/mmu_helper.c
> +++ b/target/xtensa/mmu_helper.c
> @@ -105,23 +105,19 @@ static uint32_t xtensa_tlb_get_addr_mask(const
> CPUXtensaState *env,
> bool dtlb, uint32_t way)
> {
> if (xtensa_option_enabled(env->config, XTENSA_OPTION_MMU)) {
> - bool varway56 = dtlb ?
> - env->config->dtlb.varway56 :
> - env->config->itlb.varway56;
> -
> switch (way) {
> case 4:
> return 0xfff00000 << get_page_size(env, dtlb, way) * 2;
>
> case 5:
> - if (varway56) {
> + if (env->config->tlb_variable_way[5]) {
> return 0xf8000000 << get_page_size(env, dtlb, way);
> } else {
> return 0xf8000000;
> }
>
> case 6:
> - if (varway56) {
> + if (env->config->tlb_variable_way[6]) {
> return 0xf0000000 << (1 - get_page_size(env, dtlb, way));
> } else {
> return 0xf0000000;
So we now have a tlb_variable_way bool for all 16 possible
ways, but the code actually only checks it for ways 5 and 6.
Should we have an assertion somewhere that the config
doesn't try to set it on ways where it has no effect ?
Or is there actually a generic behaviour that would make
sense for eg "way 3 is variable-way" that we just don't
currently implement?
> @@ -150,11 +146,8 @@ static uint32_t get_vpn_mask(const CPUXtensaState *env,
> bool dtlb, uint32_t way)
> return xtensa_tlb_get_addr_mask(env, dtlb, way) << 2;
> } else if (way <= 6) {
> uint32_t mask = xtensa_tlb_get_addr_mask(env, dtlb, way);
> - bool varway56 = dtlb ?
> - env->config->dtlb.varway56 :
> - env->config->itlb.varway56;
>
> - if (varway56) {
> + if (env->config->tlb_variable_way[5]) {
> return mask << (way == 5 ? 2 : 3);
> } else {
> return mask << 1;
This doesn't look right -- this branch of the if-else deals
with way == 5 and way == 6, but we're only looking at
tlb_variable_way[5].
> @@ -172,10 +165,6 @@ static void split_tlb_entry_spec_way(const
> CPUXtensaState *env, uint32_t v,
> bool dtlb, uint32_t *vpn,
> uint32_t wi, uint32_t *ei)
> {
> - bool varway56 = dtlb ?
> - env->config->dtlb.varway56 :
> - env->config->itlb.varway56;
> -
> if (!dtlb) {
> wi &= 7;
> }
> @@ -195,7 +184,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState
> *env, uint32_t v,
> break;
>
> case 5:
> - if (varway56) {
> + if (env->config->tlb_variable_way[5]) {
> uint32_t eibase = 27 + get_page_size(env, dtlb, wi);
> *ei = (v >> eibase) & 0x3;
> } else {
> @@ -204,7 +193,7 @@ static void split_tlb_entry_spec_way(const CPUXtensaState
> *env, uint32_t v,
> break;
>
> case 6:
> - if (varway56) {
> + if (env->config->tlb_variable_way[6]) {
> uint32_t eibase = 29 - get_page_size(env, dtlb, wi);
> *ei = (v >> eibase) & 0x7;
> } else {
There's no direct code duplication, but it definitely feels like
the logic for "figure out how many bits we're dealing with" is
duplicated across these three functions.
I think it ought to be possible to have a function (or maybe two)
which take account of both the way number and tlb_get_variable_way[]
such that all of these three functions then don't need to have
a switch on the way or look at tlb_variable_way[] themselves...
thanks
-- PMM
