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

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

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

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


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