| Message ID | 20240403101611.3204086-24-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Implement FEAT_NMI and FEAT_GICv3_NMI | expand |
On Wed, 3 Apr 2024 at 11:18, Jinjie Ruan <ruanjinjie@huawei.com> wrote: > > A PE that implements FEAT_NMI and FEAT_GICv3 also implements > FEAT_GICv3_NMI. A PE that does not implement FEAT_NMI, does not implement > FEAT_GICv3_NMI This is true but not really relevant here -- FEAT_GICv3_NMI is not "NMI support in the GIC", it's "does the CPU interface support NMIs". (And so I'm wondering if the code in arm_gicv3_cpuif.c should be checking cpu_isar_feature(aa64_nmi, cpu) rather than cs->gic->nmi_support; but I need to think through the consequences of that first.) The justification for "enable NMIs in the GIC device if the CPU has FEAT_NMI" is that (a) it's only OK to have a GIC with NMI support if the CPU also has NMI support and (b) if we can turn on NMI support in the GIC we should, so that we can provide the feature to the guest. > So included support FEAT_GICv3_NMI feature as part of virt platform > GIC initialization if FEAT_NMI and FEAT_GICv3 supported. > > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > --- > v4: > - Add Reviewed-by. > v3: > - Adjust to be the last after add FEAT_NMI to max. > - Check whether support FEAT_NMI and FEAT_GICv3 for FEAT_GICv3_NMI. > --- > hw/arm/virt.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index ef2e6c2c4d..63d9f5b553 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -729,6 +729,19 @@ static void create_v2m(VirtMachineState *vms) > vms->msi_controller = VIRT_MSI_CTRL_GICV2M; > } > > +/* > + * A PE that implements FEAT_NMI and FEAT_GICv3 also implements > + * FEAT_GICv3_NMI. A PE that does not implement FEAT_NMI, does not implement > + * FEAT_GICv3_NMI. > + */ > +static bool gicv3_nmi_present(VirtMachineState *vms) > +{ > + ARMCPU *cpu = ARM_CPU(qemu_get_cpu(0)); > + > + return cpu_isar_feature(aa64_nmi, cpu) && > + (vms->gic_version != VIRT_GIC_VERSION_2); I think we should add tcg_enabled() to this condition: neither KVM nor hvf support FEAT_NMI yet. Defaulting QEMU to not trying to enable NMI in the GIC device is the safe option. As and when those accelerators get NMI support, we can add the handling to QEMU and update this code in the virt board. thanks -- PMM
On 2024/4/5 21:48, Peter Maydell wrote: > On Wed, 3 Apr 2024 at 11:18, Jinjie Ruan <ruanjinjie@huawei.com> wrote: >> >> A PE that implements FEAT_NMI and FEAT_GICv3 also implements >> FEAT_GICv3_NMI. A PE that does not implement FEAT_NMI, does not implement >> FEAT_GICv3_NMI > > This is true but not really relevant here -- FEAT_GICv3_NMI > is not "NMI support in the GIC", it's "does the CPU interface > support NMIs". (And so I'm wondering if the code in arm_gicv3_cpuif.c > should be checking cpu_isar_feature(aa64_nmi, cpu) rather than > cs->gic->nmi_support; but I need to think through the consequences > of that first.) In "1.1.2 GIC architecture extensions", it said: FEAT_GICv3_NMI introduces GIC support for non-maskable interrupts (NMIs). So in my opinion, it is relevant here. > > The justification for "enable NMIs in the GIC device if the > CPU has FEAT_NMI" is that (a) it's only OK to have a GIC with > NMI support if the CPU also has NMI support and (b) if we > can turn on NMI support in the GIC we should, so that we can > provide the feature to the guest. > >> So included support FEAT_GICv3_NMI feature as part of virt platform >> GIC initialization if FEAT_NMI and FEAT_GICv3 supported. >> >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> v4: >> - Add Reviewed-by. >> v3: >> - Adjust to be the last after add FEAT_NMI to max. >> - Check whether support FEAT_NMI and FEAT_GICv3 for FEAT_GICv3_NMI. >> --- >> hw/arm/virt.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index ef2e6c2c4d..63d9f5b553 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -729,6 +729,19 @@ static void create_v2m(VirtMachineState *vms) >> vms->msi_controller = VIRT_MSI_CTRL_GICV2M; >> } >> >> +/* >> + * A PE that implements FEAT_NMI and FEAT_GICv3 also implements >> + * FEAT_GICv3_NMI. A PE that does not implement FEAT_NMI, does not implement >> + * FEAT_GICv3_NMI. >> + */ >> +static bool gicv3_nmi_present(VirtMachineState *vms) >> +{ >> + ARMCPU *cpu = ARM_CPU(qemu_get_cpu(0)); >> + >> + return cpu_isar_feature(aa64_nmi, cpu) && >> + (vms->gic_version != VIRT_GIC_VERSION_2); > > I think we should add tcg_enabled() to this condition: > neither KVM nor hvf support FEAT_NMI yet. Defaulting QEMU to > not trying to enable NMI in the GIC device is the safe > option. As and when those accelerators get NMI support, we > can add the handling to QEMU and update this code in the virt board. > > thanks > -- PMM
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index ef2e6c2c4d..63d9f5b553 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -729,6 +729,19 @@ static void create_v2m(VirtMachineState *vms) vms->msi_controller = VIRT_MSI_CTRL_GICV2M; } +/* + * A PE that implements FEAT_NMI and FEAT_GICv3 also implements + * FEAT_GICv3_NMI. A PE that does not implement FEAT_NMI, does not implement + * FEAT_GICv3_NMI. + */ +static bool gicv3_nmi_present(VirtMachineState *vms) +{ + ARMCPU *cpu = ARM_CPU(qemu_get_cpu(0)); + + return cpu_isar_feature(aa64_nmi, cpu) && + (vms->gic_version != VIRT_GIC_VERSION_2); +} + static void create_gic(VirtMachineState *vms, MemoryRegion *mem) { MachineState *ms = MACHINE(vms); @@ -802,6 +815,11 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem) vms->virt); } } + + if (gicv3_nmi_present(vms)) { + qdev_prop_set_bit(vms->gic, "has-nmi", true); + } + gicbusdev = SYS_BUS_DEVICE(vms->gic); sysbus_realize_and_unref(gicbusdev, &error_fatal); sysbus_mmio_map(gicbusdev, 0, vms->memmap[VIRT_GIC_DIST].base);