RE: [PATCH 05/39] target/hexagon: Implement modify SSR

[<ltaylorsimpson@gmail.com>]

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Friday, February 28, 2025 11:28 PM
> To: qemu-devel@nongnu.org
> Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
> philmd@linaro.org; quic_mathbern@quicinc.com; ale@rev.ng; anjo@rev.ng;
> quic_mliebel@quicinc.com; ltaylorsimpson@gmail.com;
> alex.bennee@linaro.org; quic_mburton@quicinc.com;
> sidneym@quicinc.com; Brian Cain <bcain@quicinc.com>
> Subject: [PATCH 05/39] target/hexagon: Implement modify SSR
> 
> From: Brian Cain <bcain@quicinc.com>
> 
> The per-vCPU System Status Register controls many modal behaviors of the
> system architecture.  When the SSR is updated, we trigger the necessary
> effects for interrupts, privilege/MMU, and HVX context mapping.
> 
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>  target/hexagon/cpu_helper.c | 100
> +++++++++++++++++++++++++++++++++++-
>  1 file changed, 99 insertions(+), 1 deletion(-)
> 
> diff --git a/target/hexagon/cpu_helper.c b/target/hexagon/cpu_helper.c
> index e151c6335a..3e2364a7b0 100644
> --- a/target/hexagon/cpu_helper.c
> +++ b/target/hexagon/cpu_helper.c
> @@ -14,6 +14,8 @@
>  #else
>  #include "hw/boards.h"
>  #include "hw/hexagon/hexagon.h"
> +#include "hex_interrupts.h"
> +#include "hex_mmu.h"
>  #endif
>  #include "exec/exec-all.h"
>  #include "exec/cpu_ldst.h"
> @@ -69,9 +71,105 @@ void
> hexagon_set_sys_pcycle_count(CPUHexagonState *env, uint64_t cycles)
>      g_assert_not_reached();
>  }
> 
> +static MMVector VRegs[VECTOR_UNIT_MAX][NUM_VREGS];
> +static MMQReg QRegs[VECTOR_UNIT_MAX][NUM_QREGS];

This won't scale for a system with multiple Hexagon instances.  See discussion on how to handle shared resources.

> +
> +/*
> + *                            EXT_CONTEXTS
> + * SSR.XA   2              4              6              8
> + * 000      HVX Context 0  HVX Context 0  HVX Context 0  HVX Context 0
> + * 001      HVX Context 1  HVX Context 1  HVX Context 1  HVX Context 1
> + * 010      HVX Context 0  HVX Context 2  HVX Context 2  HVX Context 2
> + * 011      HVX Context 1  HVX Context 3  HVX Context 3  HVX Context 3
> + * 100      HVX Context 0  HVX Context 0  HVX Context 4  HVX Context 4
> + * 101      HVX Context 1  HVX Context 1  HVX Context 5  HVX Context 5
> + * 110      HVX Context 0  HVX Context 2  HVX Context 2  HVX Context 6
> + * 111      HVX Context 1  HVX Context 3  HVX Context 3  HVX Context 7
> + */

This is different from what the HVX PRM says.  It only specifies what XA values  4, 5, 6, 7 mean.

Here is what it says:
The HVX scalar core can contain any number of hardware threads greater or equal to the number
of vector contexts. The scalar hardware thread is assignable to a vector context through per-
thread SSR:XA programming, as follows:
- SSR:XA = 4: HVX instructions use vector context 0.
- SSR:XA = 5: HVX instructions use vector context 1, if it is available.
- SSR:XA = 6: HVX instructions use vector context 2, if it is available.
- SSR:XA = 7: HVX instructions use vector context 3, if it is available.

