On 05/06/2014 09:06 PM, mihai.caraman@xxxxxxxxxxxxx wrote:
-----Original Message-----
From: Alexander Graf [mailto:agraf@xxxxxxx]
Sent: Friday, May 02, 2014 12:55 PM
To: Caraman Mihai Claudiu-B02008
Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; linuxppc-
dev@xxxxxxxxxxxxxxxx
Subject: Re: [PATCH v2 3/4] KVM: PPC: Alow kvmppc_get_last_inst() to fail
On 05/01/2014 02:45 AM, Mihai Caraman wrote:
...
diff --git a/arch/powerpc/include/asm/kvm_ppc.h
b/arch/powerpc/include/asm/kvm_ppc.h
index 4096f16..6e7c358 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -72,6 +72,8 @@ extern int kvmppc_sanity_check(struct kvm_vcpu
*vcpu);
extern int kvmppc_subarch_vcpu_init(struct kvm_vcpu *vcpu);
extern void kvmppc_subarch_vcpu_uninit(struct kvm_vcpu *vcpu);
+extern int kvmppc_get_last_inst(struct kvm_vcpu *vcpu, u32 *inst);
Phew. Moving this into a separate function sure has some performance
implications. Was there no way to keep it in a header?
You could just move it into its own .h file which we include after
kvm_ppc.h. That way everything's available. That would also help me a
lot with the little endian port where I'm also struggling with header
file inclusion order and kvmppc_need_byteswap().
Great, I will do this.
diff --git a/arch/powerpc/kvm/book3s_pr.c
b/arch/powerpc/kvm/book3s_pr.c
index c5c052a..b7fffd1 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -608,12 +608,9 @@ void kvmppc_giveup_ext(struct kvm_vcpu *vcpu,
ulong msr)
static int kvmppc_read_inst(struct kvm_vcpu *vcpu)
{
- ulong srr0 = kvmppc_get_pc(vcpu);
- u32 last_inst = kvmppc_get_last_inst(vcpu);
- int ret;
+ u32 last_inst;
- ret = kvmppc_ld(vcpu, &srr0, sizeof(u32), &last_inst, false);
- if (ret == -ENOENT) {
+ if (kvmppc_get_last_inst(vcpu, &last_inst) == -ENOENT) {
ENOENT?
You have to tell us :) Why does kvmppc_ld() mix emulation_result
enumeration with generic errors? Do you want to change that and
use EMULATE_FAIL instead?
Haha you're right. Man, this code sucks :). No, leave it be for now.
ulong msr = vcpu->arch.shared->msr;
msr = kvmppc_set_field(msr, 33, 33, 1);
@@ -867,15 +864,18 @@ int kvmppc_handle_exit_pr(struct kvm_run *run,
struct kvm_vcpu *vcpu,
{
enum emulation_result er;
ulong flags;
+ u32 last_inst;
program_interrupt:
flags = vcpu->arch.shadow_srr1 & 0x1f0000ull;
+ kvmppc_get_last_inst(vcpu, &last_inst);
No check for the return value?
Should we queue a program exception and resume guest?
Depends on the return code. We need to be able to handle EMULATE_AGAIN
everywhere now.
if (vcpu->arch.shared->msr & MSR_PR) {
#ifdef EXIT_DEBUG
- printk(KERN_INFO "Userspace triggered 0x700 exception
at 0x%lx (0x%x)\n", kvmppc_get_pc(vcpu), kvmppc_get_last_inst(vcpu));
+ pr_info("Userspace triggered 0x700 exception at\n"
+ "0x%lx (0x%x)\n", kvmppc_get_pc(vcpu), last_inst);
#endif
- if ((kvmppc_get_last_inst(vcpu) & 0xff0007ff) !=
+ if ((last_inst & 0xff0007ff) !=
(INS_DCBZ & 0xfffffff7)) {
kvmppc_core_queue_program(vcpu, flags);
r = RESUME_GUEST;
@@ -894,7 +894,7 @@ program_interrupt:
break;
case EMULATE_FAIL:
printk(KERN_CRIT "%s: emulation at %lx failed
(%08x)\n",
- __func__, kvmppc_get_pc(vcpu),
kvmppc_get_last_inst(vcpu));
+ __func__, kvmppc_get_pc(vcpu), last_inst);
kvmppc_core_queue_program(vcpu, flags);
r = RESUME_GUEST;
break;
@@ -911,8 +911,12 @@ program_interrupt:
break;
}
case BOOK3S_INTERRUPT_SYSCALL:
+ {
+ u32 last_sc;
+
+ kvmppc_get_last_sc(vcpu, &last_sc);
No check for the return value?
The existing code does not handle KVM_INST_FETCH_FAILED.
How should we continue if papr is enabled and last_sc fails?
I'd say go back into the guest and try again ;). Keep in mind that sc
already sets srr0 to pc + 4, so we need to subtract 4 from pc and then
go back into the guest.
if (vcpu->arch.papr_enabled &&
- (kvmppc_get_last_sc(vcpu) == 0x44000022) &&
+ (last_sc == 0x44000022) &&
!(vcpu->arch.shared->msr & MSR_PR)) {
/* SC 1 papr hypercalls */
ulong cmd = kvmppc_get_gpr(vcpu, 3);
@@ -957,6 +961,7 @@ program_interrupt:
r = RESUME_GUEST;
}
break;
+ }
case BOOK3S_INTERRUPT_FP_UNAVAIL:
case BOOK3S_INTERRUPT_ALTIVEC:
case BOOK3S_INTERRUPT_VSX:
@@ -985,15 +990,20 @@ program_interrupt:
break;
}
case BOOK3S_INTERRUPT_ALIGNMENT:
+ {
+ u32 last_inst;
+
if (kvmppc_read_inst(vcpu) == EMULATE_DONE) {
- vcpu->arch.shared->dsisr = kvmppc_alignment_dsisr(vcpu,
- kvmppc_get_last_inst(vcpu));
- vcpu->arch.shared->dar = kvmppc_alignment_dar(vcpu,
- kvmppc_get_last_inst(vcpu));
+ kvmppc_get_last_inst(vcpu, &last_inst);
I think with an error returning kvmppc_get_last_inst we can just use
completely get rid of kvmppc_read_inst() and only use
kvmppc_get_last_inst() instead.
The only purpose of kvmppc_read_inst() function is to generate an
instruction storage interrupt in case of load failure. It is also called
from kvmppc_check_ext(). Do you suggest to get rid of this logic? Or to
duplicate it in order to avoid kvmppc_get_last_inst() redundant call?
I don't think we need that logic. If we get EMULATE_AGAIN, we just have
to make sure we go back into the guest. No need to inject an ISI into
the guest - it'll do that all by itself.
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
