Re: [RFC PATCH] target/arm: de-duplicate our register XML definitions

[Peter Maydell]

On Fri, 10 Jun 2022 at 16:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> We generate the XML for each vCPU we create which for heavily threaded
> linux-user runs can add up to a lot of memory. Unfortunately we can't
> only do it once as we may have vCPUs with different capabilities in
> different cores but we can at least free duplicate definitions if we
> find them.

How big actually is the XML here? 400 bytes? 4K? 40K? 400K?

> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1070
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Can we defer the work until/unless gdb actually asks for it?

I have some details-of-the-code comments below, but first:
does gdb even support having the XML for each CPU be different?
Our gdbstub.c seems to always ask for the XML only for the first
CPU in the "process". If gdb does support heterogenous CPU XML,
does it require that different XML descriptions have different
names? We always call them system-registers.xml and sve-registers.xml
regardless of whether different CPUs might have different contents
for those XML blobs...

> ---
>  target/arm/gdbstub.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index 2f806512d0..85ef6dc6db 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -318,7 +318,24 @@ int arm_gen_dynamic_sysreg_xml(CPUState *cs, int 
> base_reg)
>      g_string_append_printf(s, "<feature 
> name=\"org.qemu.gdb.arm.sys.regs\">");
>      g_hash_table_foreach(cpu->cp_regs, arm_register_sysreg_for_xml, &param);
>      g_string_append_printf(s, "</feature>");
> -    cpu->dyn_sysreg_xml.desc = g_string_free(s, false);
> +
> +    /* De-duplicate system register descriptions */
> +    if (cs != first_cpu) {
> +        CPUState *other_cs;
> +        CPU_FOREACH(other_cs) {
> +            ARMCPU *other_arm = ARM_CPU(other_cs);
> +            if (g_strcmp0(other_arm->dyn_sysreg_xml.desc, s->str) == 0) {

If you check whether the dyn_sysreg_xml.num matches first, that will
probably be a much faster way of checking in almost all cases than
doing the strcmp on the however-long-it-is XML string: it seems unlikely
that in a heterogenous config the CPUs will manage to have exactly the
same number of registers.

If you have a setup with, say, 4 CPUs of type X and then 4 of type Y,
for every type Y CPU this loop will do the strcmp of Y's XML against
the type X XML for cpu 0, then again for 1, then again for 2, then for 3,
even though in theory we already know at that point that 0,1,2,3 all
have the same XML and so if it didn't match for cpu 0 it won't match
for 1,2,3...  But maybe the comparison against the number-of-registers
saves us from having to try to optimise that away.

> +                cpu->dyn_sysreg_xml.desc = other_arm->dyn_sysreg_xml.desc;
> +                g_string_free(s, true);
> +                s = NULL;
> +                break;
> +            }
> +        }
> +    }
> +
> +    if (s) {
> +        cpu->dyn_sysreg_xml.desc = g_string_free(s, false);
> +    }
>      return cpu->dyn_sysreg_xml.num;
>  }

-- PMM