[PATCH v2 0/2] Fixes for "Windows fails to boot"

[PATCH v2 0/2] Fixes for "Windows fails to boot"

[Claudio Fontana]

v1 -> v2:
 * moved hyperv realizefn call before cpu expansion (Vitaly)
 * added more comments (Eduardo)
 * fixed references to commit ids (Eduardo)

The combination of Commits:
f5cc5a5c ("i386: split cpu accelerators from cpu.c"...)                                                                              
30565f10 ("cpu: call AccelCPUClass::cpu_realizefn in"...) 

introduced two bugs that break cpu max and host in the refactoring,
by running initializations in the wrong order.

This small series of two patches is an attempt to correct the situation.

Please provide your test results and feedback, thanks!

Claudio

Claudio Fontana (2):
  i386: reorder call to cpu_exec_realizefn in x86_cpu_realizefn
  i386: run accel_cpu_instance_init as instance_post_init

 target/i386/cpu.c         | 89 +++++++++++++++++++++++++--------------
 target/i386/kvm/kvm-cpu.c | 12 +++++-
 2 files changed, 68 insertions(+), 33 deletions(-)

-- 
2.26.2

[PATCH v2 1/2] i386: reorder call to cpu_exec_realizefn

[Claudio Fontana]

i386 realizefn code is sensitive to ordering, and recent commits
aimed at refactoring it, splitting accelerator-specific code,
broke assumptions which need to be fixed.

We need to:

* process hyper-v enlightements first, as they assume features
  not to be expanded

* only then, expand features

* after expanding features, attempt to check them and modify them in the
  accel-specific realizefn code called by cpu_exec_realizefn().

* after the framework has been called via cpu_exec_realizefn,
  the code can check for what has or hasn't been set by accel-specific
  code, or extend its results, ie:

  - check and evenually set code_urev default
  - modify cpu->mwait after potentially being set from host CPUID.
  - finally check for phys_bits assuming all user and accel-specific
    adjustments have already been taken into account.

Fixes: f5cc5a5c ("i386: split cpu accelerators from cpu.c"...)
Fixes: 30565f10 ("cpu: call AccelCPUClass::cpu_realizefn in"...)
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 target/i386/cpu.c         | 79 +++++++++++++++++++++++++--------------
 target/i386/kvm/kvm-cpu.c | 12 +++++-
 2 files changed, 61 insertions(+), 30 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 9e211ac2ce..f7eb5f7f6e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6133,39 +6133,17 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     Error *local_err = NULL;
     static bool ht_warned;
 
-    /* Process Hyper-V enlightenments */
-    x86_cpu_hyperv_realize(cpu);
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
-    if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
-        g_autofree char *name = x86_cpu_class_get_model_name(xcc);
-        error_setg(&local_err, "CPU model '%s' requires KVM or HVF", name);
-        goto out;
-    }
-
-    if (cpu->ucode_rev == 0) {
-        /* The default is the same as KVM's.  */
-        if (IS_AMD_CPU(env)) {
-            cpu->ucode_rev = 0x01000065;
-        } else {
-            cpu->ucode_rev = 0x100000000ULL;
-        }
-    }
-
-    /* mwait extended info: needed for Core compatibility */
-    /* We always wake on interrupt even if host does not have the capability */
-    cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE;
-
     if (cpu->apic_id == UNASSIGNED_APIC_ID) {
         error_setg(errp, "apic-id property was not initialized properly");
         return;
     }
 
+    /*
+     * Process Hyper-V enlightenments.
+     * Note: this currently has to happen before the expansion of CPU features.
+     */
+    x86_cpu_hyperv_realize(cpu);
+
     x86_cpu_expand_features(cpu, &local_err);
     if (local_err) {
         goto out;
@@ -6190,11 +6168,56 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
            & CPUID_EXT2_AMD_ALIASES);
     }
 
