From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
Date: Thu, 21 Aug 2008 13:19:50 +0000
Subject: Re: [PATCH 3/6] kvmppc: rewrite guest code - sprg0-3
Message-Id: <48AD6B76.7070909@linux.vnet.ibm.com>
List-Id: <kvm-ppc.vger.kernel.org>
References: <1219142205-12062-4-git-send-email-ehrhardt@linux.vnet.ibm.com>
In-Reply-To: <1219142205-12062-4-git-send-email-ehrhardt@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
To: kvm-ppc@vger.kernel.org

Hollis Blanchard wrote:
> Some initial comments:
>
> On Tue, 2008-08-19 at 12:36 +0200, ehrhardt@linux.vnet.ibm.com wrote:
>  =20
>> +               if (rw =3D 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 */
>> +               }
>>    =20
>
> I think it's time we replaced these magic numbers (32, 36 above, but
> also in the opcode switch statements) with named constants.
>
>  =20
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=20
patch on top.
Or do you want me to put a renaming patch into the paravirtualization=20
queue now ?
>> +int kvmppc_pvmem_rewrite_instruction(struct kvm_vcpu *vcpu)
>> +{
>> ...
>> +               down_read(&current->mm->mmap_sem);
>> +               pcpage =3D gfn_to_page(vcpu->kvm,
>> +                               guest_pcaddr >> KVM_PPCPV_MAGIC_PAGE_SHI=
FT);
>> +               up_read(&current->mm->mmap_sem);
>> +               if (pcpage =3D bad_page)
>> +                       return -EFAULT;
>> +
>> +               pcmem =3D kmap_atomic(pcpage, KM_USER0);
>> +               if (!pcmem)
>> +                       return -EFAULT;
>> +               pcaddr =3D ((unsigned long)pcmem
>> +                               | (vcpu->arch.pc & KVM_PPCPV_MAGIC_PAGE_=
MASK));
>> +               BUG_ON(inst !=3D *((u32 *)pcaddr));
>> +               create_instruction(pcaddr, newinst);
>> +               kunmap_atomic(pcpage, KM_USER0);
>>    =20
>
> 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.)
>
>  =20
damn - stepped in my own trap ;-)
thanks for the hint - fixed

--=20

Gr=FCsse / regards,=20
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization