Re: [Qemu-devel] [PATCH v6 3/9] ARM: KVM: Add support for KVM on ARM architecture

[Peter Maydell]

On 23 February 2013 15:14, Andreas Färber <address@hidden> wrote:
> Am 22.02.2013 20:04, schrieb Peter Maydell:
>> +static void kvm_arm_pic_cpu_handler(void *opaque, int irq, int level)
>> +{
>> +#ifdef CONFIG_KVM
>> +    ARMCPU *armcpu = opaque;
>> +    CPUState *cpu = CPU(armcpu);
>
> I notice this is the only place you use "armcpu", elsewhere "cpu" is
> used for ARMCPU and "cs" for CPUState.

OK; I guess consistency would be better.

>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -237,6 +237,7 @@ void arm_translate_init(void);
>>  void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu);
>>  int cpu_arm_exec(CPUARMState *s);
>>  void do_interrupt(CPUARMState *);
>> +int bank_number(CPUARMState *env, int mode);
>
> Any chance to make this ARMCPU *cpu when exposing it globally?

Kind of painful given how it's used. I'd rather just change
the cpu_abort() to an assert() and drop the env parameter
altogether...

>>  void switch_mode(CPUARMState *, int);
>>  uint32_t do_arm_semihosting(CPUARMState *env);
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index e63da57..0380cb1 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -1617,7 +1617,7 @@ uint32_t HELPER(get_r13_banked)(CPUARMState *env, 
>> uint32_t mode)
>>  #else
>>
>>  /* Map CPU modes onto saved register banks.  */
>> -static inline int bank_number(CPUARMState *env, int mode)
>> +int bank_number(CPUARMState *env, int mode)
>>  {
>>      switch (mode) {
>>      case ARM_CPU_MODE_USR:
>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
>> new file mode 100644
>> index 0000000..13ebfd7
>> --- /dev/null
>> +++ b/target-arm/kvm.c
>> +const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>
> static const?

No, this is a required part of the interface that QEMU's
architecture-specific KVM code must expose to kvm-all.c, so
making it static would defeat the purpose (and fail to compile).

-- PMM