Re: [PATCH v2 4/9] provide in-kernel apic

[Anthony Liguori <aliguori@us.ibm.com>]

Glauber Costa wrote:
> This patch provides kvm with an in-kernel apic. We are currently not enabling it.
> The code is heavily based on what's in qemu-kvm.git.
>
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  hw/apic.c         |  135 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  kvm.h             |    3 +
>  target-i386/kvm.c |   18 +++++++
>  3 files changed, 152 insertions(+), 4 deletions(-)
>
> diff --git a/hw/apic.c b/hw/apic.c
> index c89008e..5635607 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -299,7 +299,11 @@ void cpu_set_apic_base(CPUState *env, uint64_t val)
>  #endif
>      if (!s)
>          return;
> -    s->apicbase = (val & 0xfffff000) |
> +
> +    if (kvm_enabled() && kvm_irqchip_in_kernel())
> +        s->apicbase = val;
> +    else
> +        s->apicbase = (val & 0xfffff000) |
>          (s->apicbase & (MSR_IA32_APICBASE_BSP | MSR_IA32_APICBASE_ENABLE));
>      /* if disabled, cannot be enabled again */
>      if (!(val & MSR_IA32_APICBASE_ENABLE)) {
> @@ -497,6 +501,13 @@ void apic_init_reset(CPUState *env)
>      s->wait_for_sipi = 1;
>
>      env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);
> +
> +#ifdef KVM_CAP_MP_STATE
> +    if (kvm_enabled() && kvm_irqchip_in_kernel())
> +        env->mp_state
> +            = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
> +#endif
>   

I don't think CAP_MP_STATE should be treated as an optional feature.

> +static int kvm_kernel_lapic_load_from_user(APICState *s)
> +{
> +    int r = 0;
> +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
> +    struct kvm_lapic_state apic;
> +    struct kvm_lapic_state *klapic = &apic;
> +    int i;
> +
> +    if (!(kvm_enabled() && kvm_irqchip_in_kernel()))
> +        return 0;
> +
> +    memset(klapic, 0, sizeof apic);
> +    kapic_set_reg(klapic, 0x2, s->id << 24);
> +    kapic_set_reg(klapic, 0x8, s->tpr);
> +    kapic_set_reg(klapic, 0xd, s->log_dest << 24);
> +    kapic_set_reg(klapic, 0xe, s->dest_mode << 28 | 0x0fffffff);
> +    kapic_set_reg(klapic, 0xf, s->spurious_vec);
> +    for (i = 0; i < 8; i++) {
> +        kapic_set_reg(klapic, 0x10 + i, s->isr[i]);
> +        kapic_set_reg(klapic, 0x18 + i, s->tmr[i]);
> +        kapic_set_reg(klapic, 0x20 + i, s->irr[i]);
> +    }
> +    kapic_set_reg(klapic, 0x28, s->esr);
> +    kapic_set_reg(klapic, 0x30, s->icr[0]);
> +    kapic_set_reg(klapic, 0x31, s->icr[1]);
> +    for (i = 0; i < APIC_LVT_NB; i++)
> +        kapic_set_reg(klapic, 0x32 + i, s->lvt[i]);
> +    kapic_set_reg(klapic, 0x38, s->initial_count);
> +    kapic_set_reg(klapic, 0x3e, s->divide_conf);
> +
> +    r = kvm_set_lapic(s->cpu_env, klapic);
> +#endif
> +    return r;
> +}
>   

You should probably just setup VMState such that it directly saves 
kvm_lapic_state and then have the pre/post functions call the kernel 
ioctls to sync it.  There's not a whole lot of point switching the state 
between two different structures.

>  static const VMStateDescription vmstate_apic = {
>      .name = "apic",
>      .version_id = 3,
>      .minimum_version_id = 3,
>      .minimum_version_id_old = 1,
>      .load_state_old = apic_load_old,
> +    .pre_save = apic_pre_save,
> +    .post_load = apic_post_load,
>      .fields      = (VMStateField []) {
>          VMSTATE_UINT32(apicbase, APICState),
>          VMSTATE_UINT8(id, APICState),
> @@ -933,9 +1052,8 @@ static const VMStateDescription vmstate_apic = {
>      }
>  };
>   

Same applies here as ioapic.  Should be a separate device.

-- 
Regards,

Anthony Liguori