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

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

[Blue Swirl]

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;
     }
 }
-- 
1.7.2.5

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

[Jan Kiszka]

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

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).

Jan


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

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

[Blue Swirl]

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.

>
> Jan
>

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

[Jan Kiszka]

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

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.

Jan



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

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

[Blue Swirl]

On Sat, Nov 3, 2012 at 7:03 PM, Jan Kiszka <jan.kiszka@web.de> 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.

I thought about that, but kvm_enabled and kvm_kernel_irqchip may not
be initialized yet during vapic_init().

>
> But first of all give this some days as I just started.

Sure.

>
> Jan
>
>

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

[Jan Kiszka]

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

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.

Jan


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

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

[Blue Swirl]

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.

>
> Jan
>

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

[Jan Kiszka]

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

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.


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

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

[Jan Kiszka]

[-- 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 --]

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

[Jan Kiszka]

[-- 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 --]