From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH v4 12/15] kvm: x86: Add user space part for in-kernel APIC Date: Sat, 10 Dec 2011 16:58:38 +0100 Message-ID: <4EE381AE.6060702@web.de> References: <0ee1c865a905e27e7776e129d21c2ccf411ffd63.1323345150.git.jan.kiszka@siemens.com> <4EE1BCA5.80709@siemens.com> <4EE1BE54.2090509@siemens.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig57FEFC00845CE7331B75C2A9" Cc: Avi Kivity , Marcelo Tosatti , "kvm@vger.kernel.org" , qemu-devel , Anthony Liguori , "Michael S. Tsirkin" , Lai Jiangshan To: Blue Swirl Return-path: Received: from fmmailgate02.web.de ([217.72.192.227]:49446 "EHLO fmmailgate02.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750826Ab1LJP6p (ORCPT ); Sat, 10 Dec 2011 10:58:45 -0500 Received: from moweb001.kundenserver.de (moweb001.kundenserver.de [172.19.20.114]) by fmmailgate02.web.de (Postfix) with ESMTP id 61BB61BB293D2 for ; Sat, 10 Dec 2011 16:58:42 +0100 (CET) In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig57FEFC00845CE7331B75C2A9 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 2011-12-10 16:40, Blue Swirl wrote: > On Fri, Dec 9, 2011 at 07:52, Jan Kiszka 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 wr= ote: >>>>> This introduces the alternative APIC backend which makes use of KVM= 's >>>>> in-kernel device model. External NMI injection via LINT1 is emulate= d 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 mod= el is >>>>> in use. >>>>> >>>>> CC: Lai Jiangshan >>>>> Signed-off-by: Jan Kiszka >>>>> --- >>>>> 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 +=3D vmport.o >>>>> obj-i386-y +=3D device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.= o >>>>> obj-i386-y +=3D debugcon.o multiboot.o >>>>> obj-i386-y +=3D pc_piix.o >>>>> -obj-i386-$(CONFIG_KVM) +=3D kvm/clock.o >>>>> +obj-i386-$(CONFIG_KVM) +=3D kvm/clock.o kvm/apic.o >>>>> obj-i386-$(CONFIG_SPICE) +=3D 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 >>>>> + * >>>>> + * 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))) =3D val; >>>>> +} >>>>> + >>>>> +static inline uint32_t kvm_apic_get_reg(struct kvm_lapic_state *ka= pic, >>>>> + int reg_id) >>>>> +{ >>>>> + return *((uint32_t *)(kapic->regs + (reg_id << 4))); >>>>> +} >>>>> + >>>>> +int kvm_put_apic(CPUState *env) >>>>> +{ >>>>> + APICState *s =3D DO_UPCAST(APICState, busdev.qdev, env->apic_s= tate); >>>> >>>> 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. >=20 > There's kvm_get_supported_msrs for example. >=20 >> 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. >=20 > 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. >=20 > 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 --------------enig57FEFC00845CE7331B75C2A9 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk7jga4ACgkQitSsb3rl5xSSLACgtn+i9reZnie5nd1cdodrnAah aL8An0ljMFKsLRFKeLl6v1QNCV0v2zUp =+oHL -----END PGP SIGNATURE----- --------------enig57FEFC00845CE7331B75C2A9--