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
>
>
