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

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

[Bernhard Beschow]

This 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 two patches fix
an issue regarding PIC interrupt handling, the third 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`

v2:
* Pick up R-b tag
* Split and rework interrupt handling patch to consider i486 SMP systems. This
    required dropping Alex' R-b tag.

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

Bernhard Beschow (3):
  hw/i386/x86: Reverse if statement
  hw/i386/x86: Fix PIC interrupt handling if APIC is globally disabled
  target/i386/cpu: Fix typo in comment

 include/hw/i386/apic.h |  1 +
 hw/i386/x86.c          |  8 ++++----
 hw/intc/apic_common.c  | 13 +++++++++++++
 target/i386/cpu.c      |  2 +-
 4 files changed, 19 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] hw/i386/x86: Reverse if statement

[Bernhard Beschow]

The if statement currently uses double negation when executing the else branch.
So swap the branches and simplify the condition to make the code more
comprehensible.

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

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 2b6291ad8d..61af705e90 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->apic_state) {
             apic_deliver_nmi(cpu->apic_state);
+        } else {
+            cpu_interrupt(cs, CPU_INTERRUPT_NMI);
         }
     }
 }
-- 
2.43.0

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

[Bernhard Beschow]

QEMU populates the apic_state attribute of x86 CPUs if supported by real
hardware or if SMP is active. When handling interrupts, it just checks whether
apic_state is populated to route the interrupt to the PIC or to the APIC.
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.

This means that when apic_state is populated, QEMU needs to check for the
MSR_IA32_APICBASE_ENABLE flag in addition. Implement this which fixes some
real-world BIOSes.

[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>
---
 include/hw/i386/apic.h |  1 +
 hw/i386/x86.c          |  4 ++--
 hw/intc/apic_common.c  | 13 +++++++++++++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
index bdc15a7a73..b2907c7179 100644
--- a/include/hw/i386/apic.h
+++ b/include/hw/i386/apic.h
@@ -11,6 +11,7 @@ void apic_deliver_nmi(DeviceState *d);
 int apic_get_interrupt(DeviceState *s);
 void cpu_set_apic_base(DeviceState *s, uint64_t val);
 uint64_t cpu_get_apic_base(DeviceState *s);
+bool cpu_is_apic_enabled(DeviceState *s);
 void cpu_set_apic_tpr(DeviceState *s, uint8_t val);
 uint8_t cpu_get_apic_tpr(DeviceState *s);
 void apic_init_reset(DeviceState *s);
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 61af705e90..16cd06c594 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -516,7 +516,7 @@ static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
     CPU_FOREACH(cs) {
         X86CPU *cpu = X86_CPU(cs);
 
-        if (cpu->apic_state) {
+        if (cpu_is_apic_enabled(cpu->apic_state)) {
             apic_deliver_nmi(cpu->apic_state);
         } else {
             cpu_interrupt(cs, CPU_INTERRUPT_NMI);
@@ -551,7 +551,7 @@ 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() &&
+    if (cpu_is_apic_enabled(cpu->apic_state) && !kvm_irqchip_in_kernel() &&
         !whpx_apic_in_platform()) {
         CPU_FOREACH(cs) {
             cpu = X86_CPU(cs);
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 6c100b48d6..bb5e916e9d 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -63,6 +63,19 @@ uint64_t cpu_get_apic_base(DeviceState *dev)
     }
 }
 
+bool cpu_is_apic_enabled(DeviceState *dev)
+{
+    APICCommonState *s;
+
+    if (!dev) {
+        return false;
+    }
+
+    s = APIC_COMMON(dev);
+
+    return s->apicbase & MSR_IA32_APICBASE_ENABLE;
+}
+
 void cpu_set_apic_tpr(DeviceState *dev, uint8_t val)
 {
     APICCommonState *s;
-- 
2.43.0

[PATCH v2 3/3] target/i386/cpu: Fix typo in comment

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 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 2524881ce2..7d11edf4fa 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2178,7 +2178,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 v2 0/3] Fix PIC interrupt handling of x86 CPUs if APIC is globally disabled

[Bernhard Beschow]

Am 6. Januar 2024 13:25:43 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>This 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 two patches fix
>
>an issue regarding PIC interrupt handling, the third 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`
>
>
>
>v2:
>
>* Pick up R-b tag
>
>* Split and rework interrupt handling patch to consider i486 SMP systems. This
>
>    required dropping Alex' R-b tag.
>

Ping

>
>
>[1] https://github.com/shentok/qemu/tree/via-apollo-pro-133t
>
>
>
>Bernhard Beschow (3):
>
>  hw/i386/x86: Reverse if statement
>
>  hw/i386/x86: Fix PIC interrupt handling if APIC is globally disabled
>
>  target/i386/cpu: Fix typo in comment
>
>
>
> include/hw/i386/apic.h |  1 +
>
> hw/i386/x86.c          |  8 ++++----
>
> hw/intc/apic_common.c  | 13 +++++++++++++
>
> target/i386/cpu.c      |  2 +-
>
> 4 files changed, 19 insertions(+), 5 deletions(-)
>
>
>
>-- >
>2.43.0
>
>
>

Re: [PATCH v2 0/3] Fix PIC interrupt handling of x86 CPUs if APIC is globally disabled

[Michael S. Tsirkin]

On Sun, Jan 14, 2024 at 10:52:28AM +0000, Bernhard Beschow wrote:
> 
> 
> Am 6. Januar 2024 13:25:43 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> >This 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 two patches fix
> >
> >an issue regarding PIC interrupt handling, the third 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`
> >
> >
> >
> >v2:
> >
> >* Pick up R-b tag
> >
> >* Split and rework interrupt handling patch to consider i486 SMP systems. This
> >
> >    required dropping Alex' R-b tag.
> >
> 
> Ping


Tagged now. Thanks!

> >
> >
> >[1] https://github.com/shentok/qemu/tree/via-apollo-pro-133t
> >
> >
> >
> >Bernhard Beschow (3):
> >
> >  hw/i386/x86: Reverse if statement
> >
> >  hw/i386/x86: Fix PIC interrupt handling if APIC is globally disabled
> >
> >  target/i386/cpu: Fix typo in comment
> >
> >
> >
> > include/hw/i386/apic.h |  1 +
> >
> > hw/i386/x86.c          |  8 ++++----
> >
> > hw/intc/apic_common.c  | 13 +++++++++++++
> >
> > target/i386/cpu.c      |  2 +-
> >
> > 4 files changed, 19 insertions(+), 5 deletions(-)
> >
> >
> >
> >-- >
> >2.43.0
> >
> >
> >