+    /*
+     * note: the call to the framework needs to happen after feature expansion,
+     * but before the checks/modifications to ucode_rev, mwait, phys_bits.
+     * These may be set by the accel-specific code,
+     * and the results are subsequently checked / assumed in this function.
+     */
+    cpu_exec_realizefn(cs, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
+        g_autofree char *name = x86_cpu_class_get_model_name(xcc);
+        error_setg(&local_err, "CPU model '%s' requires KVM or HVF", name);
+        goto out;
+    }
+
+    if (cpu->ucode_rev == 0) {
+        /*
+         * The default is the same as KVM's. Note that this check
+         * needs to happen after the evenual setting of ucode_rev in
+         * accel-specific code in cpu_exec_realizefn.
+         */
+        if (IS_AMD_CPU(env)) {
+            cpu->ucode_rev = 0x01000065;
+        } else {
+            cpu->ucode_rev = 0x100000000ULL;
+        }
+    }
+
+    /*
+     * mwait extended info: needed for Core compatibility
+     * We always wake on interrupt even if host does not have the capability.
+     *
+     * requires the accel-specific code in cpu_exec_realizefn to
+     * have already acquired the CPUID data into cpu->mwait.
+     */
+    cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE;
+
     /* For 64bit systems think about the number of physical bits to present.
      * ideally this should be the same as the host; anything other than matching
      * the host can cause incorrect guest behaviour.
      * QEMU used to pick the magic value of 40 bits that corresponds to
      * consumer AMD devices but nothing else.
+     *
+     * Note that this code assumes features expansion has already been done
+     * (as it checks for CPUID_EXT2_LM), and also assumes that potential
+     * phys_bits adjustments to match the host have been already done in
+     * accel-specific code in cpu_exec_realizefn.
      */
     if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
         if (cpu->phys_bits &&
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index c660ad4293..afd5f01413 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -26,10 +26,18 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
     /*
      * The realize order is important, since x86_cpu_realize() checks if
      * nothing else has been set by the user (or by accelerators) in
-     * cpu->ucode_rev and cpu->phys_bits.
+     * cpu->ucode_rev and cpu->phys_bits, and updates the CPUID results in
+     * mwait.ecx.
+     * This accel realization code also assumes cpu features are already expanded.
      *
      * realize order:
-     * kvm_cpu -> host_cpu -> x86_cpu
+     *
+     * x86_cpu_realize():
+     *  -> x86_cpu_expand_features()
+     *  -> cpu_exec_realizefn():
+     *            -> accel_cpu_realizefn()
+     *               kvm_cpu_realizefn() -> host_cpu_realizefn()
+     *  -> check/update ucode_rev, phys_bits, mwait
      */
     if (cpu->max_features) {
         if (enable_cpu_pm && kvm_has_waitpkg()) {
-- 
2.26.2

[PATCH v2 2/2] i386: run accel_cpu_instance_init as post_init

[Claudio Fontana]

This fixes host and max cpu initialization, by running the accel cpu
initialization only after all instance init functions are called for all
X86 cpu subclasses.

The bug this is fixing is related to the "max" and "host" i386 cpu
subclasses, which set cpu->max_features, which is then used at cpu
realization time.

In order to properly split the accel-specific max features code that
needs to be executed at cpu instance initialization time,

we cannot call the accel cpu initialization at the end of the x86 base
class initialization, or we will have no way to specialize
"max features" cpu behavior, overriding the "max" cpu class defaults,
and checking for the "max features" flag itself.

This patch moves the accel-specific cpu instance initialization to after
all x86 cpu instance code has been executed, including subclasses,

so that proper initialization of cpu "host" and "max" can be restored.

Fixes: f5cc5a5c ("i386: split cpu accelerators from cpu.c,"...)
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 target/i386/cpu.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index f7eb5f7f6e..35a857dc37 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6445,6 +6445,11 @@ static void x86_cpu_register_feature_bit_props(X86CPUClass *xcc,
     x86_cpu_register_bit_prop(xcc, name, w, bitnr);
 }
 
+static void x86_cpu_post_initfn(Object *obj)
+{
+    accel_cpu_instance_init(CPU(obj));
+}
+
 static void x86_cpu_initfn(Object *obj)
 {
     X86CPU *cpu = X86_CPU(obj);
@@ -6496,9 +6501,6 @@ static void x86_cpu_initfn(Object *obj)
     if (xcc->model) {
         x86_cpu_load_model(cpu, xcc->model);
     }
-
-    /* if required, do accelerator-specific cpu initializations */
-    accel_cpu_instance_init(CPU(obj));
 }
 
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
@@ -6833,6 +6835,8 @@ static const TypeInfo x86_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(X86CPU),
     .instance_init = x86_cpu_initfn,
+    .instance_post_init = x86_cpu_post_initfn,
+
     .abstract = true,
     .class_size = sizeof(X86CPUClass),
     .class_init = x86_cpu_common_class_init,
-- 
2.26.2

Re: [PATCH v2 0/2] Fixes for "Windows fails to boot"

[Claudio Fontana]

On 6/3/21 2:29 PM, Claudio Fontana wrote:
> v1 -> v2:
>  * moved hyperv realizefn call before cpu expansion (Vitaly)
>  * added more comments (Eduardo)
>  * fixed references to commit ids (Eduardo)
> 
> The combination of Commits:
> f5cc5a5c ("i386: split cpu accelerators from cpu.c"...)                                                                              
> 30565f10 ("cpu: call AccelCPUClass::cpu_realizefn in"...) 
> 
> introduced two bugs that break cpu max and host in the refactoring,
> by running initializations in the wrong order.
> 
> This small series of two patches is an attempt to correct the situation.
> 
> Please provide your test results and feedback, thanks!
> 
> Claudio
> 
> Claudio Fontana (2):
>   i386: reorder call to cpu_exec_realizefn in x86_cpu_realizefn
>   i386: run accel_cpu_instance_init as instance_post_init
> 
>  target/i386/cpu.c         | 89 +++++++++++++++++++++++++--------------
>  target/i386/kvm/kvm-cpu.c | 12 +++++-
>  2 files changed, 68 insertions(+), 33 deletions(-)
> 

Btw, CI/CD is all green, but as mentioned, it does not seem to catch these kind of issues.

https://gitlab.com/hw-claudio/qemu/-/pipelines/314286751

C.

Re: [PATCH v2 0/2] Fixes for "Windows fails to boot"

[Claudio Fontana]

On 6/3/21 5:10 PM, Cleber Rosa Junior wrote:
> On Thu, Jun 3, 2021 at 10:29 AM Claudio Fontana <cfontana@suse.de> wrote:
> 
>> On 6/3/21 2:29 PM, Claudio Fontana wrote:
>>> v1 -> v2:
>>>  * moved hyperv realizefn call before cpu expansion (Vitaly)
>>>  * added more comments (Eduardo)
>>>  * fixed references to commit ids (Eduardo)
>>>
>>> The combination of Commits:
>>> f5cc5a5c ("i386: split cpu accelerators from cpu.c"...)
>>
>>> 30565f10 ("cpu: call AccelCPUClass::cpu_realizefn in"...)
>>>
>>> introduced two bugs that break cpu max and host in the refactoring,
>>> by running initializations in the wrong order.
>>>
>>> This small series of two patches is an attempt to correct the situation.
>>>
>>> Please provide your test results and feedback, thanks!
>>>
>>> Claudio
>>>
>>> Claudio Fontana (2):
>>>   i386: reorder call to cpu_exec_realizefn in x86_cpu_realizefn
>>>   i386: run accel_cpu_instance_init as instance_post_init
>>>
>>>  target/i386/cpu.c         | 89 +++++++++++++++++++++++++--------------
>>>  target/i386/kvm/kvm-cpu.c | 12 +++++-
>>>  2 files changed, 68 insertions(+), 33 deletions(-)
>>>
>>
>> Btw, CI/CD is all green, but as mentioned, it does not seem to catch these
>> kind of issues.
>>
>>
> Hi Claudio,
> 
> Not familiar with the specifics of this bug, but can it be caught by
> attempting to boot an image other than Windows?  If so, we can consider
> adding a test along the lines of tests/acceptance/boot_linux_console.py.
> 
> Thanks,
> - Cleber.

Hello Cleber,

yes, all that seems to be required is the "host" cpu, q35 machine, and the firmware ./OVMF_CODE.secboot.fd and ./OVMF_VARS.secboot.fd :

./build/x86_64-softmmu/qemu-system-x86_64 \
        -cpu host \
        -enable-kvm \
        -m 4G \
        -machine q35,smm=on \
        -drive if=pflash,format=raw,readonly=on,unit=0,file="./OVMF_CODE.secboot.fd" \
        -drive if=pflash,format=raw,unit=1,file="./OVMF_VARS.secboot.fd"

With the bugged code, the firmware does not boot, and the cpu does not get into 64-bit long mode.
Applying the patches the firmware boots normally and we get the TianoCore Logo and text output.

Adding something like -display none -serial stdio would also generate text in the OK case that could be "expected" by a test:

BdsDxe: failed to load Boot0001 "UEFI QEMU DVD-ROM QM00005 " from PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x2,0xFFFF,0x0): Not Found

>>Start PXE over IPv4.

even without using any guest to boot at all, just the firmware.
I used this Fedora package for the test, containing the firmware: edk2-ovmf-20200801stable-1.fc33.noarch.rpm

I looked briefly at tests/acceptance/boot_linux_console.py, but did not see where such a test of firmware could be inserted,
could you advise?

Thanks,

Claudio

> 
> 
>> https://gitlab.com/hw-claudio/qemu/-/pipelines/314286751
>>
>> C.
>>
>>
>>
> 

Re: [PATCH v2 0/2] Fixes for "Windows fails to boot"

[Claudio Fontana]

On 6/4/21 8:32 AM, Claudio Fontana wrote:
> On 6/3/21 5:10 PM, Cleber Rosa Junior wrote:
>> On Thu, Jun 3, 2021 at 10:29 AM Claudio Fontana <cfontana@suse.de> wrote:
>>
>>> On 6/3/21 2:29 PM, Claudio Fontana wrote:
>>>> v1 -> v2:
>>>>  * moved hyperv realizefn call before cpu expansion (Vitaly)
>>>>  * added more comments (Eduardo)
>>>>  * fixed references to commit ids (Eduardo)
>>>>
>>>> The combination of Commits:
>>>> f5cc5a5c ("i386: split cpu accelerators from cpu.c"...)
>>>
>>>> 30565f10 ("cpu: call AccelCPUClass::cpu_realizefn in"...)
>>>>
>>>> introduced two bugs that break cpu max and host in the refactoring,
>>>> by running initializations in the wrong order.
>>>>
>>>> This small series of two patches is an attempt to correct the situation.
>>>>
>>>> Please provide your test results and feedback, thanks!
>>>>
>>>> Claudio
>>>>
>>>> Claudio Fontana (2):
>>>>   i386: reorder call to cpu_exec_realizefn in x86_cpu_realizefn
>>>>   i386: run accel_cpu_instance_init as instance_post_init
>>>>
>>>>  target/i386/cpu.c         | 89 +++++++++++++++++++++++++--------------
>>>>  target/i386/kvm/kvm-cpu.c | 12 +++++-
>>>>  2 files changed, 68 insertions(+), 33 deletions(-)
>>>>
>>>
>>> Btw, CI/CD is all green, but as mentioned, it does not seem to catch these
>>> kind of issues.
>>>
>>>
>> Hi Claudio,
>>
>> Not familiar with the specifics of this bug, but can it be caught by
>> attempting to boot an image other than Windows?  If so, we can consider
>> adding a test along the lines of tests/acceptance/boot_linux_console.py.
>>
>> Thanks,
>> - Cleber.
> 
> Hello Cleber,
> 
> yes, all that seems to be required is the "host" cpu, q35 machine, and the firmware ./OVMF_CODE.secboot.fd and ./OVMF_VARS.secboot.fd :
> 
> ./build/x86_64-softmmu/qemu-system-x86_64 \
>         -cpu host \
>         -enable-kvm \
>         -m 4G \
>         -machine q35,smm=on \
>         -drive if=pflash,format=raw,readonly=on,unit=0,file="./OVMF_CODE.secboot.fd" \
>         -drive if=pflash,format=raw,unit=1,file="./OVMF_VARS.secboot.fd"
> 
> With the bugged code, the firmware does not boot, and the cpu does not get into 64-bit long mode.
> Applying the patches the firmware boots normally and we get the TianoCore Logo and text output.
> 
> Adding something like -display none -serial stdio would also generate text in the OK case that could be "expected" by a test:
> 
> BdsDxe: failed to load Boot0001 "UEFI QEMU DVD-ROM QM00005 " from PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x2,0xFFFF,0x0): Not Found
> 
>>> Start PXE over IPv4.
> 
> even without using any guest to boot at all, just the firmware.
> I used this Fedora package for the test, containing the firmware: edk2-ovmf-20200801stable-1.fc33.noarch.rpm
> 
> I looked briefly at tests/acceptance/boot_linux_console.py, but did not see where such a test of firmware could be inserted,
> could you advise?


Nm I think I got it, will create a new boot_OVMF_fc33.py test.

Thanks, C


> 
>>
>>
>>> https://gitlab.com/hw-claudio/qemu/-/pipelines/314286751
>>>
>>> C.
>>>
>>>
>>>
>>
> 

Re: [PATCH v2 0/2] Fixes for "Windows fails to boot"

[Claudio Fontana]

On 6/4/21 9:01 AM, Claudio Fontana wrote:
> On 6/4/21 8:32 AM, Claudio Fontana wrote:
>> On 6/3/21 5:10 PM, Cleber Rosa Junior wrote:
>>> On Thu, Jun 3, 2021 at 10:29 AM Claudio Fontana <cfontana@suse.de> wrote:
>>>
>>>> On 6/3/21 2:29 PM, Claudio Fontana wrote:
>>>>> v1 -> v2:
>>>>>  * moved hyperv realizefn call before cpu expansion (Vitaly)
>>>>>  * added more comments (Eduardo)
>>>>>  * fixed references to commit ids (Eduardo)
>>>>>
>>>>> The combination of Commits:
>>>>> f5cc5a5c ("i386: split cpu accelerators from cpu.c"...)
>>>>
>>>>> 30565f10 ("cpu: call AccelCPUClass::cpu_realizefn in"...)
>>>>>
>>>>> introduced two bugs that break cpu max and host in the refactoring,
>>>>> by running initializations in the wrong order.
>>>>>
>>>>> This small series of two patches is an attempt to correct the situation.
>>>>>
>>>>> Please provide your test results and feedback, thanks!
>>>>>
>>>>> Claudio
>>>>>
>>>>> Claudio Fontana (2):
>>>>>   i386: reorder call to cpu_exec_realizefn in x86_cpu_realizefn
>>>>>   i386: run accel_cpu_instance_init as instance_post_init
>>>>>
>>>>>  target/i386/cpu.c         | 89 +++++++++++++++++++++++++--------------
>>>>>  target/i386/kvm/kvm-cpu.c | 12 +++++-
>>>>>  2 files changed, 68 insertions(+), 33 deletions(-)
>>>>>
>>>>
>>>> Btw, CI/CD is all green, but as mentioned, it does not seem to catch these
>>>> kind of issues.
>>>>
>>>>
>>> Hi Claudio,
>>>
>>> Not familiar with the specifics of this bug, but can it be caught by
>>> attempting to boot an image other than Windows?  If so, we can consider
>>> adding a test along the lines of tests/acceptance/boot_linux_console.py.
>>>
>>> Thanks,
>>> - Cleber.
>>
>> Hello Cleber,
>>
>> yes, all that seems to be required is the "host" cpu, q35 machine, and the firmware ./OVMF_CODE.secboot.fd and ./OVMF_VARS.secboot.fd :
>>
>> ./build/x86_64-softmmu/qemu-system-x86_64 \
>>         -cpu host \
>>         -enable-kvm \
>>         -m 4G \
>>         -machine q35,smm=on \
>>         -drive if=pflash,format=raw,readonly=on,unit=0,file="./OVMF_CODE.secboot.fd" \
>>         -drive if=pflash,format=raw,unit=1,file="./OVMF_VARS.secboot.fd"
>>
>> With the bugged code, the firmware does not boot, and the cpu does not get into 64-bit long mode.
>> Applying the patches the firmware boots normally and we get the TianoCore Logo and text output.
>>
>> Adding something like -display none -serial stdio would also generate text in the OK case that could be "expected" by a test:
>>
>> BdsDxe: failed to load Boot0001 "UEFI QEMU DVD-ROM QM00005 " from PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x2,0xFFFF,0x0): Not Found
>>
>>>> Start PXE over IPv4.
>>
>> even without using any guest to boot at all, just the firmware.
>> I used this Fedora package for the test, containing the firmware: edk2-ovmf-20200801stable-1.fc33.noarch.rpm
>>
>> I looked briefly at tests/acceptance/boot_linux_console.py, but did not see where such a test of firmware could be inserted,
>> could you advise?
> 
> 
> Nm I think I got it, will create a new boot_OVMF_fc33.py test.
> 
> Thanks, C

Question: do all of these acceptance tests require manual launch?

At least this is what I got in my CI at:

https://gitlab.com/hw-claudio/qemu

I seem to have to explicitly click on the acceptance tests to manually launch them.
Or am I missing something?

Thanks,

Claudio