Re: [PATCH 12/14] hw/hexagon: add subsystem-base property

[Anton Johansson via qemu development <qemu-devel@nongnu.org>]

On 19/10/26, Brian Cain wrote:
> Let the hexagon-sim-style machines their QDSP6 subsystem, and with it the
> l2vic and QTimer.
> 
> The config table's subsystem base is updated to align with the chosen
> address.
> 
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>  MAINTAINERS                       |  1 +
>  include/hw/hexagon/hexagon.h      |  4 ++
>  hw/hexagon/hexagon_dsp.c          | 62 +++++++++++++++++++++++-
>  tests/qtest/hexagon-config-test.c | 78 +++++++++++++++++++++++++++++++
>  tests/qtest/meson.build           |  3 +-
>  5 files changed, 146 insertions(+), 2 deletions(-)
>  create mode 100644 tests/qtest/hexagon-config-test.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 04df9db77dc..f1fbc971fed 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1386,6 +1386,7 @@ F: include/hw/misc/qcom-qdsp6ss-clk.h
>  F: configs/devices/hexagon-softmmu/default.mak
>  F: docs/system/hexagon/
>  F: tests/functional/hexagon/
> +F: tests/qtest/hexagon-config-test.c
>  F: tests/qtest/qcom-dsp-test.c
>  F: docs/devel/hexagon-sys.rst
>  
> diff --git a/include/hw/hexagon/hexagon.h b/include/hw/hexagon/hexagon.h
> index 7d15a9e1eb0..32acc2aa26b 100644
> --- a/include/hw/hexagon/hexagon.h
> +++ b/include/hw/hexagon/hexagon.h
> @@ -44,6 +44,10 @@ typedef enum {
>  
>  #define HEXAGON_CFGSPACE_ENTRIES (128)
>  
> +/* Offsets of q6ss cfg peripherals from the subsystem base. */
> +#define HEXAGON_L2VIC_OFFSET  0x10000
> +#define HEXAGON_QTIMER_OFFSET 0x21000
> +
>  union hexagon_config_table {
>      struct {
>          /* Base address of L2TCM space */
> diff --git a/hw/hexagon/hexagon_dsp.c b/hw/hexagon/hexagon_dsp.c
> index 9f6f34ef9be..1f92a8adccf 100644
> --- a/hw/hexagon/hexagon_dsp.c
> +++ b/hw/hexagon/hexagon_dsp.c
> @@ -17,6 +17,7 @@
>  #include "hw/hexagon/hex-subsys.h"
>  #include "hw/core/loader.h"
>  #include "qapi/error.h"
> +#include "qapi/visitor.h"
>  #include "qemu/error-report.h"
>  #include "qemu/log.h"
>  #include "elf.h"
> @@ -35,6 +36,7 @@
>  #include "machine_cfg_v81qa_1.h.inc"
>  
>  #define TYPE_HEXAGON_DSP_MACHINE "hexagon-dsp-machine"
> +#define HEXAGON_DSP_SUBSYSTEM_BASE 0xfc900000
>  OBJECT_DECLARE_SIMPLE_TYPE(HexagonDspMachineState, HEXAGON_DSP_MACHINE)
>  
>  struct HexagonDspMachineState {
> @@ -42,6 +44,8 @@ struct HexagonDspMachineState {
>  
>      hwaddr isdb_secure_flag;
>      hwaddr isdb_trusted_flag;
> +    uint32_t subsystem_base;
> +    struct hexagon_machine_config config;
>  };
>  
>  static HexagonDspMachineState *current_dms;
> @@ -107,6 +111,50 @@ static void do_cpu_reset(void *opaque)
>      cpu_reset(cs);
>  }
>  
> +static void hexagon_sim_get_subsystem_base(Object *obj, Visitor *v,
> +                                           const char *name, void *opaque,
> +                                           Error **errp)
> +{
> +    HexagonDspMachineState *dms = HEXAGON_DSP_MACHINE(obj);
> +    uint32_t value = dms->subsystem_base;
> +
> +    visit_type_uint32(v, name, &value, errp);
> +}
> +
> +static void hexagon_sim_set_subsystem_base(Object *obj, Visitor *v,
> +                                           const char *name, void *opaque,
> +                                           Error **errp)
> +{
> +    HexagonDspMachineState *dms = HEXAGON_DSP_MACHINE(obj);
> +    uint32_t value;
> +
> +    if (!visit_type_uint32(v, name, &value, errp)) {
> +        return;
> +    }
> +    if (!value || !QEMU_IS_ALIGNED(value, 64 * KiB)) {
> +        error_setg(errp, "subsystem-base must be nonzero and 64 KiB aligned");
> +        return;
> +    }
> +
> +    dms->subsystem_base = value;
> +}
> +
> +static void add_dsp_properties(ObjectClass *oc)
> +{
> +    object_class_property_add(oc, "subsystem-base", "uint32",
> +                              hexagon_sim_get_subsystem_base,
> +                              hexagon_sim_set_subsystem_base, NULL, NULL);
> +    object_class_property_set_description(oc, "subsystem-base",
> +                                          "Simulator subsystem base address");
> +}
> +
> +static void hexagon_dsp_init(Object *obj)
> +{
> +    HexagonDspMachineState *dms = HEXAGON_DSP_MACHINE(obj);
> +
> +    dms->subsystem_base = HEXAGON_DSP_SUBSYSTEM_BASE;
> +}
> +
>  static void hexagon_common_init(MachineState *machine, Rev_t rev,
>                                  const struct hexagon_machine_config *m_cfg)
>  {
> @@ -121,7 +169,13 @@ static void hexagon_common_init(MachineState *machine, Rev_t rev,
>  
>      machine->enable_graphics = 0;
>  
> -    hex_subsys_create(hms, m_cfg, rev);
> +    dms->config = *m_cfg;
> +    dms->config.csr_base = dms->subsystem_base;
> +    dms->config.l2vic_base = dms->subsystem_base + HEXAGON_L2VIC_OFFSET;
> +    dms->config.qtmr_region = dms->subsystem_base + HEXAGON_QTIMER_OFFSET;

Since the only requirement on `subsystem_base` is that it's non-zero and
64 KiB aligned, this addition can overflow with e.g.
`subsystem-base=0xffff0000`, adding an upper-bounds check to the set
function could be nice to avoid any footguns in the future.


> +    dms->config.cfgtable.subsystem_base = dms->subsystem_base >> 16;
> +
> +    hex_subsys_create(hms, &dms->config, rev);
>  
>      g_autofree HexagonCPU **cpus = g_new(HexagonCPU *, machine->smp.cpus);
>  
> @@ -183,6 +237,7 @@ static void v66g_1024_init(ObjectClass *oc, const void *data)
>      mc->is_default = true;
>      mc->default_cpu_type = TYPE_HEXAGON_CPU_V66;
>      mc->default_cpus = 4;
> +    add_dsp_properties(oc);
>  }

-- 
Anton Johansson
rev.ng Labs Srl.