| Message ID | 20241103152639.202480-1-salil.mehta@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/intc/arm-gicv3*: Refactor GICv3 CPU reginfo to have common invocation | expand |
On 11/4/24 1:26 AM, Salil Mehta wrote: > Refactor GICv3 code for TCG and KVM to initialize the GIC CPU interface > register information by introducing a new common hook > `ARMGICv3CommonClass::init_cpu_reginfo`. This hook can be assigned to > the respective TCG or KVM variants during the GICv3 initialization > phase and invoked during the GICv3 realization phase. The primary goals > of this refactor include: > > 1. Enhancing code modularity and reusability in future patch-sets. > 2. Providing a common invocation function for TCG and KVM during GICv3 > realization. > 3. Improving code readability. > > Note: This is a non-intrusive patch with *no* functional changes > intended. Code has been tested to confirm correct initialization > sequences across relevant scenarios. > > Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com> > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > --- > hw/intc/arm_gicv3.c | 1 + > hw/intc/arm_gicv3_cpuif.c | 245 ++++++++++++++--------------- > hw/intc/arm_gicv3_cpuif_common.c | 10 ++ > hw/intc/arm_gicv3_kvm.c | 12 +- > hw/intc/gicv3_internal.h | 1 + > include/hw/intc/arm_gicv3_common.h | 1 + > 6 files changed, 140 insertions(+), 130 deletions(-) > With the following nitpicks addressed: Reviewed-by: Gavin Shan <gshan@redhat.com> > diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c > index 58e18fff54..2a30625916 100644 > --- a/hw/intc/arm_gicv3.c > +++ b/hw/intc/arm_gicv3.c > @@ -459,6 +459,7 @@ static void arm_gicv3_class_init(ObjectClass *klass, void *data) > ARMGICv3Class *agc = ARM_GICV3_CLASS(klass); > > agcc->post_load = arm_gicv3_post_load; > + agcc->init_cpu_reginfo = gicv3_init_cpu_reginfo; > device_class_set_parent_realize(dc, arm_gic_realize, &agc->parent_realize); > } > > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c > index ea1d1b3455..75a06d6f28 100644 > --- a/hw/intc/arm_gicv3_cpuif.c > +++ b/hw/intc/arm_gicv3_cpuif.c > @@ -3025,143 +3025,138 @@ static void gicv3_cpuif_el_change_hook(ARMCPU *cpu, void *opaque) > gicv3_cpuif_virt_irq_fiq_update(cs); > } > > -void gicv3_init_cpuif(GICv3State *s) > +void gicv3_init_cpu_reginfo(CPUState *cs) > { Ideally, this function together with the dependent register information and definitions can be moved to arm_gicv3.c where is the only place this function is referred and used. In this way, gicv3_init_cpu_reginfo() becomes a static function and needn't to be exported in gicv3_internal.h. > - /* Called from the GICv3 realize function; register our system > - * registers with the CPU > - */ > - int i; > + ARMCPU *cpu = ARM_CPU(cs); > + GICv3CPUState *gcs = icc_cs_from_env(&cpu->env); > > - for (i = 0; i < s->num_cpu; i++) { > - ARMCPU *cpu = ARM_CPU(qemu_get_cpu(i)); > - GICv3CPUState *cs = &s->cpu[i]; > + /* > + * If the CPU doesn't define a GICv3 configuration, probably because > + * in real hardware it doesn't have one, then we use default values > + * matching the one used by most Arm CPUs. This applies to: > + * cpu->gic_num_lrs > + * cpu->gic_vpribits > + * cpu->gic_vprebits > + * cpu->gic_pribits > + */ > > - /* > - * If the CPU doesn't define a GICv3 configuration, probably because > - * in real hardware it doesn't have one, then we use default values > - * matching the one used by most Arm CPUs. This applies to: > - * cpu->gic_num_lrs > - * cpu->gic_vpribits > - * cpu->gic_vprebits > - * cpu->gic_pribits > - */ > + /* > + * Note that we can't just use the GICv3CPUState as an opaque pointer > + * in define_arm_cp_regs_with_opaque(), because when we're called back > + * it might be with code translated by CPU 0 but run by CPU 1, in > + * 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); > > - /* Note that we can't just use the GICv3CPUState as an opaque pointer > - * in define_arm_cp_regs_with_opaque(), because when we're called back > - * it might be with code translated by CPU 0 but run by CPU 1, in > - * 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); > + /* > + * If the CPU implements FEAT_NMI and FEAT_GICv3 it must also > + * implement FEAT_GICv3_NMI, which is the CPU interface part > + * of NMI support. This is distinct from whether the GIC proper > + * (redistributors and distributor) have NMI support. In QEMU > + * that is a property of the GIC device in s->nmi_support; > + * cs->nmi_support indicates the CPU interface's support. > + */ > + if (cpu_isar_feature(aa64_nmi, cpu)) { > + gcs->nmi_support = true; > + define_arm_cp_regs(cpu, gicv3_cpuif_gicv3_nmi_reginfo); > + } > > - /* > - * If the CPU implements FEAT_NMI and FEAT_GICv3 it must also > - * implement FEAT_GICv3_NMI, which is the CPU interface part > - * of NMI support. This is distinct from whether the GIC proper > - * (redistributors and distributor) have NMI support. In QEMU > - * that is a property of the GIC device in s->nmi_support; > - * cs->nmi_support indicates the CPU interface's support. > - */ > - if (cpu_isar_feature(aa64_nmi, cpu)) { > - cs->nmi_support = true; > - define_arm_cp_regs(cpu, gicv3_cpuif_gicv3_nmi_reginfo); > - } > + /* > + * The CPU implementation specifies the number of supported > + * bits of physical priority. For backwards compatibility > + * of migration, we have a compat property that forces use > + * of 8 priority bits regardless of what the CPU really has. > + */ > + if (gcs->gic->force_8bit_prio) { > + gcs->pribits = 8; > + } else { > + gcs->pribits = cpu->gic_pribits ?: 5; > + } > > - /* > - * The CPU implementation specifies the number of supported > - * bits of physical priority. For backwards compatibility > - * of migration, we have a compat property that forces use > - * of 8 priority bits regardless of what the CPU really has. > - */ > - if (s->force_8bit_prio) { > - cs->pribits = 8; > - } else { > - cs->pribits = cpu->gic_pribits ?: 5; > - } > + /* > + * The GICv3 has separate ID register fields for virtual priority > + * and preemption bit values, but only a single ID register field > + * for the physical priority bits. The preemption bit count is > + * always the same as the priority bit count, except that 8 bits > + * of priority means 7 preemption bits. We precalculate the > + * preemption bits because it simplifies the code and makes the > + * parallels between the virtual and physical bits of the GIC > + * a bit clearer. > + */ > + gcs->prebits = gcs->pribits; > + if (gcs->prebits == 8) { > + gcs->prebits--; > + } > + /* > + * Check that CPU code defining pribits didn't violate > + * architectural constraints our implementation relies on. > + */ > + g_assert(gcs->pribits >= 4 && gcs->pribits <= 8); > > - /* > - * The GICv3 has separate ID register fields for virtual priority > - * and preemption bit values, but only a single ID register field > - * for the physical priority bits. The preemption bit count is > - * always the same as the priority bit count, except that 8 bits > - * of priority means 7 preemption bits. We precalculate the > - * preemption bits because it simplifies the code and makes the > - * parallels between the virtual and physical bits of the GIC > - * a bit clearer. > - */ > - cs->prebits = cs->pribits; > - if (cs->prebits == 8) { > - cs->prebits--; > - } > - /* > - * Check that CPU code defining pribits didn't violate > - * architectural constraints our implementation relies on. > - */ > - g_assert(cs->pribits >= 4 && cs->pribits <= 8); > + /* > + * gicv3_cpuif_reginfo[] defines ICC_AP*R0_EL1; add definitions > + * for ICC_AP*R{1,2,3}_EL1 if the prebits value requires them. > + */ > + if (gcs->prebits >= 6) { > + define_arm_cp_regs(cpu, gicv3_cpuif_icc_apxr1_reginfo); > + } > + if (gcs->prebits == 7) { > + define_arm_cp_regs(cpu, gicv3_cpuif_icc_apxr23_reginfo); > + } > > - /* > - * gicv3_cpuif_reginfo[] defines ICC_AP*R0_EL1; add definitions > - * for ICC_AP*R{1,2,3}_EL1 if the prebits value requires them. > - */ > - if (cs->prebits >= 6) { > - define_arm_cp_regs(cpu, gicv3_cpuif_icc_apxr1_reginfo); > - } > - if (cs->prebits == 7) { > - define_arm_cp_regs(cpu, gicv3_cpuif_icc_apxr23_reginfo); > - } > + if (arm_feature(&cpu->env, ARM_FEATURE_EL2)) { > + int j; > > - if (arm_feature(&cpu->env, ARM_FEATURE_EL2)) { > - int j; > + gcs->num_list_regs = cpu->gic_num_lrs ?: 4; > + gcs->vpribits = cpu->gic_vpribits ?: 5; > + gcs->vprebits = cpu->gic_vprebits ?: 5; > > - cs->num_list_regs = cpu->gic_num_lrs ?: 4; > - cs->vpribits = cpu->gic_vpribits ?: 5; > - cs->vprebits = cpu->gic_vprebits ?: 5; > > - /* Check against architectural constraints: getting these > - * wrong would be a bug in the CPU code defining these, > - * and the implementation relies on them holding. > - */ > - g_assert(cs->vprebits <= cs->vpribits); > - g_assert(cs->vprebits >= 5 && cs->vprebits <= 7); > - g_assert(cs->vpribits >= 5 && cs->vpribits <= 8); > + /* Check against architectural constraints: getting these > + * wrong would be a bug in the CPU code defining these, > + * and the implementation relies on them holding. > + */ > + g_assert(gcs->vprebits <= gcs->vpribits); > + g_assert(gcs->vprebits >= 5 && gcs->vprebits <= 7); > + g_assert(gcs->vpribits >= 5 && gcs->vpribits <= 8); > [gshan@gshan q]$ ./scripts/checkpatch.pl --codespell patch/check/0003* WARNING: Block comments use a leading /* on a separate line #240: FILE: hw/intc/arm_gicv3_cpuif.c:3119: + /* Check against architectural constraints: getting these > - define_arm_cp_regs(cpu, gicv3_cpuif_hcr_reginfo); > + define_arm_cp_regs(cpu, gicv3_cpuif_hcr_reginfo); > > - for (j = 0; j < cs->num_list_regs; j++) { > - /* Note that the AArch64 LRs are 64-bit; the AArch32 LRs > - * are split into two cp15 regs, LR (the low part, with the > - * same encoding as the AArch64 LR) and LRC (the high part). > - */ > - ARMCPRegInfo lr_regset[] = { > - { .name = "ICH_LRn_EL2", .state = ARM_CP_STATE_BOTH, > - .opc0 = 3, .opc1 = 4, .crn = 12, > - .crm = 12 + (j >> 3), .opc2 = j & 7, > - .type = ARM_CP_IO | ARM_CP_NO_RAW, > - .nv2_redirect_offset = 0x400 + 8 * j, > - .access = PL2_RW, > - .readfn = ich_lr_read, > - .writefn = ich_lr_write, > - }, > - { .name = "ICH_LRCn_EL2", .state = ARM_CP_STATE_AA32, > - .cp = 15, .opc1 = 4, .crn = 12, > - .crm = 14 + (j >> 3), .opc2 = j & 7, > - .type = ARM_CP_IO | ARM_CP_NO_RAW, > - .access = PL2_RW, > - .readfn = ich_lr_read, > - .writefn = ich_lr_write, > - }, > - }; > - define_arm_cp_regs(cpu, lr_regset); > - } > - if (cs->vprebits >= 6) { > - define_arm_cp_regs(cpu, gicv3_cpuif_ich_apxr1_reginfo); > - } > - if (cs->vprebits == 7) { > - define_arm_cp_regs(cpu, gicv3_cpuif_ich_apxr23_reginfo); > - } > + for (j = 0; j < gcs->num_list_regs; j++) { > + /* Note that the AArch64 LRs are 64-bit; the AArch32 LRs > + * are split into two cp15 regs, LR (the low part, with the > + * same encoding as the AArch64 LR) and LRC (the high part). > + */ > + ARMCPRegInfo lr_regset[] = { > + { .name = "ICH_LRn_EL2", .state = ARM_CP_STATE_BOTH, > + .opc0 = 3, .opc1 = 4, .crn = 12, > + .crm = 12 + (j >> 3), .opc2 = j & 7, > + .type = ARM_CP_IO | ARM_CP_NO_RAW, > + .nv2_redirect_offset = 0x400 + 8 * j, > + .access = PL2_RW, > + .readfn = ich_lr_read, > + .writefn = ich_lr_write, > + }, > + { .name = "ICH_LRCn_EL2", .state = ARM_CP_STATE_AA32, > + .cp = 15, .opc1 = 4, .crn = 12, > + .crm = 14 + (j >> 3), .opc2 = j & 7, > + .type = ARM_CP_IO | ARM_CP_NO_RAW, > + .access = PL2_RW, > + .readfn = ich_lr_read, > + .writefn = ich_lr_write, > + }, > + }; > + define_arm_cp_regs(cpu, lr_regset); > + } [gshan@gshan q]$ ./scripts/checkpatch.pl --codespell patch/check/0003* WARNING: Block comments use a leading /* on a separate line #284: FILE: hw/intc/arm_gicv3_cpuif.c:3130: + /* Note that the AArch64 LRs are 64-bit; the AArch32 LRs > + if (gcs->vprebits >= 6) { > + define_arm_cp_regs(cpu, gicv3_cpuif_ich_apxr1_reginfo); > + } > + if (gcs->vprebits == 7) { > + define_arm_cp_regs(cpu, gicv3_cpuif_ich_apxr23_reginfo); > } > if (tcg_enabled() || qtest_enabled()) { > /* > @@ -3169,7 +3164,7 @@ void gicv3_init_cpuif(GICv3State *s) > * 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. > */ > - arm_register_el_change_hook(cpu, gicv3_cpuif_el_change_hook, cs); > + arm_register_el_change_hook(cpu, gicv3_cpuif_el_change_hook, gcs); > } else { > assert(!arm_feature(&cpu->env, ARM_FEATURE_EL2)); > assert(!arm_feature(&cpu->env, ARM_FEATURE_EL3)); > diff --git a/hw/intc/arm_gicv3_cpuif_common.c b/hw/intc/arm_gicv3_cpuif_common.c > index ff1239f65d..f4d5ca447b 100644 > --- a/hw/intc/arm_gicv3_cpuif_common.c > +++ b/hw/intc/arm_gicv3_cpuif_common.c > @@ -20,3 +20,13 @@ void gicv3_set_gicv3state(CPUState *cpu, GICv3CPUState *s) > > env->gicv3state = (void *)s; > }; > + > +void gicv3_init_cpuif(GICv3State *s) > +{ > + ARMGICv3CommonClass *agcc = ARM_GICV3_COMMON_GET_CLASS(s); > + > + /* define and register `system registers` with the vCPU */ > + for (int i = 0; i < s->num_cpu; i++) { > + agcc->init_cpu_reginfo(s->cpu[i].cpu); > + } > +} > diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c > index 9ea6b8e218..b4128786ca 100644 > --- a/hw/intc/arm_gicv3_kvm.c > +++ b/hw/intc/arm_gicv3_kvm.c > @@ -777,6 +777,10 @@ static void vm_change_state_handler(void *opaque, bool running, > } > } > > +static void kvm_gicv3_init_cpu_reginfo(CPUState *cs) > +{ > + define_arm_cp_regs(ARM_CPU(cs), gicv3_cpuif_reginfo); > +} > > static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) > { > @@ -812,11 +816,8 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) > > gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL); > > - for (i = 0; i < s->num_cpu; i++) { > - ARMCPU *cpu = ARM_CPU(qemu_get_cpu(i)); > - > - define_arm_cp_regs(cpu, gicv3_cpuif_reginfo); > - } > + /* Initialize vCPU interface */ > + gicv3_init_cpuif(s); > > /* Try to create the device via the device control API */ > s->dev_fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V3, false); > @@ -902,6 +903,7 @@ static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data) > > agcc->pre_save = kvm_arm_gicv3_get; > agcc->post_load = kvm_arm_gicv3_put; > + agcc->init_cpu_reginfo = kvm_gicv3_init_cpu_reginfo; > device_class_set_parent_realize(dc, kvm_arm_gicv3_realize, > &kgc->parent_realize); > resettable_class_set_parent_phases(rc, NULL, kvm_arm_gicv3_reset_hold, NULL, > diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h > index bc9f518fe8..cc8edc499b 100644 > --- a/hw/intc/gicv3_internal.h > +++ b/hw/intc/gicv3_internal.h > @@ -722,6 +722,7 @@ void gicv3_redist_vinvall(GICv3CPUState *cs, uint64_t vptaddr); > > void gicv3_redist_send_sgi(GICv3CPUState *cs, int grp, int irq, bool ns); > void gicv3_init_cpuif(GICv3State *s); > +void gicv3_init_cpu_reginfo(CPUState *cs); > > /** > * gicv3_cpuif_update: > diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h > index cd09bee3bc..7171f770e4 100644 > --- a/include/hw/intc/arm_gicv3_common.h > +++ b/include/hw/intc/arm_gicv3_common.h > @@ -338,6 +338,7 @@ struct ARMGICv3CommonClass { > > void (*pre_save)(GICv3State *s); > void (*post_load)(GICv3State *s); > + void (*init_cpu_reginfo)(CPUState *cs); > }; > > void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, Thanks, Gavin
diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c index 58e18fff54..2a30625916 100644 --- a/hw/intc/arm_gicv3.c +++ b/hw/intc/arm_gicv3.c @@ -459,6 +459,7 @@ static void arm_gicv3_class_init(ObjectClass *klass, void *data) ARMGICv3Class *agc = ARM_GICV3_CLASS(klass); agcc->post_load = arm_gicv3_post_load; + agcc->init_cpu_reginfo = gicv3_init_cpu_reginfo; device_class_set_parent_realize(dc, arm_gic_realize, &agc->parent_realize); } diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c index ea1d1b3455..75a06d6f28 100644 --- a/hw/intc/arm_gicv3_cpuif.c +++ b/hw/intc/arm_gicv3_cpuif.c @@ -3025,143 +3025,138 @@ static void gicv3_cpuif_el_change_hook(ARMCPU *cpu, void *opaque) gicv3_cpuif_virt_irq_fiq_update(cs); } -void gicv3_init_cpuif(GICv3State *s) +void gicv3_init_cpu_reginfo(CPUState *cs) { - /* Called from the GICv3 realize function; register our system - * registers with the CPU - */ - int i; + ARMCPU *cpu = ARM_CPU(cs); + GICv3CPUState *gcs = icc_cs_from_env(&cpu->env); - for (i = 0; i < s->num_cpu; i++) { - ARMCPU *cpu = ARM_CPU(qemu_get_cpu(i)); - GICv3CPUState *cs = &s->cpu[i]; + /* + * If the CPU doesn't define a GICv3 configuration, probably because + * in real hardware it doesn't have one, then we use default values + * matching the one used by most Arm CPUs. This applies to: + * cpu->gic_num_lrs + * cpu->gic_vpribits + * cpu->gic_vprebits + * cpu->gic_pribits + */ - /* - * If the CPU doesn't define a GICv3 configuration, probably because - * in real hardware it doesn't have one, then we use default values - * matching the one used by most Arm CPUs. This applies to: - * cpu->gic_num_lrs - * cpu->gic_vpribits - * cpu->gic_vprebits - * cpu->gic_pribits - */ + /* + * Note that we can't just use the GICv3CPUState as an opaque pointer + * in define_arm_cp_regs_with_opaque(), because when we're called back + * it might be with code translated by CPU 0 but run by CPU 1, in + * 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); - /* Note that we can't just use the GICv3CPUState as an opaque pointer - * in define_arm_cp_regs_with_opaque(), because when we're called back - * it might be with code translated by CPU 0 but run by CPU 1, in - * 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); + /* + * If the CPU implements FEAT_NMI and FEAT_GICv3 it must also + * implement FEAT_GICv3_NMI, which is the CPU interface part + * of NMI support. This is distinct from whether the GIC proper + * (redistributors and distributor) have NMI support. In QEMU + * that is a property of the GIC device in s->nmi_support; + * cs->nmi_support indicates the CPU interface's support. + */ + if (cpu_isar_feature(aa64_nmi, cpu)) { + gcs->nmi_support = true; + define_arm_cp_regs(cpu, gicv3_cpuif_gicv3_nmi_reginfo); + } - /* - * If the CPU implements FEAT_NMI and FEAT_GICv3 it must also - * implement FEAT_GICv3_NMI, which is the CPU interface part - * of NMI support. This is distinct from whether the GIC proper - * (redistributors and distributor) have NMI support. In QEMU - * that is a property of the GIC device in s->nmi_support; - * cs->nmi_support indicates the CPU interface's support. - */ - if (cpu_isar_feature(aa64_nmi, cpu)) { - cs->nmi_support = true; - define_arm_cp_regs(cpu, gicv3_cpuif_gicv3_nmi_reginfo); - } + /* + * The CPU implementation specifies the number of supported + * bits of physical priority. For backwards compatibility + * of migration, we have a compat property that forces use + * of 8 priority bits regardless of what the CPU really has. + */ + if (gcs->gic->force_8bit_prio) { + gcs->pribits = 8; + } else { + gcs->pribits = cpu->gic_pribits ?: 5; + } - /* - * The CPU implementation specifies the number of supported - * bits of physical priority. For backwards compatibility - * of migration, we have a compat property that forces use - * of 8 priority bits regardless of what the CPU really has. - */ - if (s->force_8bit_prio) { - cs->pribits = 8; - } else { - cs->pribits = cpu->gic_pribits ?: 5; - } + /* + * The GICv3 has separate ID register fields for virtual priority + * and preemption bit values, but only a single ID register field + * for the physical priority bits. The preemption bit count is + * always the same as the priority bit count, except that 8 bits + * of priority means 7 preemption bits. We precalculate the + * preemption bits because it simplifies the code and makes the + * parallels between the virtual and physical bits of the GIC + * a bit clearer. + */ + gcs->prebits = gcs->pribits; + if (gcs->prebits == 8) { + gcs->prebits--; + } + /* + * Check that CPU code defining pribits didn't violate + * architectural constraints our implementation relies on. + */ + g_assert(gcs->pribits >= 4 && gcs->pribits <= 8); - /* - * The GICv3 has separate ID register fields for virtual priority - * and preemption bit values, but only a single ID register field - * for the physical priority bits. The preemption bit count is - * always the same as the priority bit count, except that 8 bits - * of priority means 7 preemption bits. We precalculate the - * preemption bits because it simplifies the code and makes the - * parallels between the virtual and physical bits of the GIC - * a bit clearer. - */ - cs->prebits = cs->pribits; - if (cs->prebits == 8) { - cs->prebits--; - } - /* - * Check that CPU code defining pribits didn't violate - * architectural constraints our implementation relies on. - */ - g_assert(cs->pribits >= 4 && cs->pribits <= 8); + /* + * gicv3_cpuif_reginfo[] defines ICC_AP*R0_EL1; add definitions + * for ICC_AP*R{1,2,3}_EL1 if the prebits value requires them. + */ + if (gcs->prebits >= 6) { + define_arm_cp_regs(cpu, gicv3_cpuif_icc_apxr1_reginfo); + } + if (gcs->prebits == 7) { + define_arm_cp_regs(cpu, gicv3_cpuif_icc_apxr23_reginfo); + } - /* - * gicv3_cpuif_reginfo[] defines ICC_AP*R0_EL1; add definitions - * for ICC_AP*R{1,2,3}_EL1 if the prebits value requires them. - */ - if (cs->prebits >= 6) { - define_arm_cp_regs(cpu, gicv3_cpuif_icc_apxr1_reginfo); - } - if (cs->prebits == 7) { - define_arm_cp_regs(cpu, gicv3_cpuif_icc_apxr23_reginfo); - } + if (arm_feature(&cpu->env, ARM_FEATURE_EL2)) { + int j; - if (arm_feature(&cpu->env, ARM_FEATURE_EL2)) { - int j; + gcs->num_list_regs = cpu->gic_num_lrs ?: 4; + gcs->vpribits = cpu->gic_vpribits ?: 5; + gcs->vprebits = cpu->gic_vprebits ?: 5; - cs->num_list_regs = cpu->gic_num_lrs ?: 4; - cs->vpribits = cpu->gic_vpribits ?: 5; - cs->vprebits = cpu->gic_vprebits ?: 5; - /* Check against architectural constraints: getting these - * wrong would be a bug in the CPU code defining these, - * and the implementation relies on them holding. - */ - g_assert(cs->vprebits <= cs->vpribits); - g_assert(cs->vprebits >= 5 && cs->vprebits <= 7); - g_assert(cs->vpribits >= 5 && cs->vpribits <= 8); + /* Check against architectural constraints: getting these + * wrong would be a bug in the CPU code defining these, + * and the implementation relies on them holding. + */ + g_assert(gcs->vprebits <= gcs->vpribits); + g_assert(gcs->vprebits >= 5 && gcs->vprebits <= 7); + g_assert(gcs->vpribits >= 5 && gcs->vpribits <= 8); - define_arm_cp_regs(cpu, gicv3_cpuif_hcr_reginfo); + define_arm_cp_regs(cpu, gicv3_cpuif_hcr_reginfo); - for (j = 0; j < cs->num_list_regs; j++) { - /* Note that the AArch64 LRs are 64-bit; the AArch32 LRs - * are split into two cp15 regs, LR (the low part, with the - * same encoding as the AArch64 LR) and LRC (the high part). - */ - ARMCPRegInfo lr_regset[] = { - { .name = "ICH_LRn_EL2", .state = ARM_CP_STATE_BOTH, - .opc0 = 3, .opc1 = 4, .crn = 12, - .crm = 12 + (j >> 3), .opc2 = j & 7, - .type = ARM_CP_IO | ARM_CP_NO_RAW, - .nv2_redirect_offset = 0x400 + 8 * j, - .access = PL2_RW, - .readfn = ich_lr_read, - .writefn = ich_lr_write, - }, - { .name = "ICH_LRCn_EL2", .state = ARM_CP_STATE_AA32, - .cp = 15, .opc1 = 4, .crn = 12, - .crm = 14 + (j >> 3), .opc2 = j & 7, - .type = ARM_CP_IO | ARM_CP_NO_RAW, - .access = PL2_RW, - .readfn = ich_lr_read, - .writefn = ich_lr_write, - }, - }; - define_arm_cp_regs(cpu, lr_regset); - } - if (cs->vprebits >= 6) { - define_arm_cp_regs(cpu, gicv3_cpuif_ich_apxr1_reginfo); - } - if (cs->vprebits == 7) { - define_arm_cp_regs(cpu, gicv3_cpuif_ich_apxr23_reginfo); - } + for (j = 0; j < gcs->num_list_regs; j++) { + /* Note that the AArch64 LRs are 64-bit; the AArch32 LRs + * are split into two cp15 regs, LR (the low part, with the + * same encoding as the AArch64 LR) and LRC (the high part). + */ + ARMCPRegInfo lr_regset[] = { + { .name = "ICH_LRn_EL2", .state = ARM_CP_STATE_BOTH, + .opc0 = 3, .opc1 = 4, .crn = 12, + .crm = 12 + (j >> 3), .opc2 = j & 7, + .type = ARM_CP_IO | ARM_CP_NO_RAW, + .nv2_redirect_offset = 0x400 + 8 * j, + .access = PL2_RW, + .readfn = ich_lr_read, + .writefn = ich_lr_write, + }, + { .name = "ICH_LRCn_EL2", .state = ARM_CP_STATE_AA32, + .cp = 15, .opc1 = 4, .crn = 12, + .crm = 14 + (j >> 3), .opc2 = j & 7, + .type = ARM_CP_IO | ARM_CP_NO_RAW, + .access = PL2_RW, + .readfn = ich_lr_read, + .writefn = ich_lr_write, + }, + }; + define_arm_cp_regs(cpu, lr_regset); + } + if (gcs->vprebits >= 6) { + define_arm_cp_regs(cpu, gicv3_cpuif_ich_apxr1_reginfo); + } + if (gcs->vprebits == 7) { + define_arm_cp_regs(cpu, gicv3_cpuif_ich_apxr23_reginfo); } if (tcg_enabled() || qtest_enabled()) { /* @@ -3169,7 +3164,7 @@ void gicv3_init_cpuif(GICv3State *s) * 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. */ - arm_register_el_change_hook(cpu, gicv3_cpuif_el_change_hook, cs); + arm_register_el_change_hook(cpu, gicv3_cpuif_el_change_hook, gcs); } else { assert(!arm_feature(&cpu->env, ARM_FEATURE_EL2)); assert(!arm_feature(&cpu->env, ARM_FEATURE_EL3)); diff --git a/hw/intc/arm_gicv3_cpuif_common.c b/hw/intc/arm_gicv3_cpuif_common.c index ff1239f65d..f4d5ca447b 100644 --- a/hw/intc/arm_gicv3_cpuif_common.c +++ b/hw/intc/arm_gicv3_cpuif_common.c @@ -20,3 +20,13 @@ void gicv3_set_gicv3state(CPUState *cpu, GICv3CPUState *s) env->gicv3state = (void *)s; }; + +void gicv3_init_cpuif(GICv3State *s) +{ + ARMGICv3CommonClass *agcc = ARM_GICV3_COMMON_GET_CLASS(s); + + /* define and register `system registers` with the vCPU */ + for (int i = 0; i < s->num_cpu; i++) { + agcc->init_cpu_reginfo(s->cpu[i].cpu); + } +} diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c index 9ea6b8e218..b4128786ca 100644 --- a/hw/intc/arm_gicv3_kvm.c +++ b/hw/intc/arm_gicv3_kvm.c @@ -777,6 +777,10 @@ static void vm_change_state_handler(void *opaque, bool running, } } +static void kvm_gicv3_init_cpu_reginfo(CPUState *cs) +{ + define_arm_cp_regs(ARM_CPU(cs), gicv3_cpuif_reginfo); +} static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) { @@ -812,11 +816,8 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL); - for (i = 0; i < s->num_cpu; i++) { - ARMCPU *cpu = ARM_CPU(qemu_get_cpu(i)); - - define_arm_cp_regs(cpu, gicv3_cpuif_reginfo); - } + /* Initialize vCPU interface */ + gicv3_init_cpuif(s); /* Try to create the device via the device control API */ s->dev_fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V3, false); @@ -902,6 +903,7 @@ static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data) agcc->pre_save = kvm_arm_gicv3_get; agcc->post_load = kvm_arm_gicv3_put; + agcc->init_cpu_reginfo = kvm_gicv3_init_cpu_reginfo; device_class_set_parent_realize(dc, kvm_arm_gicv3_realize, &kgc->parent_realize); resettable_class_set_parent_phases(rc, NULL, kvm_arm_gicv3_reset_hold, NULL, diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h index bc9f518fe8..cc8edc499b 100644 --- a/hw/intc/gicv3_internal.h +++ b/hw/intc/gicv3_internal.h @@ -722,6 +722,7 @@ void gicv3_redist_vinvall(GICv3CPUState *cs, uint64_t vptaddr); void gicv3_redist_send_sgi(GICv3CPUState *cs, int grp, int irq, bool ns); void gicv3_init_cpuif(GICv3State *s); +void gicv3_init_cpu_reginfo(CPUState *cs); /** * gicv3_cpuif_update: diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h index cd09bee3bc..7171f770e4 100644 --- a/include/hw/intc/arm_gicv3_common.h +++ b/include/hw/intc/arm_gicv3_common.h @@ -338,6 +338,7 @@ struct ARMGICv3CommonClass { void (*pre_save)(GICv3State *s); void (*post_load)(GICv3State *s); + void (*init_cpu_reginfo)(CPUState *cs); }; void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
Refactor GICv3 code for TCG and KVM to initialize the GIC CPU interface register information by introducing a new common hook `ARMGICv3CommonClass::init_cpu_reginfo`. This hook can be assigned to the respective TCG or KVM variants during the GICv3 initialization phase and invoked during the GICv3 realization phase. The primary goals of this refactor include: 1. Enhancing code modularity and reusability in future patch-sets. 2. Providing a common invocation function for TCG and KVM during GICv3 realization. 3. Improving code readability. Note: This is a non-intrusive patch with *no* functional changes intended. Code has been tested to confirm correct initialization sequences across relevant scenarios. Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> Signed-off-by: Salil Mehta <salil.mehta@huawei.com> --- hw/intc/arm_gicv3.c | 1 + hw/intc/arm_gicv3_cpuif.c | 245 ++++++++++++++--------------- hw/intc/arm_gicv3_cpuif_common.c | 10 ++ hw/intc/arm_gicv3_kvm.c | 12 +- hw/intc/gicv3_internal.h | 1 + include/hw/intc/arm_gicv3_common.h | 1 + 6 files changed, 140 insertions(+), 130 deletions(-)