Re: [Qemu-devel] [PATCH] kvm: fix Win2k boot without KVM

[Jan Kiszka <jan.kiszka@web.de>]

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

On 2012-11-03 21:57, Jan Kiszka wrote:
> On 2012-11-03 20:37, Jan Kiszka wrote:
>> On 2012-11-03 20:26, Blue Swirl wrote:
>>> On Sat, Nov 3, 2012 at 7:10 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> On 2012-11-03 20:03, Jan Kiszka wrote:
>>>>> On 2012-11-03 19:56, Blue Swirl wrote:
>>>>>> On Sat, Nov 3, 2012 at 6:51 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>>>> On 2012-11-03 19:49, Blue Swirl wrote:
>>>>>>>> Ignore accesses to VAPIC when kvmvapic is not enabled.
>>>>>>>>
>>>>>>>> Cc: Jan Kiszka <jan.kiszka@web.de>
>>>>>>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>>>>>>>> ---
>>>>>>>>  hw/kvmvapic.c |    7 ++++---
>>>>>>>>  1 files changed, 4 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/kvmvapic.c b/hw/kvmvapic.c
>>>>>>>> index dc111ee..a97d532 100644
>>>>>>>> --- a/hw/kvmvapic.c
>>>>>>>> +++ b/hw/kvmvapic.c
>>>>>>>> @@ -612,6 +612,9 @@ static void vapic_write(void *opaque, hwaddr addr, uint64_t data,
>>>>>>>>      hwaddr rom_paddr;
>>>>>>>>      VAPICROMState *s = opaque;
>>>>>>>>
>>>>>>>> +    if (!kvm_irqchip_in_kernel()) {
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>>      cpu_synchronize_state(env);
>>>>>>>>
>>>>>>>>      /*
>>>>>>>> @@ -665,9 +668,7 @@ static void vapic_write(void *opaque, hwaddr addr, uint64_t data,
>>>>>>>>          break;
>>>>>>>>      default:
>>>>>>>>      case 4:
>>>>>>>> -        if (!kvm_irqchip_in_kernel()) {
>>>>>>>> -            apic_poll_irq(env->apic_state);
>>>>>>>> -        }
>>>>>>>> +        apic_poll_irq(env->apic_state);
>>>>>>>>          break;
>>>>>>>>      }
>>>>>>>>  }
>>>>>>>>
>>>>>>>
>>>>>>> NACK, I'm already debugging the true reason (related to code patching).
>>>>>>
>>>>>> This is a minimal fix that lets Win2k boot, now it does not work at
>>>>>> all. I think it should be applied for 1.3, it can be reverted when
>>>>>> (if) you find a better fix. There's no hurry though.
>>>>>
>>>>> If you want to disable it, flip apic.vapic for !kvm_enabled. Your patch
>>>>> affects user space APIC with KVM as well, though that is perfectly fine.
>>>>>
>>>>> But first of all give this some days as I just started.
>>>>
>>>> ...even more as this regression may not be related to the introduction
>>>> of the kvmvapic: My original test case for the kvmvapic under TCG,
>>>> WinXP, is now also broken, causing a segfault too.
>>>>
>>>> What I'm seeing is that tb_invalidate_phys_page_range in
>>>> patch_instruction no longer seems to detect that the currently executed
>>>> tb was just changed. Any ideas what may cause this are welcome.
>>>
>>> My theory is that the kvmvapic ROM tries to make the PIO hypercalls,
>>> but the PIO devices in QEMU are not ready, maybe not initialized at
>>> all.
>>
>> You are on the wrong track: All is set up, the first TPR accesses are
>> happening, and the kvmvapic is trying to patch them away. In TCG mode,
>> this requires a flush of the current TB afterward. And this somehow
>> fails, causing various issues as executing resumes at invalid addresses.
>>
>> Jan
>>
>> PS: A good hash is e.g. b34bd5e5c8f356ec206e5a306ee3a9b6f42c4315, long
>> after the kvmvapic merge.
>>
>> PPS: Bisecting in QEMU is no fun - too many transient build breakages.
>>
> 
> Bisection ended here: 0b57e287138728f72d88b06e69b970c5d745c44a
> (cpu_physical_memory_write_rom() needs to do TB invalidates).
> 
> The problem is that this invalidation happens with
> is_cpu_write_access=0, "consuming" the current TB for the final
> invalidation in patch_instruction with is_cpu_write_access=1. So,
> instead of generating a new TB and resuming execution there, the guest
> jumps back to an invalidated TB.
> 
> I have a patch, but it moves half of tb_invalidate_phys_page_range to
> patch_instruction. Need to check if that can be solved cleaner.
> 
> Jan
> 

Unrelated, just to note it:

The boot-up screen of Win2k is drawn much slower with git head than with
1.2 e.g. My bet is: some regression of recent memory region changes.
Requires another bisection round... :-/

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]