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

[Glauber Costa <glommer@redhat.com>]

On Thu, Oct 08, 2009 at 04:09:27PM +0200, Avi Kivity wrote:
> On 10/08/2009 03:55 PM, Anthony Liguori wrote:
>> 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.
>
> It ensures the two models are compatible.  Since they're the same device  
> from the point of view of the guest, there's no reason for them to have  
> different representations or to be incompatible.

live migration between something that has in-kernel irqchip and something that
has not is likely to be a completely untested thing. And this is the only reason
we might think of it as the same device. I don't see any value in supporting this
combination