[PATCH 0/2] Fix PIC interrupt handling of x86 CPUs if APIC is globally disabled

[PATCH 0/2] Fix PIC interrupt handling of x86 CPUs if APIC is globally disabled

[Bernhard Beschow]

This two-patch series is part of my work emulating the VIA Apollo Pro 133T
chipset in QEMU [1] and testing it by running real-world BIOSes on it. The first
patch fixes an issue regarding PIC interrupt handling, the second one just fixes
a typo in a comment.

During testing, I've found that the boot process gets stuck for some BIOSes that
disable the LAPIC globally (by disabling the enable bit in the base address
register). QEMU seems to emulate PIC interrupt handling only if a CPU doesn't
have a LAPIC, and always emulates LAPIC interrupt handling if one is present.
According to the Intel documentation, a CPU should resort to PIC interrupt
handling if its LAPIC is globally didabled. This series fixes this corner case
which makes the boot process succeed. More details can be found in the commit
message.

Testing done:
* `make check`
* `make check-avocado`

[1] https://github.com/shentok/qemu/tree/via-apollo-pro-133t

Bernhard Beschow (2):
  hw/i386/x86: Fix PIC interrupt handling if APIC globally disabled
  target/i386/cpu: Fix small typo in comment

 hw/i386/x86.c     | 10 +++++-----
 target/i386/cpu.c |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH 1/2] hw/i386/x86: Fix PIC interrupt handling if APIC globally disabled

[Bernhard Beschow]

QEMU populates the apic_state attribute of x86 CPUs if supported by real
hardware. Even when the APIC is globally disabled by a guest, this attribute
stays populated. This means that the APIC code paths are still used in this
case. However, chapter 10.4.3 of [1] requires that:

  When IA32_APIC_BASE[11] is 0, the processor is functionally equivalent to an
  IA-32 processor without an on-chip APIC. The CPUID feature flag for the APIC
  [...] is also set to 0.

Fix this by checking the APIC feature flag rather than apic_state when deciding
whether PIC or APIC behavior is required. This fixes some real-world BIOSes.

Notice that presence of the CPUID_APIC flag implies that apic_state is non-NULL.

[1] Intel 64 and IA-32 Architectures Software Developer's Manual, Vol. 3A:
    System Programming Guide, Part 1

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/x86.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 2b6291ad8d..a753d1aeca 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -516,10 +516,10 @@ static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
     CPU_FOREACH(cs) {
         X86CPU *cpu = X86_CPU(cs);
 
-        if (!cpu->apic_state) {
-            cpu_interrupt(cs, CPU_INTERRUPT_NMI);
-        } else {
+        if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC) {
             apic_deliver_nmi(cpu->apic_state);
+        } else {
+            cpu_interrupt(cs, CPU_INTERRUPT_NMI);
         }
     }
 }
@@ -551,8 +551,8 @@ static void pic_irq_request(void *opaque, int irq, int level)
     X86CPU *cpu = X86_CPU(cs);
 
     trace_x86_pic_interrupt(irq, level);
