[Qemu-devel] [PATCH 0/2] target-i386: disable LINT0 after reset and init

[Qemu-devel] [PATCH 0/2] target-i386: disable LINT0 after reset and init

[Nadav Amit]

LINT0 is currently reenabled after reset to circumvent old seabios bug, which
violates x86 specifications.  This patch-set handles this issue, by removing
the old hack from qemu and reporting to kvm that this quirk is no longer
needed.  In addition, we disable another kvm quirk that clears CD and NW from
CR0 after reset.

Thanks for reviewing these patches.

Nadav Amit (2):
  target-i386: disable LINT0 after reset
  target-i386: kvm: Disable KVM quirks

 hw/intc/apic_common.c       | 9 ---------
 linux-headers/asm-x86/kvm.h | 4 ++++
 linux-headers/linux/kvm.h   | 1 +
 target-i386/kvm.c           | 8 ++++++++
 4 files changed, 13 insertions(+), 9 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] target-i386: disable LINT0 after reset

[Nadav Amit]

Due to old Seabios bug, QEMU reenable LINT0 after reset. This bug is long gone
and therefore this hack is no longer needed.  Since it violates the
specifications, it is removed.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 hw/intc/apic_common.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 042e960..d38d24b 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -243,15 +243,6 @@ static void apic_reset_common(DeviceState *dev)
     info->vapic_base_update(s);
 
     apic_init_reset(dev);
-
-    if (bsp) {
-        /*
-         * LINT0 delivery mode on CPU #0 is set to ExtInt at initialization
-         * time typically by BIOS, so PIC interrupt can be delivered to the
-         * processor when local APIC is enabled.
-         */
-        s->lvt[APIC_LVT_LINT0] = 0x700;
-    }
 }
 
 /* This function is only used for old state version 1 and 2 */
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/2] target-i386: disable LINT0 after reset

[Paolo Bonzini]

On 13/04/2015 01:32, Nadav Amit wrote:
> Due to old Seabios bug, QEMU reenable LINT0 after reset. This bug is long gone
> and therefore this hack is no longer needed.  Since it violates the
> specifications, it is removed.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  hw/intc/apic_common.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 042e960..d38d24b 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -243,15 +243,6 @@ static void apic_reset_common(DeviceState *dev)
>      info->vapic_base_update(s);
>  
>      apic_init_reset(dev);
> -
> -    if (bsp) {
> -        /*
> -         * LINT0 delivery mode on CPU #0 is set to ExtInt at initialization
> -         * time typically by BIOS, so PIC interrupt can be delivered to the
> -         * processor when local APIC is enabled.
> -         */
> -        s->lvt[APIC_LVT_LINT0] = 0x700;
> -    }
>  }
>  
>  /* This function is only used for old state version 1 and 2 */
> 

Thanks, applied this one.  The other will have to wait for a bit, since
it depends on a patch that is destined to Linux 4.2.

Paolo

Re: [Qemu-devel] [PATCH 1/2] target-i386: disable LINT0 after reset

[Alex Williamson]

On Mon, 2015-04-13 at 02:32 +0300, Nadav Amit wrote:
> Due to old Seabios bug, QEMU reenable LINT0 after reset. This bug is long gone
> and therefore this hack is no longer needed.  Since it violates the
> specifications, it is removed.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  hw/intc/apic_common.c | 9 ---------
>  1 file changed, 9 deletions(-)

Please see bug: https://bugs.launchpad.net/qemu/+bug/1488363

Is this bug perhaps not as long gone as we thought, or is there
something else going on here?  Thanks,

Alex

> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 042e960..d38d24b 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -243,15 +243,6 @@ static void apic_reset_common(DeviceState *dev)
>      info->vapic_base_update(s);
>  
>      apic_init_reset(dev);
> -
> -    if (bsp) {
> -        /*
> -         * LINT0 delivery mode on CPU #0 is set to ExtInt at initialization
> -         * time typically by BIOS, so PIC interrupt can be delivered to the
> -         * processor when local APIC is enabled.
> -         */
> -        s->lvt[APIC_LVT_LINT0] = 0x700;
> -    }
>  }
>  
>  /* This function is only used for old state version 1 and 2 */

Re: [Qemu-devel] [PATCH 1/2] target-i386: disable LINT0 after reset

