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.
> diff --git a/arch/powerpc/kvm/booke.h b/arch/powerpc/kvm/booke.h
> index 09bfd9b..c7c60c2 100644
> --- a/arch/powerpc/kvm/booke.h
> +++ b/arch/powerpc/kvm/booke.h
> @@ -90,6 +90,9 @@ void kvmppc_vcpu_disable_spe(struct kvm_vcpu *vcpu);
> void kvmppc_booke_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
> void kvmppc_booke_vcpu_put(struct kvm_vcpu *vcpu);
>
> +void kvmppc_core_queue_inst_storage(struct kvm_vcpu *vcpu,
> + ulong esr_flags);
Whitespace
> +
> enum int_class {
> INT_CLASS_NONCRIT,
> INT_CLASS_CRIT,
> @@ -123,6 +126,8 @@ extern int kvmppc_core_emulate_mtspr_e500(struct kvm_vcpu *vcpu, int sprn,
> extern int kvmppc_core_emulate_mfspr_e500(struct kvm_vcpu *vcpu, int sprn,
> ulong *spr_val);
>
> +extern int kvmppc_ld_inst(struct kvm_vcpu *vcpu, u32 *instr) ;
Whitespace
> diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
> index ecf2247..6025cb7 100644
> --- a/arch/powerpc/kvm/e500_mmu_host.c
> +++ b/arch/powerpc/kvm/e500_mmu_host.c
> @@ -34,6 +34,7 @@
> #include "e500.h"
> #include "timing.h"
> #include "e500_mmu_host.h"
> +#include "booke.h"
>
> #include "trace_booke.h"
>
> @@ -597,6 +598,10 @@ void kvmppc_mmu_map(struct kvm_vcpu *vcpu, u64 eaddr, gpa_t gpaddr,
> }
> }
>
> +int kvmppc_ld_inst(struct kvm_vcpu *vcpu, u32 *instr) {
> + return EMULATE_FAIL;
> +};
Brace placement
> /************* MMU Notifiers *************/
>
> int kvm_unmap_hva(struct kvm *kvm, unsigned long hva)
> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
> index 2f9a087..24a8e50 100644
> --- a/arch/powerpc/kvm/emulate.c
> +++ b/arch/powerpc/kvm/emulate.c
> @@ -225,19 +225,26 @@ static int kvmppc_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn, int rt)
> * from opcode tables in the future. */
> int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
> {
> - u32 inst = kvmppc_get_last_inst(vcpu);
> - int ra = get_ra(inst);
> - int rs = get_rs(inst);
> - int rt = get_rt(inst);
> - int sprn = get_sprn(inst);
> - enum emulation_result emulated = EMULATE_DONE;
> + u32 inst;
> + int ra, rs, rt, sprn;
> + enum emulation_result emulated;
> int advance = 1;
>
> /* this default type might be overwritten by subcategories */
> kvmppc_set_exit_type(vcpu, EMULATED_INST_EXITS);
>
> + emulated = kvmppc_get_last_inst(vcpu, &inst);
> + if (emulated != EMULATE_DONE) {
> + return emulated;
> + }
Unnecessary braces
-Scott
--
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
