Re: [PATCH v12 23/23] hw/arm/virt: Add FEAT_GICv3_NMI feature support in virt GIC
On 2024/4/5 21:48, Peter Maydell wrote:
> On Wed, 3 Apr 2024 at 11:18, Jinjie Ruan 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
>> Reviewed-by: Richard Henderson
>> ---
>> 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
Re: [PATCH v12 23/23] hw/arm/virt: Add FEAT_GICv3_NMI feature support in virt GIC
On Wed, 3 Apr 2024 at 11:18, Jinjie Ruan 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
> Reviewed-by: Richard Henderson
> ---
> 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
[PATCH v12 23/23] hw/arm/virt: Add FEAT_GICv3_NMI feature support in virt GIC
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
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
Reviewed-by: Richard Henderson
---
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);
+}
+
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, _fatal);
sysbus_mmio_map(gicbusdev, 0, vms->memmap[VIRT_GIC_DIST].base);
--
2.34.1
