Re: [PATCH 28/38] target/hexagon: Initialize htid, modectl regs

[Brian Cain <brian.cain@oss.qualcomm.com>]

On 3/20/2025 12:38 PM, Sid Manning wrote:
>
>> -----Original Message-----
>> From: Richard Henderson <richard.henderson@linaro.org>
>> Sent: Thursday, March 20, 2025 10:34 AM
>> To: Sid Manning <sidneym@quicinc.com>; ltaylorsimpson@gmail.com;
>> 'Philippe Mathieu-Daudé' <philmd@linaro.org>; 'Brian Cain'
>> <brian.cain@oss.qualcomm.com>; qemu-devel@nongnu.org
>> Cc: 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 28/38] target/hexagon: Initialize htid, modectl regs
>>
>> WARNING: This email originated from outside of Qualcomm. Please be wary
>> of any links or attachments, and do not enable macros.
>>
>> On 3/19/25 09:08, Sid Manning wrote:
>>>
>>>> -----Original Message-----
>>>> From: Richard Henderson <richard.henderson@linaro.org>
>>>> Sent: Thursday, March 13, 2025 2:07 PM
>>>> To: ltaylorsimpson@gmail.com; 'Philippe Mathieu-Daudé'
>>>> <philmd@linaro.org>; 'Brian Cain' <brian.cain@oss.qualcomm.com>;
>>>> qemu- devel@nongnu.org
>>>> Cc: 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 28/38] target/hexagon: Initialize htid, modectl
>>>> regs
>>>>
>>>> WARNING: This email originated from outside of Qualcomm. Please be
>>>> wary of any links or attachments, and do not enable macros.
>>>>
>>>> On 3/13/25 11:47, ltaylorsimpson@gmail.com wrote:
>>>>> What we are trying to model is an instance of a Hexagon that has a
>>>>> number
>>>> of threads and some resources that are shared.  The shared resources
>>>> include the TLB and global S registers.  The initial thought was to
>>>> tie the shared resources to the thread with cpu_index == 0.  If we
>>>> were to model a Qualcomm SoC, there would be multiple ARM cores and
>>>> multiple Hexagon instances.  Each Hexagon instance would have
>>>> distinct shared resources.  So, you are correct that using cpu_index is not
>> going to scale.
>>>>> What is the recommended way to model this?  I see a "nr_threads"
>>>>> field in
>>>> CPUCore but no clear way to find the threads.  PPC has some cores
>>>> that add a "threads" field.  Should we follow this approach?
>>>>
>>>> I recommend that the shared resources be modeled as a separate
>>>> Object, which is linked via object_property_add_link to all of the cpus that
>> use it.
>>> [Sid Manning]
>>> Hi Richard,
>>> An example of shared resources would be the system registers.  They are
>> broken down into 2 regions.  Each thread has its own copy of system
>> registers 0-15 while registers 16-63 are global.  Right now CPUHexagonState
>> contains a pointer, g_sreg which points back to cpu[0]'s state thus keeping
>> one copy of the global registers, accesses are done with BQL held to avoid
>> race conditions.
>>> Your suggestion is to create a new object to represent the set of global
>> system registers, I tried this:
>>> #define TYPE_HEXAGON_G_SREG "hexagon.global_sreg"
>>> OBJECT_DECLARE_SIMPLE_TYPE(HexagonGlobalSREGState,
>> HEXAGON_G_SREG)
>>> struct HexagonGlobalSREGState {
>>>       SysBusDevice parent_obj;
>> SysBusDevice is more than you need -- Object is sufficient here.
> [Sid Manning]
> Thanks!  Will change that to Object.
>
>>>       uint32_t regs[64];
>>> };
>>>
>>> In our virtual machine init:
>>> vms->g_sreg =
>> HEXAGON_G_SREG(qdev_new(TYPE_HEXAGON_G_SREG));
>>> and
>>>    object_property_set_link(OBJECT(cpu), "global-sreg",
>>> OBJECT(vms->g_sreg), &error_abort);
>>>
>>> to attach the global regs to the cpu, but the above doesn't update cpu
>> elements the same way calls to qdev_prop_set_uint32 will do,
>> object_property_set_link doesn’t error out and returns true.
>>
>> Did you add the DEFINE_PROP_LINK to match?  I'd expect something like
>>
>>       DEFINE_PROP_LINK("global-sreg", HexagonCPU, g_sreg,
>>                        TYPE_HEXAGON_G_SREG, HexagonGlobalSREGState *),
>>
>> Beyond that, I guess I'd have to see an actual patch to work out what's
>> wrong.
> [Sid Manning]
> Yes, PROP_LINK above is almost exactly what I added.
>
> Below is a patch representing what I tried.  I hoped that cpu->global_sreg would be updated after the call to object_property_set_link but it was not, in the patch below I manually set it.
>
> diff --git a/hw/hexagon/sysreg.h b/hw/hexagon/sysreg.h
> new file mode 100644
> index 0000000000..d7204896cf
> --- /dev/null
> +++ b/hw/hexagon/sysreg.h
> @@ -0,0 +1,47 @@
> +/*
> + * Hexagon system reg
> + * FIXME
> + */
> +
> +#ifndef HW_HEXAGON_HART_H
> +#define HW_HEXAGON_HART_H
> +#if !defined(CONFIG_USER_ONLY)
> +#include "hw/sysbus.h"
> +#include "qom/object.h"
> +
> +#define NUM_SREGS 64
> +struct HexagonGlobalSREGState {
> +    struct Object parent_obj;
> +    uint32_t regs[NUM_SREGS];
> +};
> +
> +#define TYPE_HEXAGON_G_SREG "hexagon.global_sreg"
> +OBJECT_DECLARE_SIMPLE_TYPE(HexagonGlobalSREGState, HEXAGON_G_SREG)
> +
> +static void hexagon_global_sreg_init(Object *obj)
> +{
> +    HexagonGlobalSREGState *s = HEXAGON_G_SREG(obj);
> +    /*
> +     * The first 16 registers are thread local and should not come from
> +     * this structure
> +     */
> +    for (int i = 0; i < 16; i++) {
> +        s->regs[i] = 0xffffffff;
> +    }
> +}
> +
> +static const TypeInfo hexagon_sreg_info = {
> +    .name          = TYPE_HEXAGON_G_SREG,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(struct HexagonGlobalSREGState),
> +    .instance_init = hexagon_global_sreg_init,
> +};
> +
> +__attribute__ ((unused))
> +static void hexagon_sreg_register_types(void)
> +{
> +    type_register_static(&hexagon_sreg_info);
> +}
> +#endif
> +#endif
> +
> diff --git a/hw/hexagon/virt.c b/hw/hexagon/virt.c
> index 1e7ac4e5b7..d2d599ac1d 100644
> --- a/hw/hexagon/virt.c
> +++ b/hw/hexagon/virt.c
> @@ -10,12 +10,14 @@
>   #include "hw/char/pl011.h"
>   #include "hw/core/sysbus-fdt.h"
>   #include "hw/hexagon/hexagon.h"
> +#include "hw/hexagon/sysreg.h"
>   #include "hw/hexagon/virt.h"
>   #include "hw/loader.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/register.h"
>   #include "hw/timer/qct-qtimer.h"
>   #include "qemu/error-report.h"
> +#include "qapi/error.h"
>   #include "qemu/guest-random.h"
>   #include "qemu/units.h"
>   #include "elf.h"
> @@ -335,6 +337,7 @@ static void virt_init(MachineState *ms)
>           cpu_model = HEXAGON_CPU_TYPE_NAME("v73");
>       }
>   
> +    vms->g_sreg = HEXAGON_G_SREG(qdev_new(TYPE_HEXAGON_G_SREG));
>       HexagonCPU *cpu_0 = NULL;
>       for (int i = 0; i < ms->smp.cpus; i++) {
>           HexagonCPU *cpu = HEXAGON_CPU(object_new(ms->cpu_type));
> @@ -356,6 +359,14 @@ static void virt_init(MachineState *ms)
>           qdev_prop_set_uint32(DEVICE(cpu), "qtimer-base-addr", m_cfg->qtmr_region);
>           qdev_prop_set_uint32(DEVICE(cpu), "jtlb-entries",
>                                m_cfg->cfgtable.jtlb_size_entries);
> +        bool rc = object_property_set_link(OBJECT(cpu), "global-sreg",
> +                                           OBJECT(vms->g_sreg), &error_abort);
> +        g_assert(rc == true);
> +
> +        /* This is doing what I think object_property_set_link should do.*/
> +        cpu->global_sreg = vms->g_sreg;
> +
> +
>   
>           if (!qdev_realize_and_unref(DEVICE(cpu), NULL, errp)) {
>               return;
> @@ -413,3 +424,5 @@ static const TypeInfo virt_machine_types[] = { {
>   } };
>   
>   DEFINE_TYPES(virt_machine_types)
> +
> +type_init(hexagon_sreg_register_types)
> diff --git a/include/hw/hexagon/virt.h b/include/hw/hexagon/virt.h
> index 0c165a786d..dcd09d50b1 100644
> --- a/include/hw/hexagon/virt.h
> +++ b/include/hw/hexagon/virt.h
> @@ -9,6 +9,7 @@
>   #define HW_HEXAGONVIRT_H
>   
>   #include "hw/boards.h"
> +#include "hw/hexagon/sysreg.h"
>   #include "target/hexagon/cpu.h"
>   
>   struct HexagonVirtMachineState {
> @@ -22,6 +23,7 @@ struct HexagonVirtMachineState {
>       MemoryRegion tcm;
>       MemoryRegion vtcm;
>       DeviceState *l2vic;
> +    HexagonGlobalSREGState *g_sreg;
>   };
>   
>   void hexagon_load_fdt(const struct HexagonVirtMachineState *vms);
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
> index c649aef99e..9773ee0be8 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -80,6 +80,8 @@ static const Property hexagon_cpu_properties[] = {
>       DEFINE_PROP_UINT32("exec-start-addr", HexagonCPU, boot_addr, 0xffffffffULL),
>       DEFINE_PROP_UINT64("config-table-addr", HexagonCPU, config_table_addr,
>                          0xffffffffULL),
> +    DEFINE_PROP_LINK("global-sreg", HexagonCPU, global_sreg,
> +                     TYPE_HEXAGON_G_SREG, HexagonGlobalSREGState *),
>   #endif
>       DEFINE_PROP_UINT32("dsp-rev", HexagonCPU, rev_reg, 0),
>       DEFINE_PROP_BOOL("lldb-compat", HexagonCPU, lldb_compat, false),
> @@ -378,6 +380,11 @@ static void hexagon_cpu_reset_hold(Object *obj, ResetType type)
>       CPUState *cs = CPU(obj);
>       HexagonCPUClass *mcc = HEXAGON_CPU_GET_CLASS(obj);
>       CPUHexagonState *env = cpu_env(cs);
> +#ifndef CONFIG_USER_ONLY
> +    HexagonCPU *cpu = HEXAGON_CPU(cs);
> +    env->g_sreg = cpu->global_sreg->regs;
> +#endif
> +
>   
>       if (mcc->parent_phases.hold) {
>           mcc->parent_phases.hold(obj, type);
> @@ -389,11 +396,6 @@ static void hexagon_cpu_reset_hold(Object *obj, ResetType type)
>       set_float_default_nan_pattern(0b11111111, &env->fp_status);
>   
>   #ifndef CONFIG_USER_ONLY
> -    HexagonCPU *cpu = HEXAGON_CPU(cs);
> -
> -    if (cs->cpu_index == 0) {
> -        memset(env->g_sreg, 0, sizeof(target_ulong) * NUM_SREGS);
> -    }
>       memset(env->t_sreg, 0, sizeof(target_ulong) * NUM_SREGS);
>       memset(env->greg, 0, sizeof(target_ulong) * NUM_GREGS);
>   
> @@ -468,13 +470,6 @@ static void hexagon_cpu_realize(DeviceState *dev, Error **errp)
>       CPUHexagonState *env = cpu_env(cs);
>   #ifndef CONFIG_USER_ONLY
>       hex_mmu_realize(env);
> -    if (cs->cpu_index == 0) {
> -        env->g_sreg = g_new0(target_ulong, NUM_SREGS);
> -    } else {
> -        CPUState *cpu0 = qemu_get_cpu(0);
> -        CPUHexagonState *env0 = cpu_env(cpu0);
> -        env->g_sreg = env0->g_sreg;
> -    }
>   #endif
>       if (cs->cpu_index == 0) {
>           env->g_pcycle_base = g_malloc0(sizeof(*env->g_pcycle_base));
> diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h
> index 8b334068e2..716dd8253b 100644
> --- a/target/hexagon/cpu.h
> +++ b/target/hexagon/cpu.h
> @@ -19,10 +19,10 @@
>   #define HEXAGON_CPU_H
>   
>   #include "fpu/softfloat-types.h"
> +#include "hw/hexagon/sysreg.h"
>   
>   #define NUM_GREGS 32
>   #define GREG_WRITES_MAX 32
> -#define NUM_SREGS 64
>   #define SREG_WRITES_MAX 64
>   
>   #include "cpu-qom.h"
> @@ -199,6 +199,7 @@ struct ArchCPU {
>       uint32_t hvx_contexts;
>       uint32_t boot_addr;
>       uint64_t config_table_addr;
> +    HexagonGlobalSREGState *global_sreg;
>   #endif
>   };
>
>

I think the re-design of the global registers present in v2 should be a 
satisfactory resolution to this thread but please let me know if that's 
not the case.

>