[Jan Kiszka]

On 2015-09-15 23:19, Alex Williamson wrote:
> On Mon, 2015-04-13 at 02:32 +0300, Nadav Amit wrote:
>> Due to old Seabios bug, QEMU reenable LINT0 after reset. This bug is long gone
>> and therefore this hack is no longer needed.  Since it violates the
>> specifications, it is removed.
>>
>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>> ---
>>  hw/intc/apic_common.c | 9 ---------
>>  1 file changed, 9 deletions(-)
> 
> Please see bug: https://bugs.launchpad.net/qemu/+bug/1488363
> 
> Is this bug perhaps not as long gone as we thought, or is there
> something else going on here?  Thanks,

I would say, someone needs to check if the SeaBIOS line that is supposed
to enable LINT0 is actually executed on one of the broken systems and,
if not, why not.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/2] target-i386: disable LINT0 after reset

[Nadav Amit]

[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]

I'll try to reproduce the problem today.

Nadav
On Sep 16, 2015 8:23 AM, "Jan Kiszka" <jan.kiszka@siemens.com> wrote:

> On 2015-09-15 23:19, Alex Williamson wrote:
> > On Mon, 2015-04-13 at 02:32 +0300, Nadav Amit wrote:
> >> Due to old Seabios bug, QEMU reenable LINT0 after reset. This bug is
> long gone
> >> and therefore this hack is no longer needed.  Since it violates the
> >> specifications, it is removed.
> >>
> >> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> >> ---
> >>  hw/intc/apic_common.c | 9 ---------
> >>  1 file changed, 9 deletions(-)
> >
> > Please see bug: https://bugs.launchpad.net/qemu/+bug/1488363
> >
> > Is this bug perhaps not as long gone as we thought, or is there
> > something else going on here?  Thanks,
>
> I would say, someone needs to check if the SeaBIOS line that is supposed
> to enable LINT0 is actually executed on one of the broken systems and,
> if not, why not.
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SES-DE
> Corporate Competence Center Embedded Linux
>

