[PATCH] kvm-pr: manage illegal instructions
Thomas Huth
thuth at redhat.com
Thu May 19 17:11:53 AEST 2016
On 18.05.2016 12:53, Thomas Huth wrote:
> On 18.05.2016 12:18, Thomas Huth wrote:
>> On 17.05.2016 19:49, Laurent Vivier wrote:
>>>
>>>
>>> On 17/05/2016 10:37, Alexander Graf wrote:
>>>> On 05/17/2016 10:35 AM, Laurent Vivier wrote:
>>>>>
>>>>> On 12/05/2016 16:23, Laurent Vivier wrote:
>>>>>>
>>>>>> On 12/05/2016 11:27, Alexander Graf wrote:
>>>>>>> On 05/12/2016 11:10 AM, Laurent Vivier wrote:
>>>>>>>> On 11/05/2016 13:49, Alexander Graf wrote:
>>>>>>>>> On 05/11/2016 01:14 PM, Laurent Vivier wrote:
>>>>>>>>>> On 11/05/2016 12:35, Alexander Graf wrote:
>>>>>>>>>>> On 03/15/2016 09:18 PM, Laurent Vivier wrote:
>>>>>>>>>>>> While writing some instruction tests for kvm-unit-tests for
>>>>>>>>>>>> powerpc,
>>>>>>>>>>>> I've found that illegal instructions are not managed correctly
>>>>>>>>>>>> with
>>>>>>>>>>>> kvm-pr,
>>>>>>>>>>>> while it is fine with kvm-hv.
>>>>>>>>>>>>
>>>>>>>>>>>> When an illegal instruction (like ".long 0") is processed by
>>>>>>>>>>>> kvm-pr,
>>>>>>>>>>>> the kernel logs are filled with:
>>>>>>>>>>>>
>>>>>>>>>>>> Couldn't emulate instruction 0x00000000 (op 0 xop 0)
>>>>>>>>>>>> kvmppc_handle_exit_pr: emulation at 700 failed (00000000)
>>>>>>>>>>>>
>>>>>>>>>>>> While the exception handler receives an interrupt for each
>>>>>>>>>>>> instruction
>>>>>>>>>>>> executed after the illegal instruction.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Laurent Vivier <lvivier at redhat.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>> arch/powerpc/kvm/book3s_emulate.c | 4 +++-
>>>>>>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/arch/powerpc/kvm/book3s_emulate.c
>>>>>>>>>>>> b/arch/powerpc/kvm/book3s_emulate.c
>>>>>>>>>>>> index 2afdb9c..4ee969d 100644
>>>>>>>>>>>> --- a/arch/powerpc/kvm/book3s_emulate.c
>>>>>>>>>>>> +++ b/arch/powerpc/kvm/book3s_emulate.c
>>>>>>>>>>>> @@ -99,7 +99,6 @@ int kvmppc_core_emulate_op_pr(struct kvm_run
>>>>>>>>>>>> *run,
>>>>>>>>>>>> struct kvm_vcpu *vcpu,
>>>>>>>>>>>> switch (get_op(inst)) {
>>>>>>>>>>>> case 0:
>>>>>>>>>>>> - emulated = EMULATE_FAIL;
>>>>>>>>>>>> if ((kvmppc_get_msr(vcpu) & MSR_LE) &&
>>>>>>>>>>>> (inst == swab32(inst_sc))) {
>>>>>>>>>>>> /*
>>>>>>>>>>>> @@ -112,6 +111,9 @@ int kvmppc_core_emulate_op_pr(struct kvm_run
>>>>>>>>>>>> *run,
>>>>>>>>>>>> struct kvm_vcpu *vcpu,
>>>>>>>>>>>> kvmppc_set_gpr(vcpu, 3, EV_UNIMPLEMENTED);
>>>>>>>>>>>> kvmppc_set_pc(vcpu, kvmppc_get_pc(vcpu) + 4);
>>>>>>>>>>>> emulated = EMULATE_DONE;
>>>>>>>>>>>> + } else {
>>>>>>>>>>>> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>>>>>>>>>>> But isn't that exactly what the semantic of EMULATE_FAIL is?
>>>>>>>>>>> Fixing it
>>>>>>>>>>> up in book3s_emulate.c is definitely the wrong spot.
>>>>>>>>>>>
>>>>>>>>>>> So what is the problem you're trying to solve? Is the SRR0 at the
>>>>>>>>>>> wrong
>>>>>>>>>>> spot or are the log messages the problem?
>>>>>>>>>> No, the problem is the host kernel logs are filled by the message
>>>>>>>>>> and
>>>>>>>>>> the execution hangs. And the host becomes unresponsiveness, even
>>>>>>>>>> after
>>>>>>>>>> the end of the tests.
>>>>>>>>>>
>>>>>>>>>> Please, try to run kvm-unit-tests (the emulator test) on a KVM-PR
>>>>>>>>>> host,
>>>>>>>>>> and check the kernel logs (dmesg), then try to ssh to the host...
>>>>>>>>> Ok, so the log messages are the problem. Please fix the message
>>>>>>>>> output
>>>>>>>>> then - or remove it altogether. Or if you like, create a module
>>>>>>>>> parameter that allows you to emit them.
>>>>>>>>>
>>>>>>>>> I personally think the best solution would be to just convert the
>>>>>>>>> message into a trace point.
>>>>>>>>>
>>>>>>>>> While at it, please see whether the guest can trigger similar host
>>>>>>>>> log
>>>>>>>>> output excess in other code paths.
>>>>>>>> The problem is not really with the log messages: they are
>>>>>>>> consequence of
>>>>>>>> the bug I try to fix.
>>>>>>>>
>>>>>>>> What happens is once kvm_pr decodes an invalid instruction all the
>>>>>>>> valid
>>>>>>>> following instructions trigger a Program exception to the guest
>>>>>>>> (but are
>>>>>>>> executed correctly). It has no real consequence on big machine like
>>>>>>>> POWER8, except that the guest become very slow and the log files of
>>>>>>>> the
>>>>>>>> host are filled with messages (and qemu uses 100% of the CPU). On a
>>>>>>>> smaller machine like a PowerMac G5, the machine becomes simply
>>>>>>>> unusable.
>>>>>>> It's probably more related to your verbosity level of kernel messages.
>>>>>>> If you pass loglevel=0 (or quiet) to you kernel cmdline you won't get
>>>>>>> the messages printed to serial which is what's slowing you down.
>>>>>>>
>>>>>>> The other problem sounds pretty severe, but the only thing your patch
>>>>>>> does any different from the current code flow would be the patch below.
>>>>>>> Or did I miss anything?
>>>>>>>
>>>>>>> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
>>>>>>> index 5cc2e7a..4672bc2 100644
>>>>>>> --- a/arch/powerpc/kvm/emulate.c
>>>>>>> +++ b/arch/powerpc/kvm/emulate.c
>>>>>>> @@ -302,7 +302,11 @@ int kvmppc_emulate_instruction(struct kvm_run
>>>>>>> *run,
>>>>>>> struct kvm_vcpu *vcpu)
>>>>>>> advance = 0;
>>>>>>> printk(KERN_ERR "Couldn't emulate instruction
>>>>>>> 0x%08x "
>>>>>>> "(op %d xop %d)\n", inst, get_op(inst),
>>>>>>> get_xop(inst));
>>>>>>> +#ifdef CONFIG_PPC_BOOK3S
>>>>>>> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>>>>>>> +#else
>>>>>>> kvmppc_core_queue_program(vcpu, 0);
>>>>>>> +#endif
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>> Do you want I send an updated patch with your changes?
>>>>
>>>> Well, you reported the issue and narrowed it down, so feel free to send
>>>> it under your name :). I merely simplified your patch a bit.
>>>
>>> Well, while I was trying to update the patch, I've re-tested this... and
>>> it fails. I don't know what I'm doing bad now or what I did bad before
>>> but it seems it doesn't work. :(
>>>
>>> Thomas, could try the patch from Alex?
>>
>> The patch from Alex also does not work for me.
>
> I've now had a closer look at the code, and I think the endless loop is
> caused by the fact that we try to inject a program interrupt twice here.
[...]
> Injecting the program interrupt twice of course destroys the
> return address in SRR0, causing this strange behavior.
> If I comment out the kvmppc_core_queue_program() in
> kvmppc_emulate_instruction(), the endless loop is gone.
Apart from this problem with injecting the program exception twice,
there is now another problem left here: In kvmppc_handle_exit_pr(), the
flags variable is 0, thus we're injecting the program exception without
indication what it is all about.
It works fine if I simply hard-code flags = SRR1_PROGILL, but I guess
that's not the right solution to this problem.
I'm currently trying to understand the call chain of this function, but
I have a hard time to understand why we don't get a proper value in this
case...
Alex (or somebody else who's more familiar with this code), do you have
an idea why flags could be zero here?
Thomas
More information about the Linuxppc-dev
mailing list