Re: [PATCH 3/4] KVM: PPC: Alow kvmppc_get_last_inst() to fail

[Alexander Graf <agraf@suse.de>]

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