> +static int parse_context_idx(CPUHexagonState *env, uint8_t XA) {
> +    int ret;
> +    HexagonCPU *cpu = env_archcpu(env);

You should assert that cpu->hvx_contexts is in { 2, 4, 6, 8 }.  This will future proof against changes to the hardware as well as protect against bad command-line settings.

> +    if (cpu->hvx_contexts == 6 && XA >= 6) {
> +        ret = XA - 6 + 2;
> +    } else {
> +        ret = XA % cpu->hvx_contexts;
> +    }
> +    g_assert(ret >= 0 && ret < VECTOR_UNIT_MAX);
> +    return ret;
> +}
> +
> +static void check_overcommitted_hvx(CPUHexagonState *env, uint32_t
> ssr)
> +{
> +    if (!GET_FIELD(SSR_XE, ssr)) {
> +        return;
> +    }

What does SSR_XE indicate?

> +
> +    uint8_t XA = GET_SSR_FIELD(SSR_XA, ssr);
> +
> +    CPUState *cs;
> +    CPU_FOREACH(cs) {
> +        CPUHexagonState *env_ = cpu_env(cs);

This underscore is confusing.  Use a full name such as thread_env.

> +        if (env_ == env) {
> +            continue;
> +        }
> +        /* Check if another thread has the XE bit set and same XA */
> +        uint32_t ssr_ = arch_get_system_reg(env_, HEX_SREG_SSR);

Ditto

> +        if (GET_SSR_FIELD(SSR_XE2, ssr_) && GET_FIELD(SSR_XA, ssr_) == XA) {

The comment says check the XE bit but the code checks XE2.  Also, note the XE check on the current thread above.

> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                    "setting SSR.XA '%d' on thread %d but thread"
> +                    " %d has same extension active\n", XA, env->threadId,
> +                    env_->threadId);
> +        }
> +    }
> +}
> +
>  void hexagon_modify_ssr(CPUHexagonState *env, uint32_t new, uint32_t
> old)  {
> -    g_assert_not_reached();
> +    g_assert(bql_locked());
> +
> +    bool old_EX = GET_SSR_FIELD(SSR_EX, old);
> +    bool old_UM = GET_SSR_FIELD(SSR_UM, old);
> +    bool old_GM = GET_SSR_FIELD(SSR_GM, old);
> +    bool old_IE = GET_SSR_FIELD(SSR_IE, old);
> +    uint8_t old_XA = GET_SSR_FIELD(SSR_XA, old);
> +    bool new_EX = GET_SSR_FIELD(SSR_EX, new);
> +    bool new_UM = GET_SSR_FIELD(SSR_UM, new);
> +    bool new_GM = GET_SSR_FIELD(SSR_GM, new);
> +    bool new_IE = GET_SSR_FIELD(SSR_IE, new);
> +    uint8_t new_XA = GET_SSR_FIELD(SSR_XA, new);
> +
> +    if ((old_EX != new_EX) ||
> +        (old_UM != new_UM) ||
> +        (old_GM != new_GM)) {
> +        hex_mmu_mode_change(env);
> +    }
> +
> +    uint8_t old_asid = GET_SSR_FIELD(SSR_ASID, old);
> +    uint8_t new_asid = GET_SSR_FIELD(SSR_ASID, new);
> +    if (new_asid != old_asid) {
> +        CPUState *cs = env_cpu(env);
> +        tlb_flush(cs);
> +    }
> +
> +    if (old_XA != new_XA) {
> +        int old_unit = parse_context_idx(env, old_XA);
> +        int new_unit = parse_context_idx(env, new_XA);

Check that old_unit != new_unit.  Per the table above, different XA values can point to the same unit.  For example, if cpu->hvx_contexts is 2, the XA=0 and XA=2 both point to context 0.

> +
> +        /* Ownership exchange */
> +        memcpy(VRegs[old_unit], env->VRegs, sizeof(env->VRegs));
> +        memcpy(QRegs[old_unit], env->QRegs, sizeof(env->QRegs));
> +        memcpy(env->VRegs, VRegs[new_unit], sizeof(env->VRegs));
> +        memcpy(env->QRegs, QRegs[new_unit], sizeof(env->QRegs));

What does the hardware do?  Does it clear the context, or is that the OS'es job?

If the hardware leaves the context alone, the above should be
1) Copy env->{VQ}Regs to old_unit
2) Copy new_unit to env->{VQ}Regs

Should you check SSR_EX before doing these copies?

Do you need to do anything when SSR_EX changes?

> +
> +        check_overcommitted_hvx(env, new);
> +    }
> +
> +    /* See if the interrupts have been enabled or we have exited EX mode */
> +    if ((new_IE && !old_IE) ||
> +        (!new_EX && old_EX)) {
> +        hex_interrupt_update(env);
> +    }
>  }
> 
>  void clear_wait_mode(CPUHexagonState *env)
> --
> 2.34.1