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

[Avi Kivity <avi@redhat.com>]

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.


-- 
error compiling committee.c: too many arguments to function