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

[<ltaylorsimpson@gmail.com>]

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Tuesday, March 18, 2025 6:47 PM
> To: ltaylorsimpson@gmail.com; 'Sid Manning' <sidneym@quicinc.com>;
> qemu-devel@nongnu.org
> Cc: richard.henderson@linaro.org; philmd@linaro.org; 'Matheus Bernardino
> (QUIC)' <quic_mathbern@quicinc.com>; ale@rev.ng; anjo@rev.ng; 'Marco
> Liebel (QUIC)' <quic_mliebel@quicinc.com>; alex.bennee@linaro.org; 'Mark
> Burton (QUIC)' <quic_mburton@quicinc.com>; 'Brian Cain'
> <bcain@quicinc.com>
> Subject: Re: [PATCH 05/39] target/hexagon: Implement modify SSR
> 
> 
> On 3/18/2025 2:14 PM, ltaylorsimpson@gmail.com wrote:
> >
> >> -----Original Message-----
> >> From: Sid Manning <sidneym@quicinc.com>
> >> Sent: Tuesday, March 18, 2025 1:34 PM
> >> To: ltaylorsimpson@gmail.com; 'Brian Cain'
> >> <brian.cain@oss.qualcomm.com>; qemu-devel@nongnu.org
> >> Cc: richard.henderson@linaro.org; philmd@linaro.org; Matheus
> >> Bernardino
> >> (QUIC) <quic_mathbern@quicinc.com>; ale@rev.ng; anjo@rev.ng; Marco
> >> Liebel (QUIC) <quic_mliebel@quicinc.com>; alex.bennee@linaro.org;
> >> Mark Burton (QUIC) <quic_mburton@quicinc.com>; Brian Cain
> >> <bcain@quicinc.com>
> >> Subject: RE: [PATCH 05/39] target/hexagon: Implement modify SSR
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: ltaylorsimpson@gmail.com <ltaylorsimpson@gmail.com>
> >>> Sent: Monday, March 17, 2025 12:37 PM
> >>> To: 'Brian Cain' <brian.cain@oss.qualcomm.com>; qemu-
> >> devel@nongnu.org
> >>> Cc: richard.henderson@linaro.org; philmd@linaro.org; Matheus
> >>> Bernardino
> >>> (QUIC) <quic_mathbern@quicinc.com>; ale@rev.ng; anjo@rev.ng;
> Marco
> >>> Liebel (QUIC) <quic_mliebel@quicinc.com>; alex.bennee@linaro.org;
> >>> Mark Burton (QUIC) <quic_mburton@quicinc.com>; Sid Manning
> >>> <sidneym@quicinc.com>; Brian Cain <bcain@quicinc.com>
> >>> Subject: RE: [PATCH 05/39] target/hexagon: Implement modify SSR
> >>>
> >>>> -----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>
> >>>> ---
> >>> What does SSR_XE indicate?
> >> [Sid Manning]
> >> If SSR:XE isn't set this thread doesn't have the coproc enabled so
> >> discard additional checking.  Any coproc insn issued when ssr:xe is 0
> >> will trigger a, "Illegal execution of coprocessor instruction." error.
> >
> >
> >>>> +    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?
> >> Nothing would keep a single htid from taking hvx unit 0, doing some hvx-
> ops
> >> , swapping to hvx unit 1 doing some more hvx-ops and so on.   We are
> doing
> >> this because each thread has a private copy of the hvx register
> >> state.  Since HVX units and threads are independent if one thread
> >> changes its ownership from HVX context 0 to HVX context 1 we have to
> >> do this copy.  Instead of memcpy should create a new object that
> >> represents the HVX units available and change env->VRegs/QRegs to
> point to the one currently owned.
> >>
> >> Refactoring this will be an improvement.
> >>
> >>
> >>> 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?
> >> I think you mean SSR:XE before doing the copies.  I think we have to
> >> do the copy here regardless of ssr:xe since a thread could swap
> >> ownership, update ssr:xa, without explicitly having ssr:xe set.
> >> Maybe the OS disables SSR:XE while changing hvx unit ownership?
> > Correct.  I meant SSR:XE.
> >
> > Some refactoring is in order but need to understand the HW behavior more
> specifically.
> > - What will the HW do if more than one thread claims ownership of the
> same HVX context?
> > - What happens if a thread claims ownership without setting SSR:XE and
> then sets SSR:XE later?
> > - What about this example?
> >      1) Thread 0 claims context 1 and sets SSR:XE
> >      2) Thread 0 does some HVX computation
> >      3) Thread 0 is done with HVX for now so clears SSR:XE
> >      4) Thread 1 claims context 1 and sets SSR:XE, does some work, then
> clears SSR:XE
> >      5) Thread 0 wants to do more HVX, so it sets SSR:XE (still
> > pointing to HVX context 1)
> >
> > We should keep the copies for the contexts and local copies inside
> CPUHexagonState.  This makes TCG generation easier as well as having
> common code between system mode and linux-user mode.
> 
> Good point that linux-user will need their own exclusive HVX context. But it
> doesn't sound right to me to have CPU state store both the system contexts
> *and* a local context for system emulation.  Our current design under
> review is better than that IMO.  Once we have some experience modeling
> the system registers as an independent QEMU Object, I think we'll be better
> prepared to model the HVX contexts similarly.  Ideally we can remap these
> via something along the lines of "object_property_set_link()" when the
> contexts change, without requiring any copies.  And ideally the existing TCG
> should work as-is, despite the remappable register files.
> 
> "What happens when ... " - multiple threads picking the same context is
> almost certainly explicitly or implicitly undefined architectural behavior, so
> whatever QEMU does should be appropriate.  However, we'll talk to the
> architecture team and get a definitive answer.

I caution against putting a level of indirection into CPUHexagonState for the HVX registers.  The HVX TCG implementation relies on an offset from the start of CPUHexagonState to identify the operands.  This would be a very significant rewrite if it's even possible.  I don't know if TCG's gvec code generation can handle random pointers or a level of indirection.

If the behavior is undefined, we can avoid the copies.  Then we just need some bookkeeping to check if multiple threads try to claim the same context (if that behavior is defined).  If copies are needed, we could keep the  hardware contexts as shared a shared resource.  Another alternative would be to track the current owner of a context and copy from the previous owner's {VQ}Regs to the new owners {VQ}Regs.

Thanks,
Taylor