Re: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3

[Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>]

Hollis Blanchard wrote:
> Some initial comments:
>
> On Tue, 2008-08-19 at 12:36 +0200, ehrhardt@linux.vnet.ibm.com wrote:
>   
>> +               if (rw = KVM_PPCPV_PVMEM_READ) {
>> +                       set_op(32, &newinst); /* lwz */
>> +                       set_rt(get_rt(inst), &newinst); /* set original rt */
>> +               } else {
>> +                       set_op(36, &newinst); /* stw */
>> +                       set_rs(get_rs(inst), &newinst); /* set original rs */
>> +               }
>>     
>
> I think it's time we replaced these magic numbers (32, 36 above, but
> also in the opcode switch statements) with named constants.
>
>   
Yes, but I wanted to keep it in sync with the emulation switch code here.
Once we agree on this patch series it is easy to throw a numer->name 
patch on top.
Or do you want me to put a renaming patch into the paravirtualization 
queue now ?
>> +int kvmppc_pvmem_rewrite_instruction(struct kvm_vcpu *vcpu)
>> +{
>> ...
>> +               down_read(&current->mm->mmap_sem);
>> +               pcpage = gfn_to_page(vcpu->kvm,
>> +                               guest_pcaddr >> KVM_PPCPV_MAGIC_PAGE_SHIFT);
>> +               up_read(&current->mm->mmap_sem);
>> +               if (pcpage = bad_page)
>> +                       return -EFAULT;
>> +
>> +               pcmem = kmap_atomic(pcpage, KM_USER0);
>> +               if (!pcmem)
>> +                       return -EFAULT;
>> +               pcaddr = ((unsigned long)pcmem
>> +                               | (vcpu->arch.pc & KVM_PPCPV_MAGIC_PAGE_MASK));
>> +               BUG_ON(inst != *((u32 *)pcaddr));
>> +               create_instruction(pcaddr, newinst);
>> +               kunmap_atomic(pcpage, KM_USER0);
>>     
>
> This suffers from the same kunmap_atomic() problem you pointed out to me
> elsewhere in my own code. :) (You're passing pcpage to kunmap_atomic()
> instead of pcaddr.)
>
>   
damn - stepped in my own trap ;-)
thanks for the hint - fixed

-- 

Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization