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

Claudio Fontana posted 2 patches 2 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/next-importer-push tags/patchew/20210603123001.17843-1-cfontana@suse.de
Maintainers: Marcelo Tosatti <mtosatti@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
target/i386/cpu.c         | 89 +++++++++++++++++++++++++--------------
target/i386/kvm/kvm-cpu.c | 12 +++++-
2 files changed, 68 insertions(+), 33 deletions(-)
[PATCH v2 0/2] Fixes for "Windows fails to boot"
Posted by Claudio Fontana 2 years, 9 months ago
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


Re: [PATCH v2 0/2] Fixes for "Windows fails to boot"
Posted by Claudio Fontana 2 years, 9 months ago
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"
Posted by Cleber Rosa Junior 2 years, 9 months ago
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.


> https://gitlab.com/hw-claudio/qemu/-/pipelines/314286751
>
> C.
>
>
>
Re: [PATCH v2 0/2] Fixes for "Windows fails to boot"
Posted by Claudio Fontana 2 years, 9 months ago
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"
Posted by Claudio Fontana 2 years, 9 months ago
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"
Posted by Claudio Fontana 2 years, 9 months ago
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