On 03/26/2014 09:52 PM, Scott Wood wrote:
On Thu, 2014-02-20 at 18:30 +0200, Mihai Caraman wrote:
diff --git a/arch/powerpc/kvm/book3s_paired_singles.c b/arch/powerpc/kvm/book3s_paired_singles.c
index a59a25a..80c533e 100644
--- a/arch/powerpc/kvm/book3s_paired_singles.c
+++ b/arch/powerpc/kvm/book3s_paired_singles.c
@@ -640,19 +640,24 @@ static int kvmppc_ps_one_in(struct kvm_vcpu *vcpu, bool rc,
int kvmppc_emulate_paired_single(struct kvm_run *run, struct kvm_vcpu *vcpu)
{
- u32 inst = kvmppc_get_last_inst(vcpu);
+ u32 inst;
enum emulation_result emulated = EMULATE_DONE;
-
- int ax_rd = inst_get_field(inst, 6, 10);
- int ax_ra = inst_get_field(inst, 11, 15);
- int ax_rb = inst_get_field(inst, 16, 20);
- int ax_rc = inst_get_field(inst, 21, 25);
- short full_d = inst_get_field(inst, 16, 31);
-
- u64 *fpr_d = &vcpu->arch.fpr[ax_rd];
- u64 *fpr_a = &vcpu->arch.fpr[ax_ra];
- u64 *fpr_b = &vcpu->arch.fpr[ax_rb];
- u64 *fpr_c = &vcpu->arch.fpr[ax_rc];
+ int ax_rd, ax_ra, ax_rb, ax_rc;
+ short full_d;
+ u64 *fpr_d, *fpr_a, *fpr_b, *fpr_c;
+
+ kvmppc_get_last_inst(vcpu, &inst);
Should probably check for failure here and elsewhere -- even though it
can't currently fail on book3s, the interface now allows it.
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 5b9e906..b0d884d 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -624,9 +624,10 @@ 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);
+ u32 last_inst;
int ret;
+ kvmppc_get_last_inst(vcpu, &last_inst);
ret = kvmppc_ld(vcpu, &srr0, sizeof(u32), &last_inst, false);
This isn't new, but this function looks odd to me -- calling
kvmppc_get_last_inst() but ignoring last_inst, then calling kvmppc_ld()
and ignoring anything but failure. last_inst itself is never read. And
no comments to explain the weirdness. :-)
I get that kvmppc_get_last_inst() is probably being used for the side
effect of filling in vcpu->arch.last_inst, but why store the return
value without using it? Why pass the address of it to kvmppc_ld(),
which seems to be used only as an indirect way of determining whether
kvmppc_get_last_inst() failed? And that whole mechanism becomes
stranger once it becomes possible for kvmppc_get_last_inst() to directly
return failure.
If you're interested in the history of this, here's the patch :)
https://github.com/mirrors/linux-2.6/commit/c7f38f46f2a98d232147e47284cb4e7363296a3e
The idea is that we need 2 things to be good after this function:
1) vcpu->arch.last_inst is valid
2) if the last instruction is not readable, return failure
Hence this weird construct. I don't think it's really necessary though -
just remove the kvmppc_ld() call and only fail read_inst() when the
caching didn't work and we can't translate the address.
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