[-- Attachment #2: Type: text/html, Size: 1591 bytes --]

Re: [Qemu-devel] [PATCH 1/2] target-i386: disable LINT0 after reset

[Gerd Hoffmann]

On Mi, 2015-09-16 at 07:23 +0200, Jan Kiszka wrote:
> On 2015-09-15 23:19, Alex Williamson wrote:
> > On Mon, 2015-04-13 at 02:32 +0300, Nadav Amit wrote:
> >> Due to old Seabios bug, QEMU reenable LINT0 after reset. This bug is long gone
> >> and therefore this hack is no longer needed.  Since it violates the
> >> specifications, it is removed.
> >>
> >> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> >> ---
> >>  hw/intc/apic_common.c | 9 ---------
> >>  1 file changed, 9 deletions(-)
> > 
> > Please see bug: https://bugs.launchpad.net/qemu/+bug/1488363
> > 
> > Is this bug perhaps not as long gone as we thought, or is there
> > something else going on here?  Thanks,
> 
> I would say, someone needs to check if the SeaBIOS line that is supposed
> to enable LINT0 is actually executed on one of the broken systems and,
> if not, why not.

There is only one reason (beside miscompiling seabios with
CONFIG_QEMU=n) why seabios would skip acpi initialization, and that is
apic not being present according to cpuid:

    cpuid(1, &eax, &ebx, &ecx, &cpuid_features);
    if (eax < 1 || !(cpuid_features & CPUID_APIC)) {
        // No apic - only the main cpu is present.

https://www.kraxel.org/cgit/seabios/tree/src/fw/smp.c#n79

cheers,
  Gerd

PS: coreboot tripped over this too, fixed just a few days ago.

Re: [Qemu-devel] [PATCH 1/2] target-i386: disable LINT0 after reset

[Nadav Amit]

I don’t happen to have a similar platform. On regular qemu/kvm runs with
q35, I see APIC_LVT0 is set once to 0x8700 on the BSP - as expected:

 qemu-system-x86-19345 [011] d... 2583274.503018: kvm_entry: vcpu 0
 qemu-system-x86-19345 [011] d... 2583274.503019: kvm_exit: reason APIC_ACCESS rip 0x7ffb8288 info 1350 0
 qemu-system-x86-19345 [011] .... 2583274.503020: kvm_emulate_insn: 0:7ffb8288:c7 05 50 03 e0 fe 00 87 00 00 (prot32)
 qemu-system-x86-19345 [011] .... 2583274.503021: kvm_mmio: mmio write len 4 gpa 0xfee00350 val 0x8700
 qemu-system-x86-19345 [011] .... 2583274.503021: kvm_apic: apic_write APIC_LVT0 = 0x8700

If someone sends a trace ( http://www.linux-kvm.org/page/Tracing ) of the
failure, I would be happy to assist.

Nadav

Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mi, 2015-09-16 at 07:23 +0200, Jan Kiszka wrote:
>> On 2015-09-15 23:19, Alex Williamson wrote:
>>> On Mon, 2015-04-13 at 02:32 +0300, Nadav Amit wrote:
>>>> Due to old Seabios bug, QEMU reenable LINT0 after reset. This bug is long gone
>>>> and therefore this hack is no longer needed.  Since it violates the
>>>> specifications, it is removed.
>>>> 
>>>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>>>> ---
>>>> hw/intc/apic_common.c | 9 ---------
>>>> 1 file changed, 9 deletions(-)
>>> 
>>> Please see bug: https://bugs.launchpad.net/qemu/+bug/1488363
>>> 
>>> Is this bug perhaps not as long gone as we thought, or is there
>>> something else going on here?  Thanks,
>> 
>> I would say, someone needs to check if the SeaBIOS line that is supposed
>> to enable LINT0 is actually executed on one of the broken systems and,
>> if not, why not.
> 
> There is only one reason (beside miscompiling seabios with
> CONFIG_QEMU=n) why seabios would skip acpi initialization, and that is
> apic not being present according to cpuid:
> 
>    cpuid(1, &eax, &ebx, &ecx, &cpuid_features);
>    if (eax < 1 || !(cpuid_features & CPUID_APIC)) {
>        // No apic - only the main cpu is present.
> 
> https://www.kraxel.org/cgit/seabios/tree/src/fw/smp.c#n79
> 
> cheers,
>  Gerd
> 
> PS: coreboot tripped over this too, fixed just a few days ago.

[Qemu-devel] [PATCH 2/2] target-i386: kvm: Disable KVM quirks

[Nadav Amit]

KVM has quirks to overcome legacy QEMU bugs that are already resolved.  Using a
new KVM feature for disabling these quirks.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 linux-headers/asm-x86/kvm.h | 4 ++++
 linux-headers/linux/kvm.h   | 1 +
 target-i386/kvm.c           | 8 ++++++++
 3 files changed, 13 insertions(+)

diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h
index d7dcef5..f8fbb4a 100644
--- a/linux-headers/asm-x86/kvm.h
+++ b/linux-headers/asm-x86/kvm.h
@@ -345,4 +345,8 @@ struct kvm_xcrs {
 struct kvm_sync_regs {
 };
 
+/* KVM legacy quirks for use with KVM_CAP_DISABLE_QUIRKS */
+#define KVM_QUIRK_LINT0_DISABLED	(1 << 0)
+#define KVM_QUIRK_CD_NW_CLEARED		(1 << 1)
+
 #endif /* _ASM_X86_KVM_H */
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 60a54c8..757e869 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -760,6 +760,7 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_PPC_ENABLE_HCALL 104
 #define KVM_CAP_CHECK_EXTENSION_VM 105
 #define KVM_CAP_S390_USER_SIGP 106
+#define KVM_CAP_DISABLE_QUIRKS 115
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 41d09e5..3b28122 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -898,6 +898,14 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
             return ret;
         }
     }
+    if (kvm_check_extension(s, KVM_CAP_ENABLE_CAP_VM)) {
+        ret = kvm_vm_enable_cap(s, KVM_CAP_DISABLE_QUIRKS, 0,
+                                KVM_QUIRK_LINT0_DISABLED |
+                                KVM_QUIRK_CD_NW_CLEARED);
+        if (ret < 0) {
+            return ret;
+        }
+    }
     return 0;
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/2] target-i386: kvm: Disable KVM quirks

[Paolo Bonzini]

On 13/04/2015 01:32, Nadav Amit wrote:
> +    if (kvm_check_extension(s, KVM_CAP_ENABLE_CAP_VM)) {

The right capability to check here is KVM_CAP_DISABLE_QUIRKS, not
KVM_CAP_ENABLE_CAP_VM.

Paolo

> +        ret = kvm_vm_enable_cap(s, KVM_CAP_DISABLE_QUIRKS, 0,
> +                                KVM_QUIRK_LINT0_DISABLED |
> +                                KVM_QUIRK_CD_NW_CLEARED);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }

Re: [Qemu-devel] [PATCH 2/2] target-i386: kvm: Disable KVM quirks

[Nadav Amit]

Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 13/04/2015 01:32, Nadav Amit wrote:
>> +    if (kvm_check_extension(s, KVM_CAP_ENABLE_CAP_VM)) {
> 
> The right capability to check here is KVM_CAP_DISABLE_QUIRKS, not
> KVM_CAP_ENABLE_CAP_VM.
> 
> Paolo
> 
>> +        ret = kvm_vm_enable_cap(s, KVM_CAP_DISABLE_QUIRKS, 0,
>> +                                KVM_QUIRK_LINT0_DISABLED |
>> +                                KVM_QUIRK_CD_NW_CLEARED);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>> +    }

Of course…
Do you want a v2 now, later (after 4.2), or would you change it yourself?

Thanks,
Nadav

Re: [Qemu-devel] [PATCH 2/2] target-i386: kvm: Disable KVM quirks

[Paolo Bonzini]

On 13/04/2015 16:17, Nadav Amit wrote:
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>>
>>
>> On 13/04/2015 01:32, Nadav Amit wrote:
>>> +    if (kvm_check_extension(s, KVM_CAP_ENABLE_CAP_VM)) {
>>
>> The right capability to check here is KVM_CAP_DISABLE_QUIRKS, not
>> KVM_CAP_ENABLE_CAP_VM.
>>
>> Paolo
>>
>>> +        ret = kvm_vm_enable_cap(s, KVM_CAP_DISABLE_QUIRKS, 0,
>>> +                                KVM_QUIRK_LINT0_DISABLED |
>>> +                                KVM_QUIRK_CD_NW_CLEARED);
>>> +        if (ret < 0) {
>>> +            return ret;
>>> +        }
>>> +    }
> 
> Of course…
> Do you want a v2 now, later (after 4.2), or would you change it yourself?

Later, close to 4.2 but not necessarily after it.

Paolo

Re: [Qemu-devel] [PATCH 2/2] target-i386: kvm: Disable KVM quirks

[Nadav Amit]

Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 13/04/2015 16:17, Nadav Amit wrote:
>> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> 
>>> On 13/04/2015 01:32, Nadav Amit wrote:
>>>> +    if (kvm_check_extension(s, KVM_CAP_ENABLE_CAP_VM)) {
>>> 
>>> The right capability to check here is KVM_CAP_DISABLE_QUIRKS, not
>>> KVM_CAP_ENABLE_CAP_VM.
>>> 
>>> Paolo
>>> 
>>>> +        ret = kvm_vm_enable_cap(s, KVM_CAP_DISABLE_QUIRKS, 0,
>>>> +                                KVM_QUIRK_LINT0_DISABLED |
>>>> +                                KVM_QUIRK_CD_NW_CLEARED);
>>>> +        if (ret < 0) {
>>>> +            return ret;
>>>> +        }
>>>> +    }
>> 
>> Of course…
>> Do you want a v2 now, later (after 4.2), or would you change it yourself?
> 
> Later, close to 4.2 but not necessarily after it.

Anyhow, in that case the KVM patch is also wrong (not reporting
KVM_CAP_DISABLE_QUIRKS is supported). I don’t want to spam, so I’ll run some
tests and resubmit.

Nadav

Re: [Qemu-devel] [PATCH 2/2] target-i386: kvm: Disable KVM quirks

[Paolo Bonzini]

On 13/04/2015 16:33, Nadav Amit wrote:
>>> >> Of course…
>>> >> Do you want a v2 now, later (after 4.2), or would you change it yourself?
>> > 
>> > Later, close to 4.2 but not necessarily after it.
> Anyhow, in that case the KVM patch is also wrong (not reporting
> KVM_CAP_DISABLE_QUIRKS is supported). I don’t want to spam, so I’ll run some
> tests and resubmit.

Don't worry, I've fixed that already.

Paolo