-    if (cpu->apic_state && !kvm_irqchip_in_kernel() &&
-        !whpx_apic_in_platform()) {
+    if ((cpu->env.features[FEAT_1_EDX] & CPUID_APIC) &&
+        !kvm_irqchip_in_kernel() && !whpx_apic_in_platform()) {
         CPU_FOREACH(cs) {
             cpu = X86_CPU(cs);
             if (apic_accept_pic_intr(cpu->apic_state)) {
-- 
2.43.0

[PATCH 2/2] target/i386/cpu: Fix small typo in comment

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 target/i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 95d5f16cd5..2ae271e9a1 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2179,7 +2179,7 @@ static const CPUCaches epyc_genoa_cache_info = {
  *  Conceal VM entries from PT
  *  Enable ENCLS exiting
  *  Mode-based execute control (XS/XU)
- s  TSC scaling (Skylake Server and newer)
+ *  TSC scaling (Skylake Server and newer)
  *  GPA translation for PT (IceLake and newer)
  *  User wait and pause
  *  ENCLV exiting
-- 
2.43.0

Re: [PATCH 2/2] target/i386/cpu: Fix small typo in comment

[Alex Bennée]

Bernhard Beschow <shentey@gmail.com> writes:

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  target/i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 95d5f16cd5..2ae271e9a1 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -2179,7 +2179,7 @@ static const CPUCaches epyc_genoa_cache_info = {
>   *  Conceal VM entries from PT
>   *  Enable ENCLS exiting
>   *  Mode-based execute control (XS/XU)
> - s  TSC scaling (Skylake Server and newer)
> + *  TSC scaling (Skylake Server and newer)
>   *  GPA translation for PT (IceLake and newer)
>   *  User wait and pause
>   *  ENCLV exiting

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 1/2] hw/i386/x86: Fix PIC interrupt handling if APIC globally disabled

[Alex Bennée]

Bernhard Beschow <shentey@gmail.com> writes:

> QEMU populates the apic_state attribute of x86 CPUs if supported by real
> hardware. Even when the APIC is globally disabled by a guest, this attribute
> stays populated. This means that the APIC code paths are still used in this
> case. However, chapter 10.4.3 of [1] requires that:
>
>   When IA32_APIC_BASE[11] is 0, the processor is functionally equivalent to an
>   IA-32 processor without an on-chip APIC. The CPUID feature flag for the APIC
>   [...] is also set to 0.
>
> Fix this by checking the APIC feature flag rather than apic_state when deciding
> whether PIC or APIC behavior is required. This fixes some real-world BIOSes.
>
> Notice that presence of the CPUID_APIC flag implies that apic_state is non-NULL.
>
> [1] Intel 64 and IA-32 Architectures Software Developer's Manual, Vol. 3A:
>     System Programming Guide, Part 1
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  hw/i386/x86.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 2b6291ad8d..a753d1aeca 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -516,10 +516,10 @@ static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
>      CPU_FOREACH(cs) {
>          X86CPU *cpu = X86_CPU(cs);
>  
> -        if (!cpu->apic_state) {
> -            cpu_interrupt(cs, CPU_INTERRUPT_NMI);
> -        } else {
> +        if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC) {

You could assert the relationship between the feature and ->apic_state with:

  g_assert(cpu->apic_state)

But probably unnecessary in the grand scheme of things. Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 1/2] hw/i386/x86: Fix PIC interrupt handling if APIC globally disabled

[Bernhard Beschow]

Am 3. Januar 2024 09:12:24 UTC schrieb "Alex Bennée" <alex.bennee@linaro.org>:
>Bernhard Beschow <shentey@gmail.com> writes:
>
>> QEMU populates the apic_state attribute of x86 CPUs if supported by real
>> hardware. Even when the APIC is globally disabled by a guest, this attribute
>> stays populated. This means that the APIC code paths are still used in this
>> case. However, chapter 10.4.3 of [1] requires that:
>>
>>   When IA32_APIC_BASE[11] is 0, the processor is functionally equivalent to an
>>   IA-32 processor without an on-chip APIC. The CPUID feature flag for the APIC
>>   [...] is also set to 0.
>>
>> Fix this by checking the APIC feature flag rather than apic_state when deciding
>> whether PIC or APIC behavior is required. This fixes some real-world BIOSes.
>>
>> Notice that presence of the CPUID_APIC flag implies that apic_state is non-NULL.
>>
>> [1] Intel 64 and IA-32 Architectures Software Developer's Manual, Vol. 3A:
>>     System Programming Guide, Part 1
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>  hw/i386/x86.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> index 2b6291ad8d..a753d1aeca 100644
>> --- a/hw/i386/x86.c
>> +++ b/hw/i386/x86.c
>> @@ -516,10 +516,10 @@ static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
>>      CPU_FOREACH(cs) {
>>          X86CPU *cpu = X86_CPU(cs);
>>  
>> -        if (!cpu->apic_state) {
>> -            cpu_interrupt(cs, CPU_INTERRUPT_NMI);
>> -        } else {
>> +        if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC) {
>
>You could assert the relationship between the feature and ->apic_state with:
>
>  g_assert(cpu->apic_state)
>
>But probably unnecessary in the grand scheme of things.

I like the idea so I'll respin.

Thanks,
Bernhard 

> Anyway:
>
>Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>