Re: [RFC PATCH 04/45] KVM: arm/arm64: vgic-new: Add data structure definitions

[Christoffer Dall <christoffer.dall@linaro.org>]

On Tue, Apr 05, 2016 at 02:34:42PM +0100, Andre Przywara wrote:
> Hi,
> 
> On 29/03/16 14:09, Christoffer Dall wrote:
> > On Fri, Mar 25, 2016 at 02:04:27AM +0000, Andre Przywara wrote:
> >> From: Christoffer Dall <christoffer.dall@linaro.org>
> >>
> >> Add a new header file for the new and improved GIC implementation.
> >> The big change is that we now have a struct vgic_irq per IRQ instead
> >> of spreading all the information over various bitmaps.
> >>
> >> We include this new header conditionally from within the old header
> >> file for the time being to avoid touching all the users.
> >>
> >> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >> ---
> >>  include/kvm/arm_vgic.h  |   5 ++
> >>  include/kvm/vgic/vgic.h | 198 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 203 insertions(+)
> >>  create mode 100644 include/kvm/vgic/vgic.h
> >>
> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> >> index 7656a46..db289a2 100644
> >> --- a/include/kvm/arm_vgic.h
> >> +++ b/include/kvm/arm_vgic.h
> >> @@ -19,6 +19,10 @@
> >>  #ifndef __ASM_ARM_KVM_VGIC_H
> >>  #define __ASM_ARM_KVM_VGIC_H
> >>  
> >> +#ifdef CONFIG_KVM_NEW_VGIC
> >> +#include <kvm/vgic/vgic.h>
> >> +#else
> >> +
> >>  #include <linux/kernel.h>
> >>  #include <linux/kvm.h>
> >>  #include <linux/irqreturn.h>
> >> @@ -376,4 +380,5 @@ static inline int vgic_v3_probe(struct device_node *vgic_node,
> >>  }
> >>  #endif
> >>  
> >> +#endif	/* old VGIC include */
> >>  #endif
> >> diff --git a/include/kvm/vgic/vgic.h b/include/kvm/vgic/vgic.h
> >> new file mode 100644
> >> index 0000000..659f8b1
> >> --- /dev/null
> >> +++ b/include/kvm/vgic/vgic.h
> >> @@ -0,0 +1,198 @@
> >> +/*
> >> + * Copyright (C) 2015, 2016 ARM Ltd.
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >> + */
> >> +#ifndef __ASM_ARM_KVM_VGIC_VGIC_H
> >> +#define __ASM_ARM_KVM_VGIC_VGIC_H
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/kvm.h>
> >> +#include <linux/irqreturn.h>
> >> +#include <linux/spinlock.h>
> >> +#include <linux/types.h>
> >> +#include <kvm/iodev.h>
> >> +
> >> +#define VGIC_V3_MAX_CPUS	255
> >> +#define VGIC_V2_MAX_CPUS	8
> >> +#define VGIC_NR_IRQS_LEGACY     256
> >> +#define VGIC_NR_SGIS		16
> >> +#define VGIC_NR_PPIS		16
> >> +#define VGIC_NR_PRIVATE_IRQS	(VGIC_NR_SGIS + VGIC_NR_PPIS)
> >> +#define VGIC_MAX_PRIVATE	(VGIC_NR_PRIVATE_IRQS - 1)
> >> +#define VGIC_MAX_SPI		1019
> >> +#define VGIC_MAX_RESERVED	1023
> >> +#define VGIC_MIN_LPI		8192
> >> +
> >> +enum vgic_type {
> >> +	VGIC_V2,		/* Good ol' GICv2 */
> >> +	VGIC_V3,		/* New fancy GICv3 */
> >> +};
> >> +
> >> +/* same for all guests, as depending only on the _host's_ GIC model */
> >> +struct vgic_global {
> >> +	/* type of the host GIC */
> >> +	enum vgic_type		type;
> >> +
> >> +	/* Physical address of vgic virtual cpu interface */
> >> +	phys_addr_t		vcpu_base;
> >> +
> >> +	/* virtual control interface mapping */
> >> +	void __iomem		*vctrl_base;
> >> +
> >> +	/* Number of implemented list registers */
> >> +	int			nr_lr;
> >> +
> >> +	/* Maintenance IRQ number */
> >> +	unsigned int		maint_irq;
> >> +
> >> +	/* maximum number of VCPUs allowed (GICv2 limits us to 8) */
> >> +	int			max_gic_vcpus;
> >> +
> >> +	/* Only needed for the legacy KVM_CREATE_IRQCHIP */
> >> +	bool			can_emulate_gicv2;
> >> +};
> >> +
> >> +extern struct vgic_global kvm_vgic_global_state;
> >> +
> >> +#define VGIC_V2_MAX_LRS		(1 << 6)
> >> +#define VGIC_V3_MAX_LRS		16
> >> +#define VGIC_V3_LR_INDEX(lr)	(VGIC_V3_MAX_LRS - 1 - lr)
> >> +
> >> +enum vgic_irq_config {
> >> +	VGIC_CONFIG_EDGE = 0,
> >> +	VGIC_CONFIG_LEVEL
> >> +};
> >> +
> >> +struct vgic_irq {
> >> +	spinlock_t irq_lock;		/* Protects the content of the struct */
> >> +	struct list_head ap_list;
> >> +
> >> +	struct kvm_vcpu *vcpu;		/* SGIs and PPIs: The VCPU
> >> +					 * SPIs and LPIs: The VCPU whose ap_list
> >> +					 * on which this is queued.
> >> +					 */
> >> +
> >> +	struct kvm_vcpu *target_vcpu;	/* The VCPU that this interrupt should
> >> +					 * be send to, as a result of the
> >> +					 * targets reg (v2) or the
> >> +					 * affinity reg (v3).
> >> +					 */
> >> +
> >> +	u32 intid;			/* Guest visible INTID */
> >> +	bool pending;
> >> +	bool line_level;		/* Level only */
> >> +	bool soft_pending;		/* Level only */
> >> +	bool active;			/* not used for LPIs */
> >> +	bool enabled;
> >> +	bool hw;			/* Tied to HW IRQ */
> >> +	u32 hwintid;			/* HW INTID number */
> >> +	union {
> >> +		u8 targets;			/* GICv2 target VCPUs mask */
> >> +		u32 mpidr;			/* GICv3 target VCPU */
> >> +	};
> >> +	u8 source;			/* GICv2 SGIs only */
> >> +	u8 priority;
> >> +	enum vgic_irq_config config;	/* Level or edge */
> >> +};
> >> +
> >> +struct vgic_dist {
> >> +	bool			in_kernel;
> >> +	bool			ready;
> >> +
> >> +	/* vGIC model the kernel emulates for the guest (GICv2 or GICv3) */
> >> +	u32			vgic_model;
> >> +
> >> +	int			nr_spis;
> >> +
> >> +	/* TODO: Consider moving to global state */
> >> +	/* Virtual control interface mapping */
> >> +	void __iomem		*vctrl_base;
> >> +
> >> +	/* base addresses in guest physical address space: */
> >> +	gpa_t			vgic_dist_base;		/* distributor */
> >> +	union {
> >> +		/* either a GICv2 CPU interface */
> >> +		gpa_t			vgic_cpu_base;
> >> +		/* or a number of GICv3 redistributor regions */
> >> +		gpa_t			vgic_redist_base;
> >> +	};
> >> +
> >> +	/* distributor enabled */
> >> +	u32			enabled;
> >> +
> >> +	struct vgic_irq		*spis;
> >> +};
> >> +
> >> +struct vgic_v2_cpu_if {
> >> +	u32		vgic_hcr;
> >> +	u32		vgic_vmcr;
> >> +	u32		vgic_misr;	/* Saved only */
> >> +	u64		vgic_eisr;	/* Saved only */
> >> +	u64		vgic_elrsr;	/* Saved only */
> >> +	u32		vgic_apr;
> >> +	u32		vgic_lr[VGIC_V2_MAX_LRS];
> >> +};
> >> +
> >> +struct vgic_v3_cpu_if {
> >> +#ifdef CONFIG_KVM_ARM_VGIC_V3
> >> +	u32		vgic_hcr;
> >> +	u32		vgic_vmcr;
> >> +	u32		vgic_sre;	/* Restored only, change ignored */
> >> +	u32		vgic_misr;	/* Saved only */
> >> +	u32		vgic_eisr;	/* Saved only */
> >> +	u32		vgic_elrsr;	/* Saved only */
> >> +	u32		vgic_ap0r[4];
> >> +	u32		vgic_ap1r[4];
> >> +	u64		vgic_lr[VGIC_V3_MAX_LRS];
> >> +#endif
> >> +};
> >> +
> >> +struct vgic_cpu {
> >> +	/* CPU vif control registers for world switch */
> >> +	union {
> >> +		struct vgic_v2_cpu_if	vgic_v2;
> >> +		struct vgic_v3_cpu_if	vgic_v3;
> >> +	};
> >> +
> >> +	/* TODO: Move nr_lr to a global state */
> > 
> > what is our current plan and status about this TODO?
> 
> The point is that the current HYP code is accessing this field. We do
> have this field in the current "global" struct vgic_params there as
> well, but this struct is (more or less) private to vgic-v2.c, so not
> easily accessible from virt/kvm/arm/hyp/*.c.
> So I suggest we keep this in here for the time being and eventually
> remove it (and rework the save/restore code) once we get rid of the old
> VGIC code.
> 
Hmmm, I haven't tried it, but I don't understand why moving it now is
difficult.  I'd really like to avoid having a bunch of todo's lingering
around and having duplicated state in the new code.

I think I noticed a number of places in this patch series which refers
tot he vgic_cpu->nr_lr instead of the global place already...

-Christoffer