On 2011-12-10 16:40, Blue Swirl wrote:
> On Fri, Dec 9, 2011 at 07:52, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
>>>>> ---
>>>>> 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@xxxxxxxxxxx>
>>>>> + *
>>>>> + * 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:
signature.asc
Description: OpenPGP digital signature
