Re: [PATCH] hvf: arm: Add support for GICv3

[Alexander Graf]

On 06.01.23 17:37, Peter Maydell wrote:
> On Mon, 19 Dec 2022 at 22:08, Alexander Graf <agraf@csgraf.de> wrote:
> > We currently only support GICv2 emulation. To also support GICv3, we will
> > need to pass a few system registers into their respective handler functions.
> >
> > This patch adds support for HVF to call into the TCG callbacks for GICv3
> > system register handlers. This is safe because the GICv3 TCG code is generic
> > as long as we limit ourselves to EL0 and EL1 - which are the only modes
> > supported by HVF.
> >
> > To make sure nobody trips over that, we also annotate callbacks that don't
> > work in HVF mode, such as EL state change hooks.
> >
> > With GICv3 support in place, we can run with more than 8 vCPUs.
> >
> > Signed-off-by: Alexander Graf <agraf@csgraf.de>
> > ---
> >   hw/intc/arm_gicv3_cpuif.c   |   8 +-
> >   target/arm/hvf/hvf.c        | 151 ++++++++++++++++++++++++++++++++++++
> >   target/arm/hvf/trace-events |   2 +
> >   3 files changed, 160 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> > index b17b29288c..b4e387268c 100644
> > --- a/hw/intc/arm_gicv3_cpuif.c
> > +++ b/hw/intc/arm_gicv3_cpuif.c
> > @@ -21,6 +21,7 @@
> >   #include "hw/irq.h"
> >   #include "cpu.h"
> >   #include "target/arm/cpregs.h"
> > +#include "sysemu/tcg.h"
> >
> >   /*
> >    * Special case return value from hppvi_index(); must be larger than
> > @@ -2810,6 +2811,8 @@ void gicv3_init_cpuif(GICv3State *s)
> >            * which case we'd get the wrong value.
> >            * So instead we define the regs with no ri->opaque info, and
> >            * get back to the GICv3CPUState from the CPUARMState.
> > +         *
> > +         * These CP regs callbacks can be called from either TCG or HVF code.
> >            */
> >           define_arm_cp_regs(cpu, gicv3_cpuif_reginfo);
> >
> > @@ -2905,6 +2908,9 @@ void gicv3_init_cpuif(GICv3State *s)
> >                   define_arm_cp_regs(cpu, gicv3_cpuif_ich_apxr23_reginfo);
> >               }
> >           }
> > -        arm_register_el_change_hook(cpu, gicv3_cpuif_el_change_hook, cs);
> > +        if (tcg_enabled()) {
> > +            /* We can only trap EL changes with TCG for now */
> We could expand this a bit:
>
>   We can only trap EL changes with TCG. However the GIC interrupt
>   state only changes on EL changes involving EL2 or EL3, so for
>   the non-TCG case this is OK, as EL2 and EL3 can't exist.
>
> and assert:
>   assert(!arm_feature(&cpu->env, ARM_FEATURE_EL2));
>   assert(!arm_feature(&cpu->env, ARM_FEATURE_EL3));


Good idea! Let me add that.


> > +static uint32_t hvf_reg2cp_reg(uint32_t reg)
> > +{
> > +    return ENCODE_AA64_CP_REG(CP_REG_ARM64_SYSREG_CP,
> > +                              (reg >> 10) & 0xf,
> > +                              (reg >> 1) & 0xf,
> > +                              (reg >> 20) & 0x3,
> > +                              (reg >> 14) & 0x7,
> > +                              (reg >> 17) & 0x7);
> This file has #defines for these shift and mask constants
> (SYSREG_OP0_SHIFT etc).


Ugh, thanks for catching that!


> > +}
> > +
> > +static bool hvf_sysreg_read_cp(CPUState *cpu, uint32_t reg, uint64_t *val)
> > +{
> > +    ARMCPU *arm_cpu = ARM_CPU(cpu);
> > +    CPUARMState *env = &arm_cpu->env;
> > +    const ARMCPRegInfo *ri;
> > +
> > +    ri = get_arm_cp_reginfo(arm_cpu->cp_regs, hvf_reg2cp_reg(reg));
> > +    if (ri) {
> > +        if (ri->accessfn) {
> > +            if (ri->accessfn(env, ri, true) != CP_ACCESS_OK) {
> > +                return false;
> > +            }
> > +        }
> > +        if (ri->type & ARM_CP_CONST) {
> > +            *val = ri->resetvalue;
> > +        } else if (ri->readfn) {
> > +            *val = ri->readfn(env, ri);
> > +        } else {
> > +            *val = CPREG_FIELD64(env, ri);
> > +        }
> > +        trace_hvf_vgic_read(ri->name, *val);
> > +        return true;
> > +    }
> Can we get here for attempts by EL0 to access EL1-only
> sysregs, or does hvf send the exception to EL1 without
> trapping out to us? If we can get here for EL0 accesses we
> need to check against ri->access as well as ri->accessfn.


I just validated, GICv3 EL1 registers trap to EL1 inside the guest:


$ cat a.S
.global start
.global _main
_main:
start:
        mrs x0, ICC_AP0R0_EL1
        mov x0, #0x1234
        msr ICC_AP0R0_EL1, x0
        mov x0, #0
        ret
$ gcc -nostdlib a.S
$ gdb ./a.out
(gdb) r
Program received signal SIGILL, Illegal instruction.
0x00000000004000d4 in start ()
(gdb) x/i $pc
=> 0x4000d4 <start>:        mrs     x0, icc_ap0r0_el1


So no need to check ri->access :)


Alex