Re: [Qemu-devel] [PATCH 1/3] kvm: move KVM_GET_LAPIC/KVM_SET_LAPIC to hw/kvm/apic.c

[Jan Kiszka]

On 2012-10-30 13:16, Paolo Bonzini wrote:
> Leave knowledge of the KVM in-kernel LAPIC ioctls to hw/kvm/apic.c.
> The CPU doesn't need to know anything about kvm_lapic_state.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/kvm/apic.c     |   76 
> ++++++++++++++++++++++++++++++-----------------------
>  kvm.h             |    4 +-
>  target-i386/kvm.c |   14 +--------
>  3 files changed, 47 insertions(+), 47 deletions(-)
> 
> diff --git a/hw/kvm/apic.c b/hw/kvm/apic.c
> index dbac7ff..ddf6b7d 100644
> --- a/hw/kvm/apic.c
> +++ b/hw/kvm/apic.c
> @@ -25,62 +25,72 @@ static inline uint32_t kvm_apic_get_reg(struct 
> kvm_lapic_state *kapic,
>      return *((uint32_t *)(kapic->regs + (reg_id << 4)));
>  }
>  
> -void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
> +int kvm_put_apic_state(DeviceState *d)
>  {
>      APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
> +    struct kvm_lapic_state kapic;
>      int i;
>  
> -    memset(kapic, 0, sizeof(*kapic));
> -    kvm_apic_set_reg(kapic, 0x2, s->id << 24);
> -    kvm_apic_set_reg(kapic, 0x8, s->tpr);
> -    kvm_apic_set_reg(kapic, 0xd, s->log_dest << 24);
> -    kvm_apic_set_reg(kapic, 0xe, s->dest_mode << 28 | 0x0fffffff);
> -    kvm_apic_set_reg(kapic, 0xf, s->spurious_vec);
> +    memset(&kapic, 0, sizeof(kapic));
> +    kvm_apic_set_reg(&kapic, 0x2, s->id << 24);
> +    kvm_apic_set_reg(&kapic, 0x8, s->tpr);
> +    kvm_apic_set_reg(&kapic, 0xd, s->log_dest << 24);
> +    kvm_apic_set_reg(&kapic, 0xe, s->dest_mode << 28 | 0x0fffffff);
> +    kvm_apic_set_reg(&kapic, 0xf, s->spurious_vec);
>      for (i = 0; i < 8; i++) {
> -        kvm_apic_set_reg(kapic, 0x10 + i, s->isr[i]);
> -        kvm_apic_set_reg(kapic, 0x18 + i, s->tmr[i]);
> -        kvm_apic_set_reg(kapic, 0x20 + i, s->irr[i]);
> +        kvm_apic_set_reg(&kapic, 0x10 + i, s->isr[i]);
> +        kvm_apic_set_reg(&kapic, 0x18 + i, s->tmr[i]);
> +        kvm_apic_set_reg(&kapic, 0x20 + i, s->irr[i]);
>      }
> -    kvm_apic_set_reg(kapic, 0x28, s->esr);
> -    kvm_apic_set_reg(kapic, 0x30, s->icr[0]);
> -    kvm_apic_set_reg(kapic, 0x31, s->icr[1]);
> +    kvm_apic_set_reg(&kapic, 0x28, s->esr);
> +    kvm_apic_set_reg(&kapic, 0x30, s->icr[0]);
> +    kvm_apic_set_reg(&kapic, 0x31, s->icr[1]);
>      for (i = 0; i < APIC_LVT_NB; i++) {
> -        kvm_apic_set_reg(kapic, 0x32 + i, s->lvt[i]);
> +        kvm_apic_set_reg(&kapic, 0x32 + i, s->lvt[i]);
>      }
> -    kvm_apic_set_reg(kapic, 0x38, s->initial_count);
> -    kvm_apic_set_reg(kapic, 0x3e, s->divide_conf);
> +    kvm_apic_set_reg(&kapic, 0x38, s->initial_count);
> +    kvm_apic_set_reg(&kapic, 0x3e, s->divide_conf);
> +
> +    return kvm_vcpu_ioctl(s->cpu_env, KVM_SET_LAPIC, &kapic);
>  }
>  
> -void kvm_get_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
> +int kvm_get_apic_state(DeviceState *d)
>  {
>      APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
> -    int i, v;
> -
> -    s->id = kvm_apic_get_reg(kapic, 0x2) >> 24;
> -    s->tpr = kvm_apic_get_reg(kapic, 0x8);
> -    s->arb_id = kvm_apic_get_reg(kapic, 0x9);
> -    s->log_dest = kvm_apic_get_reg(kapic, 0xd) >> 24;
> -    s->dest_mode = kvm_apic_get_reg(kapic, 0xe) >> 28;
> -    s->spurious_vec = kvm_apic_get_reg(kapic, 0xf);
> +    struct kvm_lapic_state kapic;
> +    int i, v, ret;
> +
> +    ret = kvm_vcpu_ioctl(s->cpu_env, KVM_GET_LAPIC, &kapic);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    s->id = kvm_apic_get_reg(&kapic, 0x2) >> 24;
> +    s->tpr = kvm_apic_get_reg(&kapic, 0x8);
> +    s->arb_id = kvm_apic_get_reg(&kapic, 0x9);
> +    s->log_dest = kvm_apic_get_reg(&kapic, 0xd) >> 24;
> +    s->dest_mode = kvm_apic_get_reg(&kapic, 0xe) >> 28;
> +    s->spurious_vec = kvm_apic_get_reg(&kapic, 0xf);
>      for (i = 0; i < 8; i++) {
> -        s->isr[i] = kvm_apic_get_reg(kapic, 0x10 + i);
> -        s->tmr[i] = kvm_apic_get_reg(kapic, 0x18 + i);
> -        s->irr[i] = kvm_apic_get_reg(kapic, 0x20 + i);
> +        s->isr[i] = kvm_apic_get_reg(&kapic, 0x10 + i);
> +        s->tmr[i] = kvm_apic_get_reg(&kapic, 0x18 + i);
> +        s->irr[i] = kvm_apic_get_reg(&kapic, 0x20 + i);
>      }
> -    s->esr = kvm_apic_get_reg(kapic, 0x28);
> -    s->icr[0] = kvm_apic_get_reg(kapic, 0x30);
> -    s->icr[1] = kvm_apic_get_reg(kapic, 0x31);
> +    s->esr = kvm_apic_get_reg(&kapic, 0x28);
> +    s->icr[0] = kvm_apic_get_reg(&kapic, 0x30);
> +    s->icr[1] = kvm_apic_get_reg(&kapic, 0x31);
>      for (i = 0; i < APIC_LVT_NB; i++) {
> -        s->lvt[i] = kvm_apic_get_reg(kapic, 0x32 + i);
> +        s->lvt[i] = kvm_apic_get_reg(&kapic, 0x32 + i);
>      }
> -    s->initial_count = kvm_apic_get_reg(kapic, 0x38);
> -    s->divide_conf = kvm_apic_get_reg(kapic, 0x3e);
> +    s->initial_count = kvm_apic_get_reg(&kapic, 0x38);
> +    s->divide_conf = kvm_apic_get_reg(&kapic, 0x3e);
>  
>      v = (s->divide_conf & 3) | ((s->divide_conf >> 1) & 4);
>      s->count_shift = (v + 1) & 7;
>  
>      s->initial_count_load_time = qemu_get_clock_ns(vm_clock);
>      apic_next_timer(s, s->initial_count_load_time);
> +    return 0;
>  }
>  
>  static void kvm_apic_set_base(APICCommonState *s, uint64_t val)
> diff --git a/kvm.h b/kvm.h
> index 2b26dcb..0056f92 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -191,8 +191,8 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
>  
>  void kvm_irqchip_add_irq_route(KVMState *s, int gsi, int irqchip, int pin);
>  
> -void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic);
> -void kvm_get_apic_state(DeviceState *d, struct kvm_lapic_state *kapic);
> +int kvm_put_apic_state(DeviceState *d);
> +int kvm_get_apic_state(DeviceState *d);
>  
>  struct kvm_guest_debug;
>  struct kvm_debug_exit_arch;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 3aa62b2..092d4f1 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -1384,16 +1384,9 @@ static int kvm_get_mp_state(CPUX86State *env)
>  static int kvm_get_apic(CPUX86State *env)
>  {
>      DeviceState *apic = env->apic_state;
> -    struct kvm_lapic_state kapic;
> -    int ret;
>  
>      if (apic && kvm_irqchip_in_kernel()) {
> -        ret = kvm_vcpu_ioctl(env, KVM_GET_LAPIC, &kapic);
> -        if (ret < 0) {
> -            return ret;
> -        }
> -
> -        kvm_get_apic_state(apic, &kapic);
> +        return kvm_get_apic_state(apic);
>      }
>      return 0;
>  }
> @@ -1401,12 +1394,9 @@ static int kvm_get_apic(CPUX86State *env)
>  static int kvm_put_apic(CPUX86State *env)
>  {
>      DeviceState *apic = env->apic_state;
> -    struct kvm_lapic_state kapic;
>  
>      if (apic && kvm_irqchip_in_kernel()) {
> -        kvm_put_apic_state(apic, &kapic);
> -
> -        return kvm_vcpu_ioctl(env, KVM_SET_LAPIC, &kapic);
> +        return kvm_put_apic_state(apic);
>      }
>      return 0;
>  }
> 

This refactoring is fine with me.

Acked-by: Jan Kiszka <address@hidden>

Jan

signature.asc
Description: OpenPGP digital signature