Re: [PATCH 07/10] provide apic-kvm

[Jan Kiszka <jan.kiszka@siemens.com>]

Glauber Costa wrote:
> This patch provides the file apic-kvm.c, which implements a schim over
> the kvm in-kernel APIC.
> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  Makefile.target   |    2 +-
>  hw/apic-kvm.c     |  157 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pc.c           |    6 ++-
>  hw/pc.h           |    2 +
>  kvm.h             |    5 ++
>  target-i386/cpu.h |    4 ++
>  target-i386/kvm.c |   25 ++++++++-
>  7 files changed, 197 insertions(+), 4 deletions(-)
>  create mode 100644 hw/apic-kvm.c
> 
> diff --git a/Makefile.target b/Makefile.target
> index bc5263e..f00af07 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -213,7 +213,7 @@ obj-i386-y += usb-uhci.o vmmouse.o vmport.o vmware_vga.o hpet.o
>  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
>  obj-i386-y += ne2000-isa.o debugcon.o multiboot.o
>  
> -obj-i386-$(CONFIG_KVM) += ioapic-kvm.o i8259-kvm.o
> +obj-i386-$(CONFIG_KVM) += ioapic-kvm.o i8259-kvm.o apic-kvm.o
>  
>  # shared objects
>  obj-ppc-y = ppc.o ide/core.o ide/qdev.o ide/isa.o ide/pci.o ide/macio.o
> diff --git a/hw/apic-kvm.c b/hw/apic-kvm.c
> new file mode 100644
> index 0000000..089fa45
> --- /dev/null
> +++ b/hw/apic-kvm.c
> @@ -0,0 +1,157 @@
> +#include "hw.h"
> +#include "pc.h"
> +#include "pci.h"
> +#include "msix.h"
> +#include "qemu-timer.h"
> +#include "host-utils.h"
> +#include "kvm.h"
> +
> +#define APIC_LVT_NB      6
> +#define APIC_LVT_LINT0   3
> +
> +struct qemu_lapic_state {
> +    uint32_t apicbase;
> +    uint8_t id;
> +    uint8_t arb_id;
> +    uint8_t tpr;
> +    uint32_t spurious_vec;
> +    uint8_t log_dest;
> +    uint8_t dest_mode;
> +    uint32_t isr[8];  /* in service register */
> +    uint32_t tmr[8];  /* trigger mode register */
> +    uint32_t irr[8]; /* interrupt request register */
> +    uint32_t lvt[APIC_LVT_NB];
> +    uint32_t esr; /* error register */
> +    uint32_t icr[2];
> +
> +    uint32_t divide_conf;
> +    int count_shift;
> +    uint32_t initial_count;
> +    int64_t initial_count_load_time, next_time;
> +    uint32_t idx;
> +    int sipi_vector;
> +    int wait_for_sipi;
> +};
> +
> +typedef struct APICState {
> +    CPUState *cpu_env;
> +
> +/* KVM lapic structure is just a big array of regs. But it is what kvm
> + * functions expect. So have both the fields separated, for easy access,
> + * and the kvm stucture, for ioctls communications */
> +    union {
> +        struct qemu_lapic_state dev;
> +        struct kvm_lapic_state kvm_lapic_state;

That looks fishy to me on second sight: Is, e.g., loading the
kvm_lapic_state from the kernel supposed to magically fill the (totally
unaligned) qemu_lapic_state structure? I'm missing the translations of
kvm_kernel_lapic_load_from_user/save_to_user here or some effort to
arrange qemu_lapic_state in a way that it robustly maps on the register
array passed to/from the kernel (if that is possible, haven't checked yet).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux