Re: [PATCH v4 12/15] kvm: x86: Add user space part for in-kernel APIC

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 4101 bytes --]

On 2011-12-10 16:40, Blue Swirl wrote:
> On Fri, Dec 9, 2011 at 07:52, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2011-12-09 08:45, Jan Kiszka wrote:
>>> On 2011-12-08 22:16, Blue Swirl wrote:
>>>> On Thu, Dec 8, 2011 at 11:52, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>> This introduces the alternative APIC backend which makes use of KVM's
>>>>> in-kernel device model. External NMI injection via LINT1 is emulated by
>>>>> checking the current state of the in-kernel APIC, only injecting a NMI
>>>>> into the VCPU if LINT1 is unmasked and configured to DM_NMI.
>>>>>
>>>>> MSI is not yet supported, so we disable this when the in-kernel model is
>>>>> in use.
>>>>>
>>>>> CC: Lai Jiangshan <laijs@cn.fujitsu.com>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>>  Makefile.target   |    2 +-
>>>>>  hw/kvm/apic.c     |  154 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  hw/pc.c           |   15 ++++--
>>>>>  kvm.h             |    3 +
>>>>>  target-i386/kvm.c |    8 +++
>>>>>  5 files changed, 176 insertions(+), 6 deletions(-)
>>>>>  create mode 100644 hw/kvm/apic.c
>>>>>
>>>>> diff --git a/Makefile.target b/Makefile.target
>>>>> index b549988..76de485 100644
>>>>> --- a/Makefile.target
>>>>> +++ b/Makefile.target
>>>>> @@ -236,7 +236,7 @@ obj-i386-y += vmport.o
>>>>>  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
>>>>>  obj-i386-y += debugcon.o multiboot.o
>>>>>  obj-i386-y += pc_piix.o
>>>>> -obj-i386-$(CONFIG_KVM) += kvm/clock.o
>>>>> +obj-i386-$(CONFIG_KVM) += kvm/clock.o kvm/apic.o
>>>>>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>>>>
>>>>>  # shared objects
>>>>> diff --git a/hw/kvm/apic.c b/hw/kvm/apic.c
>>>>> new file mode 100644
>>>>> index 0000000..3924f9e
>>>>> --- /dev/null
>>>>> +++ b/hw/kvm/apic.c
>>>>> @@ -0,0 +1,154 @@
>>>>> +/*
>>>>> + * KVM in-kernel APIC support
>>>>> + *
>>>>> + * Copyright (c) 2011 Siemens AG
>>>>> + *
>>>>> + * Authors:
>>>>> + *  Jan Kiszka          <jan.kiszka@siemens.com>
>>>>> + *
>>>>> + * This work is licensed under the terms of the GNU GPL version 2.
>>>>> + * See the COPYING file in the top-level directory.
>>>>> + */
>>>>> +#include "hw/apic_internal.h"
>>>>> +#include "kvm.h"
>>>>> +
>>>>> +static inline void kvm_apic_set_reg(struct kvm_lapic_state *kapic,
>>>>> +                                   int reg_id, uint32_t val)
>>>>> +{
>>>>> +    *((uint32_t *)(kapic->regs + (reg_id << 4))) = val;
>>>>> +}
>>>>> +
>>>>> +static inline uint32_t kvm_apic_get_reg(struct kvm_lapic_state *kapic,
>>>>> +                                       int reg_id)
>>>>> +{
>>>>> +    return *((uint32_t *)(kapic->regs + (reg_id << 4)));
>>>>> +}
>>>>> +
>>>>> +int kvm_put_apic(CPUState *env)
>>>>> +{
>>>>> +    APICState *s = DO_UPCAST(APICState, busdev.qdev, env->apic_state);
>>>>
>>>> Please pass APICState instead of CPUState.
>>>
>>> DeviceState, I suppose. Yes, makes more sense, update will follow.
>>
>> On second look: no, I'll keep it as is. All kvm_get/put_* helpers have
>> this kind of signature, i.e. are working against env.
> 
> There's kvm_get_supported_msrs for example.
> 
>> kvm_get/put_apic
>> just happens to be implemented outside of target-i386/kvm.c. And they
>> require both APIC and CPUState anyway, so it makes no difference.
> 
> It does, passing CPUState violates layering. Please split the
> functions so that the ioctl calls which need CPUState go to kvm.c. For
> example, the functions in kvm/apic.c could just perform copying from
> kvm_lapic_state fields to APICstate fields and vice versa.

That's a good idea.

> 
> The KVM interface by the way does not look so clever. Why isn't there
> just an array of 32 bit fields so the casts can be avoided? Perhaps
> APICState should be (later) changed to match KVM version so that the
> structure can be passed directly without copying.

Wouldn't that complicate the use in the user space model again? At least
for registers that are used with both backends.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]