Considering the state of the project, I see no point in discussing this
anymore. I have reset the master branch and lets just move forward.
/Christoffer
On Mon, Apr 13, 2009 at 7:02 PM, Brian Smith <bls2129 at columbia.edu> wrote:
> Am I missing something. How did you improve readability? For a refresher,
> here is what kvm_regs used to look like.
>
> struct kvm_regs {
> __u32 regs0_7[8]; /* A register for each of the unbanked
> registers (R0 - R7) */
> __u32 fiq_regs8_12[5]; /* A register for each of the banked fiq
> registers (R8 - R12) */
> __u32 usr_regs8_12[5]; /* A register for each of the banked usr
> registers (R8 - R12) */
> __u32 reg13[6]; /* Register 13 for each of the banked modes,
> indexed by MODE_ */
> __u32 reg14[6]; /* Register 14 for each of the banked modes,
> indexed by MODE_ */
> __u32 reg15; /* Register 15 */
> __u32 cpsr;
> __u32 spsr[5]; /* The SPSR for each mode, indexed by MODE_.
> user and system do not have one */
> };
>
> and here it is with your additions
> struct kvm_regs {
> /* Regs for current mode */
> __u32 regs[16];
>
> /* Banked registers */
> __u32 banked_spsr[5];
> __u32 banked_r13[6];
> __u32 banked_r14[6];
>
> /* Banked r8-r12 */
> __u32 usr_regs[5];
> __u32 fiq_regs[5];
>
> __u32 cpsr;
> __u32 spsr;
> };
>
> Again, note that regs[8] - regs[14] are covered by the banked registers, so
> I am still curious as to which is the "right" variable to use. If I was in
> MODE_USER, would the following be true? What if they aren't?
> regs[8] = usr_regs[0]
> regs[9] = usr_regs[1]
> regs[10] = usr_regs[2]
> regs[11] = usr_regs[3]
> regs[12] = usr_regs[4]
> regs[13] = banked_r13[MODE_USER]
> regs[14] = banked_r14[MODE_USER]
>
> Here is your QEMU setregs function, with my changes in diff format (+ is
> lines I added, - is ones I removed), to achieve the same functionality.
> FWIW, I don't believe you need to do a GET_REGS before the SET_REGS since
> you are writing all the registers.
> Also, it appears this function needs to be reworked due to the fact that
> QEMUs indexes aren't the same as KVMs (see kvm.h MODE_* declares) and the
> backed registers are stail.
>
> int kvm_arch_put_registers(CPUState *env)
> {
> struct kvm_regs regs;
> int ret;
>
> ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, ®s);
> if (ret < 0)
> return ret;
>
> - memcpy(regs.regs, env->regs, sizeof(uint32_t) * 16);
> + memcpy(regs.regs0_7, env->regs, sizeof(uint32_t) * 8);
>
> - /* TODO: Figure out if offset is in beginning or end of banked_spsr
> */
> - memcpy(regs.banked_spsr,
> - &env->banked_spsr[1],
> - sizeof(uint32_t) * 5);
> + memcpy(regs.spsr, env->banked_spsr, sizeof(uint32_t) * 5);
>
> - memcpy(regs.banked_r13, env->banked_r13, sizeof(uint32_t) * 6);
> + memcpy(regs.reg13, env->banked_r13, sizeof(uint32_t) * 6);
>
> - memcpy(regs.banked_r14, env->banked_r14, sizeof(uint32_t) * 6);
> + memcpy(regs.reg14, env->banked_r14, sizeof(uint32_t) * 6);
>
> - memcpy(regs.usr_regs, env->usr_regs, sizeof(uint32_t) * 5);
> + memcpy(regs.usr_regs8_12, env->usr_regs, sizeof(uint32_t) * 5);
>
> - memcpy(regs.fiq_regs, env->fiq_regs, sizeof(uint32_t) * 5);
> + memcpy(regs.fiq_regs8_12, env->fiq_regs, sizeof(uint32_t) * 5);
>
> regs.cpsr = cpsr_read(env);
> - regs.spsr = env->spsr;
>
> ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, ®s);
> if (ret < 0)
> return ret;
>
> return ret;
> }
>
> I ask that you please explain to me what value you are adding to the
> interface, and please answer my concerns below if you still think we need
> the change (not just the answers, but how someone besides the three of us
> would be able to answer it without help).
>
> And finally, here is the function, as I think you would need to implement
> it. This is regardless of what we do with kvm_regs, but below is assuming
> the old kvm_regs.
> int kvm_arch_put_registers(CPUState *env)
> {
> struct kvm_regs regs;
> int ret,i;
> backupRegisters(env);
>
> memcpy(regs.regs0_7, env->regs, sizeof(uint32_t) * 8);
> memcpy(regs.usr_regs, env->usr_regs, sizeof(uint32_t) * 5);
> memcpy(regs.fiq_regs, env->fiq_regs, sizeof(uint32_t) * 5);
>
> for(i=0; i<6; i++) {
> regs.banked_r13[i] = env->banked_r13[convertKVMToQemuIdx(i)];
> regs.banked_r14[i] = env->banked_r13[convertKVMToQemuIdx(i)];
> }
> for(i=0; i<5; i++) {
> regs.banked_spsr[i] = env->banked_spsr[convertKVMToQemuIdx(i)];
> }
> regs.cpsr = cpsr_read(env);
>
> ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, ®s);
> if (ret < 0)
> return ret;
>
> return ret;
> }
>
> /* Taken from helper.c, switch_mode */
> void backupRegisters(env)
> {
> int mode;
> int i;
>
> mode = env->uncached_cpsr & CPSR_M;
> if (mode == ARM_CPU_MODE_FIQ) {
> memcpy (env->fiq_regs, env->regs + 8, 5 * sizeof(uint32_t));
> memcpy (env->regs + 8, env->usr_regs, 5 * sizeof(uint32_t));
> }
>
> i = bank_number(mode);
> env->banked_r13[i] = env->regs[13];
> env->banked_r14[i] = env->regs[14];
> env->banked_spsr[i] = env->spsr;
> }
>
> /* Taken from helper.c bank_number */
> int convertKVMToQEMUIdx(int kvmIdx)
> {
> switch(kvmIdx){
> case MODE_FIQ:
> return 5;
> case MODE_IRQ:
> return 4;
> case MODE_SUP:
> return 1;
> case MODE_ABORT:
> return 2;
> case MODE_UNDEF:
> return 3;
> case MODE_USER:
> case MODE_SYSTEM:
> return 0;
>
> }
>
>
>
>
> bls2129 at columbia.edu wrote:
>
>> Christoffer,
>> Having regs[16] is an internal optimization so when emulating
>> something, lets say a mov from one reg to another, we don't have to figure
>> out where to get/put it every time based on the current mode.
>>
>> With optimization:
>> reg[r?] = xxxx
>>
>> Without optimization
>> if R? < R8 then
>> ...
>> else if R? < R12 & mode=fiq
>> fiq
>> else if R? < R12
>> usr
>> else if R? = R13
>> reg13
>> else if R? = R14
>> reg14
>> else
>> reg15
>>
>> That optimization, I believe, shouldn't be externalized through the
>> kvm_regs struct. I believe adding a regs[16] is additional complexity.
>> For example, did you know that regs[8] through regs[14] in kvm_regs are
>> irrelevant? In set_regs we do not use them because we reload vcpu->regs
>> from the backed registers, overwriting anything that was in there. Does
>> everyone trying to interface with kvm know this? Or would they think that
>> the backed regs (fiq_regs, usr_regs, reg13 and reg14) for the current mode
>> are the ones that are irrelevant. Should the backed regs for the current
>> mode be the same that is in regs? What if they are different, is that an
>> error? Does regs represent the registers for the current mode before or
>> after setting the CPSR? Leaving it as it was raises less questions in my
>> mind.
>>
>> I understand QEMU has the same structure (that is where it was taken
>> from), but we aren't building an interface between QEMU and KVM. We are
>> building an interface that is used by QEMU. Because they have similarities
>> doesn't mean we should complicate things for anyone else that might use the
>> interface. On the same note, how does QEMU "back" their regs? When you do
>> the set_regs are regs and the backed regs consistent?
>>
>> Feel free to move the testTrans.make to the svn repository, it will
>> require some fixup though because it needs arm_translate.c and whatever
>> header files that points to.
>>
>> Finally, the name change doesn't matter to me either way, I was only
>> curious as to the motivation.
>>
>> Thanks,
>> Brian
>>
>>
>> Quoting Christoffer Dall <christofferdall at christofferdall.dk>:
>>
>>
>>
>>> Hi.
>>>
>>> Brian, you're right about the main branch. That was just stupid on my
>>> part.
>>>
>>> The changes to kvm_regs is because the role of kvm_regs is mainly to
>>> transfer registers between kvm_vcpu->arch and the QEMU CPUState struct.
>>> QEMU
>>> CPUState and kvm_vcpu had almost exact definitions and it was simply
>>> unnecessary complexity for debugging to have a different scheme for an
>>> intermediate step. Do you agree, or am I missing something here?
>>>
>>> I completetly understand if you don't want to change many lines of code,
>>> which is also what I promised to do in earlier e-mails. I modified
>>> kvmtest
>>> to work with the modified kvm_regs, but I don't know where to look for
>>> more
>>> code dependent on this?
>>>
>>> By the way, when I come to think of this, should we move the special
>>> makefile from the kernel source to the svn source like the kvmtest or is
>>> it
>>> easier to keep it in the kernel source?
>>>
>>> Regarding the kvmarm_xxx prefix, maybe you're right. I didn't realize it
>>> was
>>> only for interface functions. I just changed the names going over them to
>>> use lower-case more "kernel looking" code, and then it was convenient
>>> with
>>> the kvmarm_xxx prefix for code completion in the editor, but I'll change
>>> it
>>> back if you think so.
>>>
>>> In general, maybe we should have someone write a "naming" and "code
>>> style"
>>> convention page in the wiki, which we can discuss for our KVM code.
>>>
>>> Regards,
>>> Christoffer
>>>
>>> On Sun, Apr 12, 2009 at 8:24 PM, Brian Smith <bls2129 at columbia.edu>
>>> wrote:
>>>
>>>
>>>
>>>> I'm fine with the changes to the makefile and moving the headers to
>>>> /usr/src/android/kernel.
>>>>
>>>> For the commits you made to the master branch, that is unacceptable. I
>>>> have been very careful not to merge to the master branch before letting
>>>> you give the okay, I expect the same be done. I don't care about the
>>>> formatting/name changes so much as the incompatible and redundant
>>>> changes you made to kvm_regs. Please tell me why this is necessary, and
>>>> then I will start thinking about reworking my 50+ kvm_regs scaffolding I
>>>> have created for testing the emulation code.
>>>>
>>>> For the name changes to kvmarm_xxx, did these become interfaces? I was
>>>> under the impression that kvmarm_xxx type stuff should only be for
>>>> interfaces, and if a function is internal to that source file that
>>>> prefix shouldn't be used. I don't care either way, I'm just curious.
>>>>
>>>> Regards,
>>>> Brian
>>>>
>>>> Christoffer Dall wrote:
>>>>
>>>>
>>>>> Hi.
>>>>>
>>>>> I changed the kvmtest Makefile to be more general. Thus, it assumes
>>>>> that the "arm-none-linux-gnueabi-*" tools are on the PATH and do not
>>>>> reference these absolutely. This follows standard convention for use
>>>>> of gcc and makes the Makefile portable across our different machines.
>>>>>
>>>>> Unfortunately we have to directly reference a path for our cusom
>>>>> kernel, and we have different locations for this one. Therefore, I
>>>>> though of the best 'convention' to follow and link with headers from
>>>>> /usr/src/android/kernel/... If you don't keep the source there (as I
>>>>> didn't for instance) create a symbolic link to you kernel source.
>>>>>
>>>>> This way we don't have to modify the Makefile on each commit.
>>>>>
>>>>> Also, as can be seen in a recent git commit message, I cleaned up some
>>>>> unused files in the kernel source and consolidated a few header files.
>>>>> I will make sure that this merges with whatever commits we have after
>>>>> this weekend.
>>>>>
>>>>>
>>>>> Best regards,
>>>>> Christoffer
>>>>>
>>>>> ------------------------------------------------------------------------
>>>>>
>>>>> _______________________________________________
>>>>> Android-virt mailing list
>>>>> Android-virt at lists.cs.columbia.edu
>>>>> https://lists.cs.columbia.edu/cucslists/listinfo/android-virt
>>>>>
>>>>>
>>>>>
>>>> _______________________________________________
>>>> Android-virt mailing list
>>>> Android-virt at lists.cs.columbia.edu
>>>> https://lists.cs.columbia.edu/cucslists/listinfo/android-virt
>>>>
>>>>
>>>>
>>>
>>
>>
>> _______________________________________________
>> Android-virt mailing list
>> Android-virt at lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/cucslists/listinfo/android-virt
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.cs.columbia.edu/pipermail/android-virt/attachments/20090413/838fdf91/attachment-0001.html
