[PATCH v3 16/19] arm/arm64: KVM: add virtual GICv3 distributor emulation
Christoffer Dall
christoffer.dall at linaro.org
Fri Nov 7 06:30:40 PST 2014
On Fri, Oct 31, 2014 at 05:26:51PM +0000, Andre Przywara wrote:
> With everything separated and prepared, we implement a model of a
> GICv3 distributor and redistributors by using the existing framework
> to provide handler functions for each register group.
new paragraph
> Currently we limit the emulation to a model enforcing a single
> security state, with SRE==1 (forcing system register access) and
> ARE==1 (allowing more than 8 VCPUs).
new paragraph
> We share some of functions provided for GICv2 emulation, but take
> the different ways of addressing (v)CPUs into account.
> Save and restore is currently not implemented.
>
> Similar to the split-off GICv2 specific code, the new emulation code
> goes into a new file (vgic-v3-emul.c).
>
> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> ---
> arch/arm64/kvm/Makefile | 1 +
> include/kvm/arm_vgic.h | 10 +-
> include/linux/irqchip/arm-gic-v3.h | 26 ++
> include/linux/kvm_host.h | 1 +
> include/uapi/linux/kvm.h | 2 +
> virt/kvm/arm/vgic-v3-emul.c | 891 ++++++++++++++++++++++++++++++++++++
> virt/kvm/arm/vgic.c | 11 +-
> virt/kvm/arm/vgic.h | 3 +
> 8 files changed, 942 insertions(+), 3 deletions(-)
> create mode 100644 virt/kvm/arm/vgic-v3-emul.c
>
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index d957353..4e6e09e 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -24,5 +24,6 @@ kvm-$(CONFIG_KVM_ARM_VGIC) += $(KVM)/arm/vgic-v2.o
> kvm-$(CONFIG_KVM_ARM_VGIC) += $(KVM)/arm/vgic-v2-emul.o
> kvm-$(CONFIG_KVM_ARM_VGIC) += vgic-v2-switch.o
> kvm-$(CONFIG_KVM_ARM_VGIC) += $(KVM)/arm/vgic-v3.o
> +kvm-$(CONFIG_KVM_ARM_VGIC) += $(KVM)/arm/vgic-v3-emul.o
> kvm-$(CONFIG_KVM_ARM_VGIC) += vgic-v3-switch.o
> kvm-$(CONFIG_KVM_ARM_TIMER) += $(KVM)/arm/arch_timer.o
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 8827bc7..c303083 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -160,7 +160,11 @@ struct vgic_dist {
>
> /* Distributor and vcpu interface mapping in the guest */
> phys_addr_t vgic_dist_base;
> - phys_addr_t vgic_cpu_base;
> + /* GICv2 and GICv3 use different mapped register blocks */
> + union {
> + phys_addr_t vgic_cpu_base;
> + phys_addr_t vgic_redist_base;
> + };
>
> /* Distributor enabled */
> u32 enabled;
> @@ -222,6 +226,9 @@ struct vgic_dist {
> */
> struct vgic_bitmap *irq_spi_target;
>
> + /* Target MPIDR for each IRQ (needed for GICv3 IROUTERn) only */
> + u32 *irq_spi_mpidr;
> +
> /* Bitmap indicating which CPU has something pending */
> unsigned long *irq_pending_on_cpu;
>
> @@ -297,6 +304,7 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
> void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
> int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
> bool level);
> +void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
> int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
> bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> struct kvm_exit_mmio *mmio);
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 03a4ea3..6a649bc 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -33,6 +33,7 @@
> #define GICD_SETSPI_SR 0x0050
> #define GICD_CLRSPI_SR 0x0058
> #define GICD_SEIR 0x0068
> +#define GICD_IGROUPR 0x0080
> #define GICD_ISENABLER 0x0100
> #define GICD_ICENABLER 0x0180
> #define GICD_ISPENDR 0x0200
> @@ -41,14 +42,31 @@
> #define GICD_ICACTIVER 0x0380
> #define GICD_IPRIORITYR 0x0400
> #define GICD_ICFGR 0x0C00
> +#define GICD_IGRPMODR 0x0D00
> +#define GICD_NSACR 0x0E00
> #define GICD_IROUTER 0x6000
> +#define GICD_IDREGS 0xFFD0
> #define GICD_PIDR2 0xFFE8
>
> +/*
> + * Non-ARE distributor registers, needed to provide the RES0
> + * semantics for KVM's emulated GICv3
> + */
huh? I think this comment as to do a better job at explaining this, or,
just go away.
Why are we re-defining these registers? Is it just a conincidence that
the offsets happen to be the same as for GICv2 so it would be
semantically incorrect to reuse the defines, or?
> +#define GICD_ITARGETSR 0x0800
> +#define GICD_SGIR 0x0F00
> +#define GICD_CPENDSGIR 0x0F10
> +#define GICD_SPENDSGIR 0x0F20
> +
> +
> #define GICD_CTLR_RWP (1U << 31)
> +#define GICD_CTLR_DS (1U << 6)
> #define GICD_CTLR_ARE_NS (1U << 4)
> #define GICD_CTLR_ENABLE_G1A (1U << 1)
> #define GICD_CTLR_ENABLE_G1 (1U << 0)
>
> +#define GICD_TYPER_LPIS (1U << 17)
> +#define GICD_TYPER_MBIS (1U << 16)
> +
> #define GICD_IROUTER_SPI_MODE_ONE (0U << 31)
> #define GICD_IROUTER_SPI_MODE_ANY (1U << 31)
>
> @@ -56,6 +74,8 @@
> #define GIC_PIDR2_ARCH_GICv3 0x30
> #define GIC_PIDR2_ARCH_GICv4 0x40
>
> +#define GIC_V3_DIST_SIZE 0x10000
> +
> /*
> * Re-Distributor registers, offsets from RD_base
> */
> @@ -74,6 +94,7 @@
> #define GICR_SYNCR 0x00C0
> #define GICR_MOVLPIR 0x0100
> #define GICR_MOVALLR 0x0110
> +#define GICR_IDREGS GICD_IDREGS
> #define GICR_PIDR2 GICD_PIDR2
>
> #define GICR_WAKER_ProcessorSleep (1U << 1)
> @@ -82,6 +103,7 @@
> /*
> * Re-Distributor registers, offsets from SGI_base
> */
> +#define GICR_IGROUPR0 GICD_IGROUPR
> #define GICR_ISENABLER0 GICD_ISENABLER
> #define GICR_ICENABLER0 GICD_ICENABLER
> #define GICR_ISPENDR0 GICD_ISPENDR
> @@ -90,10 +112,14 @@
> #define GICR_ICACTIVER0 GICD_ICACTIVER
> #define GICR_IPRIORITYR0 GICD_IPRIORITYR
> #define GICR_ICFGR0 GICD_ICFGR
> +#define GICR_IGRPMODR0 GICD_IGRPMODR
> +#define GICR_NSACR GICD_NSACR
>
> #define GICR_TYPER_VLPIS (1U << 1)
> #define GICR_TYPER_LAST (1U << 4)
>
> +#define GIC_V3_REDIST_SIZE 0x20000
> +
> /*
> * CPU interface registers
> */
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 326ba7a..4a7798e 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1085,6 +1085,7 @@ void kvm_unregister_device_ops(u32 type);
> extern struct kvm_device_ops kvm_mpic_ops;
> extern struct kvm_device_ops kvm_xics_ops;
> extern struct kvm_device_ops kvm_arm_vgic_v2_ops;
> +extern struct kvm_device_ops kvm_arm_vgic_v3_ops;
>
> #ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
>
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 6076882..24cb129 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -960,6 +960,8 @@ enum kvm_device_type {
> #define KVM_DEV_TYPE_ARM_VGIC_V2 KVM_DEV_TYPE_ARM_VGIC_V2
> KVM_DEV_TYPE_FLIC,
> #define KVM_DEV_TYPE_FLIC KVM_DEV_TYPE_FLIC
> + KVM_DEV_TYPE_ARM_VGIC_V3,
> +#define KVM_DEV_TYPE_ARM_VGIC_V3 KVM_DEV_TYPE_ARM_VGIC_V3
You need to document this device type in
Documentation/virtual/kvm/devices/ (probably in arm-vgic.txt).
That goes for patch 19 as well, but I'll remind you when I look at that
patch more closely.
> KVM_DEV_TYPE_MAX,
> };
>
> diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
> new file mode 100644
> index 0000000..bcb5374
> --- /dev/null
> +++ b/virt/kvm/arm/vgic-v3-emul.c
> @@ -0,0 +1,891 @@
> +/*
> + * GICv3 distributor and redistributor emulation on GICv3 hardware
> + *
> + * able to run on a pure native host GICv3 (which forces ARE=1)
> + *
> + * forcing ARE=1 and DS=1, not covering LPIs yet (TYPER.LPIS=0)
I think the above two lines require rewriting, may I suggest:
GICv3 emulation is currently only supported on a GICv3 host, but
supports both hardware with or without the optional GICv2 backwards
compatibility features.
We emulate a GICv3 without the backwards compatibility features (meaning
the emulated GICD_CTLR.ARE resets to 1 and is RAO/WI) and with only a
single security state (the emulated GICD_CTLR.DS=1, RAO/WI). This
emulated GICv3 does not yet include support for LPIs (TYPER.LIPS=0,
RAZ/WI).
But pay particular attention to the bit about us emulating a GICv3 with
only a single security state, because you're implementing GICD_IGROUPR
and GICR_IGROUPR as RAZ/WI, which is then a limitation of the emulated
GIC (just like we don't emulate priorities), which is fine, but let's
then state that as such.
> + *
> + * Copyright (C) 2014 ARM Ltd.
> + * Author: Andre Przywara <andre.przywara at arm.com>
> + *
> + * 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/>.
> + */
> +
> +#include <linux/cpu.h>
> +#include <linux/kvm.h>
> +#include <linux/kvm_host.h>
> +#include <linux/interrupt.h>
> +
> +#include <linux/irqchip/arm-gic-v3.h>
> +#include <kvm/arm_vgic.h>
> +
> +#include <asm/kvm_emulate.h>
> +#include <asm/kvm_arm.h>
> +#include <asm/kvm_mmu.h>
> +
> +#include "vgic.h"
> +
> +#define INTERRUPT_ID_BITS 10
> +
> +static bool handle_mmio_misc(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio, phys_addr_t offset,
> + void *private)
> +{
> + u32 reg = 0, val;
> + u32 word_offset = offset & 3;
> +
> + switch (offset & ~3) {
> + case GICD_CTLR:
> + /*
> + * Force ARE and DS to 1, the guest cannot change this.
> + * For the time being we only support Group1 interrupts.
> + */
> + if (vcpu->kvm->arch.vgic.enabled)
> + reg = GICD_CTLR_ENABLE_G1A;
> + reg |= GICD_CTLR_ARE_NS | GICD_CTLR_DS;
> +
> + vgic_reg_access(mmio, ®, word_offset,
> + ACCESS_READ_VALUE | ACCESS_WRITE_VALUE);
> + if (mmio->is_write) {
> + vcpu->kvm->arch.vgic.enabled = !!(reg & GICD_CTLR_ENABLE_G1A);
> + vgic_update_state(vcpu->kvm);
> + return true;
> + }
> + break;
so we don't implement read-as-written for this register, should we at
least print a warning or something if the guest tries to enable group 0
interrupts?
> + case GICD_TYPER:
> + /*
> + * as this implementation does not provide compatibility
Upper-case ^
> + * with GICv2 (ARE==1), we report zero CPUs in the lower 5 bits.
lower 5 bits? You mean we report bits [7:5] as 000 right?
> + * Also TYPER.LPIS is 0 for now and TYPER.MBIS is not supported.
drop the 'for now' just say we report TYPER.LPIS=0 and TYPER.MBIS=0;
because we don't support LBIs or MBIs.
> + */
> +
> + /* claim we support at most 1024 (-4) SPIs via this interface */
claim? Does this not hold in reality? It doesn't seem to be what the
code does. I'm doubting the usefulnes of this comment.
> + val = min(vcpu->kvm->arch.vgic.nr_irqs, 1024);
> + reg |= (val >> 5) - 1;
> +
> + reg |= (INTERRUPT_ID_BITS - 1) << 19;
but it happens that we have no explanation about the arbitrarily chosen
10 bits?
> +
> + vgic_reg_access(mmio, ®, word_offset,
> + ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED);
> + break;
> + case GICD_IIDR:
> + reg = (PRODUCT_ID_KVM << 24) | (IMPLEMENTER_ARM << 0);
> + vgic_reg_access(mmio, ®, word_offset,
> + ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED);
> + break;
> + default:
> + vgic_reg_access(mmio, NULL, word_offset,
> + ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);
> + break;
> + }
I'm getting increasingly skeptic about the value of combining these
registers into a single misc function?
> +
> + return false;
> +}
> +
> +static bool handle_mmio_set_enable_reg_dist(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset,
> + void *private)
> +{
> + if (likely(offset >= VGIC_NR_PRIVATE_IRQS / 8))
> + return vgic_handle_enable_reg(vcpu->kvm, mmio, offset,
> + vcpu->vcpu_id,
> + ACCESS_WRITE_SETBIT);
> +
> + vgic_reg_access(mmio, NULL, offset & 3,
> + ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);
Somewhat general question:
This made me wonder if we check for unaligned accesses anywhere or could
the guest get away with (offset & 3) = 2 and mmio->len = 4? Then
semantics for this would start being weird...
> + return false;
> +}
> +
> +static bool handle_mmio_clear_enable_reg_dist(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset,
> + void *private)
> +{
> + if (likely(offset >= VGIC_NR_PRIVATE_IRQS / 8))
> + return vgic_handle_enable_reg(vcpu->kvm, mmio, offset,
> + vcpu->vcpu_id,
> + ACCESS_WRITE_CLEARBIT);
> +
> + vgic_reg_access(mmio, NULL, offset & 3,
> + ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);
> + return false;
> +}
> +
> +static bool handle_mmio_set_pending_reg_dist(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset,
> + void *private)
> +{
> + if (likely(offset >= VGIC_NR_PRIVATE_IRQS / 8))
> + return vgic_handle_set_pending_reg(vcpu->kvm, mmio, offset,
> + vcpu->vcpu_id);
> +
> + vgic_reg_access(mmio, NULL, offset & 3,
> + ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);
> + return false;
> +}
> +
> +static bool handle_mmio_clear_pending_reg_dist(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset,
> + void *private)
> +{
> + if (likely(offset >= VGIC_NR_PRIVATE_IRQS / 8))
> + return vgic_handle_clear_pending_reg(vcpu->kvm, mmio, offset,
> + vcpu->vcpu_id);
> +
> + vgic_reg_access(mmio, NULL, offset & 3,
> + ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);
> + return false;
> +}
> +
> +static bool handle_mmio_priority_reg_dist(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset,
> + void *private)
> +{
> + u32 *reg;
> +
> + if (unlikely(offset < VGIC_NR_PRIVATE_IRQS)) {
> + vgic_reg_access(mmio, NULL, offset & 3,
Just noticed, you don't need to mask off the upper bits all these places, do you?
I think it should be consistent with what we do in the v2 emulation.
The only place you may need to do that is in the handle_mmio_misc function.
> + ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);
> + return false;
> + }
> +
> + reg = vgic_bytemap_get_reg(&vcpu->kvm->arch.vgic.irq_priority,
> + vcpu->vcpu_id, offset);
> + vgic_reg_access(mmio, reg, offset,
> + ACCESS_READ_VALUE | ACCESS_WRITE_VALUE);
> + return false;
> +}
> +
> +static bool handle_mmio_cfg_reg_dist(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset,
> + void *private)
> +{
> + u32 *reg;
> +
> + if (unlikely(offset < VGIC_NR_PRIVATE_IRQS / 4)) {
> + vgic_reg_access(mmio, NULL, offset & 3,
> + ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);
> + return false;
> + }
> +
> + reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_cfg,
> + vcpu->vcpu_id, offset >> 1);
> +
> + return vgic_handle_cfg_reg(reg, mmio, offset);
> +}
> +
> +static u32 compress_mpidr(unsigned long mpidr)
can you comment on this function which format it returns and which
context that's useful in?
> +{
> + u32 ret;
> +
> + ret = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> + ret |= MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8;
> + ret |= MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16;
> + ret |= MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24;
> +
> + return ret;
> +}
> +
> +static unsigned long uncompress_mpidr(u32 value)
> +{
> + unsigned long mpidr;
> +
> + mpidr = ((value >> 0) & 0xFF) << MPIDR_LEVEL_SHIFT(0);
> + mpidr |= ((value >> 8) & 0xFF) << MPIDR_LEVEL_SHIFT(1);
> + mpidr |= ((value >> 16) & 0xFF) << MPIDR_LEVEL_SHIFT(2);
> + mpidr |= (u64)((value >> 24) & 0xFF) << MPIDR_LEVEL_SHIFT(3);
> +
> + return mpidr;
> +}
> +
> +/*
> + * Lookup the given MPIDR value to get the vcpu_id (if there is one)
> + * and store that in the irq_spi_cpu[] array.
> + * This limits the number of VCPUs to 255 for now, extending the data
> + * type (or storing kvm_vcpu poiners) should lift the limit.
> + * Store the original MPIDR value in an extra array.
why? To maintain read-as-written?
> + * Unallocated MPIDRs are translated to a special value and catched
s/catched/caught/
> + * before any array accesses.
> + */
> +static bool handle_mmio_route_reg(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset, void *private)
> +{
> + struct kvm *kvm = vcpu->kvm;
> + struct vgic_dist *dist = &kvm->arch.vgic;
> + int irq;
> + u32 reg;
> + int vcpu_id;
> + unsigned long *bmap, mpidr;
> + u32 word_offset = offset & 3;
> +
> + /*
> + * Private interrupts cannot be re-routed, so this register
> + * is RES0 for any IRQ < 32.
> + * Also the upper 32 bits of each 64 bit register are zero,
> + * as we don't support Aff3 and that's the only value up there.
drop the rest of the sentence after Aff3.
> + */
> + if (unlikely(offset < VGIC_NR_PRIVATE_IRQS * 8) || (offset & 4) == 4) {
you don't need the '== 4' part.
> + vgic_reg_access(mmio, NULL, word_offset,
> + ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);
> + return false;
> + }
> +
> + irq = (offset / 8) - VGIC_NR_PRIVATE_IRQS;
can we not call this irq? spi instead maybe?
> +
> + /* get the stored MPIDR for this IRQ */
> + mpidr = uncompress_mpidr(dist->irq_spi_mpidr[irq]);
> + mpidr &= MPIDR_HWID_BITMASK;
> + reg = mpidr;
> +
> + vgic_reg_access(mmio, ®, word_offset,
> + ACCESS_READ_VALUE | ACCESS_WRITE_VALUE);
> +
> + if (!mmio->is_write)
> + return false;
> +
> + /*
> + * Now clear the currently assigned vCPU from the map, making room
> + * for the new one to be written below
> + */
> + vcpu = kvm_mpidr_to_vcpu(kvm, mpidr);
> + if (likely(vcpu)) {
> + vcpu_id = vcpu->vcpu_id;
> + bmap = vgic_bitmap_get_shared_map(&dist->irq_spi_target[vcpu_id]);
> + clear_bit(irq, bmap);
this is the atomic version, right? is it known to be faster on arm64
because it's written in assembly and that's why we're using it instead
of __clear_bit?
> + }
> +
> + dist->irq_spi_mpidr[irq] = compress_mpidr(reg);
> + vcpu = kvm_mpidr_to_vcpu(kvm, reg & MPIDR_HWID_BITMASK);
> +
> + /*
> + * The spec says that non-existent MPIDR values should not be
> + * forwarded to any existent (v)CPU, but should be able to become
> + * pending anyway. We simply keep the irq_spi_target[] array empty, so
> + * the interrupt will never be injected.
> + * irq_spi_cpu[irq] gets a magic value in this case.
> + */
> + if (likely(vcpu)) {
> + vcpu_id = vcpu->vcpu_id;
> + dist->irq_spi_cpu[irq] = vcpu_id;
> + bmap = vgic_bitmap_get_shared_map(&dist->irq_spi_target[vcpu_id]);
> + set_bit(irq, bmap);
__set_bit ?
> + } else
> + dist->irq_spi_cpu[irq] = VCPU_NOT_ALLOCATED;
according to the CodingStyle (and me) this wants braces.
> +
> + vgic_update_state(kvm);
> +
> + return true;
> +}
> +
> +static bool handle_mmio_idregs(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset, void *private)
> +{
> + u32 reg = 0;
> +
> + switch (offset + GICD_IDREGS) {
> + case GICD_PIDR2:
> + reg = 0x3b;
> + break;
> + }
> +
> + vgic_reg_access(mmio, ®, offset & 3,
> + ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED);
> +
> + return false;
> +}
> +
> +static const struct mmio_range vgic_dist_ranges[] = {
can we call this vgic_v3_dist_ranges ?
> + { /*
> + * handling CTLR, TYPER, IIDR and STATUSR
> + */
this one doesn't need wings (and you're not doing that below)
> + .base = GICD_CTLR,
> + .len = 20,
nit: why do we specify this len as decimal and the others in hex?
> + .bits_per_irq = 0,
> + .handle_mmio = handle_mmio_misc,
> + },
are we not mentioning the status register here because it's optional?
> + {
> + /* when DS=1, this is RAZ/WI */
> + .base = GICD_SETSPI_SR,
> + .len = 0x04,
> + .bits_per_irq = 0,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + {
> + /* when DS=1, this is RAZ/WI */
> + .base = GICD_CLRSPI_SR,
> + .len = 0x04,
> + .bits_per_irq = 0,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
why are we only listing the _SR versions and not the _NSR versions?
> + {
> + .base = GICD_IGROUPR,
> + .len = 0x80,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
this one may warrant a TODO: Group 0 interrupts not yet supported.
> + {
> + .base = GICD_ISENABLER,
> + .len = 0x80,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_set_enable_reg_dist,
> + },
> + {
> + .base = GICD_ICENABLER,
> + .len = 0x80,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_clear_enable_reg_dist,
> + },
> + {
> + .base = GICD_ISPENDR,
> + .len = 0x80,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_set_pending_reg_dist,
> + },
> + {
> + .base = GICD_ICPENDR,
> + .len = 0x80,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_clear_pending_reg_dist,
> + },
> + {
> + .base = GICD_ISACTIVER,
> + .len = 0x80,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + {
> + .base = GICD_ICACTIVER,
> + .len = 0x80,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + {
> + .base = GICD_IPRIORITYR,
> + .len = 0x400,
> + .bits_per_irq = 8,
> + .handle_mmio = handle_mmio_priority_reg_dist,
> + },
> + {
> + /* TARGETSRn is RES0 when ARE=1 */
> + .base = GICD_ITARGETSR,
> + .len = 0x400,
> + .bits_per_irq = 8,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + {
> + .base = GICD_ICFGR,
> + .len = 0x100,
> + .bits_per_irq = 2,
> + .handle_mmio = handle_mmio_cfg_reg_dist,
> + },
> + {
> + /* this is RAZ/WI when DS=1 */
> + .base = GICD_IGRPMODR,
> + .len = 0x80,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + {
> + /* with DS==1 this is RAZ/WI */
any reason why the two comments above are not identical? (I know, I
have OCD).
> + .base = GICD_NSACR,
> + .len = 0x100,
> + .bits_per_irq = 2,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + /* the next three blocks are RES0 if ARE=1 */
probably nicer to just have a comment for each register where this
applies.
> + {
> + .base = GICD_SGIR,
> + .len = 4,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + {
> + .base = GICD_CPENDSGIR,
> + .len = 0x10,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + {
> + .base = GICD_SPENDSGIR,
> + .len = 0x10,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + {
> + .base = GICD_IROUTER,
> + .len = 0x2000,
shouldn't this be 0x1ee0?
> + .bits_per_irq = 64,
> + .handle_mmio = handle_mmio_route_reg,
> + },
> + {
> + .base = GICD_IDREGS,
> + .len = 0x30,
> + .bits_per_irq = 0,
> + .handle_mmio = handle_mmio_idregs,
> + },
> + {},
> +};
> +
> +static bool handle_mmio_set_enable_reg_redist(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset,
> + void *private)
> +{
> + struct kvm_vcpu *target_redist_vcpu = private;
> +
> + return vgic_handle_enable_reg(vcpu->kvm, mmio, offset,
> + target_redist_vcpu->vcpu_id,
> + ACCESS_WRITE_SETBIT);
> +}
> +
> +static bool handle_mmio_clear_enable_reg_redist(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset,
> + void *private)
> +{
> + struct kvm_vcpu *target_redist_vcpu = private;
> +
> + return vgic_handle_enable_reg(vcpu->kvm, mmio, offset,
> + target_redist_vcpu->vcpu_id,
> + ACCESS_WRITE_CLEARBIT);
> +}
> +
> +static bool handle_mmio_set_pending_reg_redist(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset,
> + void *private)
> +{
> + struct kvm_vcpu *target_redist_vcpu = private;
> +
> + return vgic_handle_set_pending_reg(vcpu->kvm, mmio, offset,
> + target_redist_vcpu->vcpu_id);
> +}
> +
> +static bool handle_mmio_clear_pending_reg_redist(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset,
> + void *private)
> +{
> + struct kvm_vcpu *target_redist_vcpu = private;
> +
> + return vgic_handle_clear_pending_reg(vcpu->kvm, mmio, offset,
> + target_redist_vcpu->vcpu_id);
> +}
> +
> +static bool handle_mmio_priority_reg_redist(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset,
> + void *private)
> +{
> + struct kvm_vcpu *target_redist_vcpu = private;
> + u32 *reg;
> +
> + reg = vgic_bytemap_get_reg(&vcpu->kvm->arch.vgic.irq_priority,
> + target_redist_vcpu->vcpu_id, offset);
> + vgic_reg_access(mmio, reg, offset,
> + ACCESS_READ_VALUE | ACCESS_WRITE_VALUE);
> + return false;
> +}
> +
> +static bool handle_mmio_cfg_reg_redist(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset,
> + void *private)
> +{
> + u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_cfg,
> + *(int *)private, offset >> 1);
> +
> + return vgic_handle_cfg_reg(reg, mmio, offset);
> +}
> +
> +static const struct mmio_range vgic_redist_sgi_ranges[] = {
> + {
> + .base = GICR_IGROUPR0,
> + .len = 4,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_raz_wi,
shouldn't these RAO/WI instead?
> + },
> + {
> + .base = GICR_ISENABLER0,
> + .len = 4,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_set_enable_reg_redist,
> + },
> + {
> + .base = GICR_ICENABLER0,
> + .len = 4,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_clear_enable_reg_redist,
> + },
> + {
> + .base = GICR_ISPENDR0,
> + .len = 4,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_set_pending_reg_redist,
> + },
> + {
> + .base = GICR_ICPENDR0,
> + .len = 4,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_clear_pending_reg_redist,
> + },
> + {
> + .base = GICR_ISACTIVER0,
> + .len = 4,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + {
> + .base = GICR_ICACTIVER0,
> + .len = 4,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + {
> + .base = GICR_IPRIORITYR0,
> + .len = 32,
> + .bits_per_irq = 8,
> + .handle_mmio = handle_mmio_priority_reg_redist,
> + },
> + {
> + .base = GICR_ICFGR0,
> + .len = 8,
> + .bits_per_irq = 2,
> + .handle_mmio = handle_mmio_cfg_reg_redist,
> + },
> + {
> + .base = GICR_IGRPMODR0,
> + .len = 4,
> + .bits_per_irq = 1,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + {
> + .base = GICR_NSACR,
> + .len = 4,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + {},
> +};
> +
> +static bool handle_mmio_misc_redist(struct kvm_vcpu *vcpu,
> + struct kvm_exit_mmio *mmio,
> + phys_addr_t offset, void *private)
> +{
> + u32 reg;
> + u32 word_offset = offset & 3;
> + u64 mpidr;
> + struct kvm_vcpu *target_redist_vcpu = private;
> + int target_vcpu_id = target_redist_vcpu->vcpu_id;
> +
> + switch (offset & ~3) {
> + case GICR_CTLR:
> + /* since we don't support LPIs, this register is zero for now */
> + vgic_reg_access(mmio, ®, word_offset,
> + ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);
> + break;
> + case GICR_TYPER + 4:
> + mpidr = kvm_vcpu_get_mpidr(target_redist_vcpu);
> + reg = compress_mpidr(mpidr);
> +
> + vgic_reg_access(mmio, ®, word_offset,
> + ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED);
> + break;
> + case GICR_TYPER:
> + reg = target_redist_vcpu->vcpu_id << 8;
> + if (target_vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1)
> + reg |= GICR_TYPER_LAST;
> + vgic_reg_access(mmio, ®, word_offset,
> + ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED);
> + break;
> + case GICR_IIDR:
> + reg = (PRODUCT_ID_KVM << 24) | (IMPLEMENTER_ARM << 0);
> + vgic_reg_access(mmio, ®, word_offset,
> + ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED);
> + break;
the fact that you could reuse handle_mmio_iidr directly here and that
GICR_TYPER reads funny here, indicates to me that we should once again
split this up into smaller functions.
> + default:
> + vgic_reg_access(mmio, NULL, word_offset,
> + ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);
> + break;
> + }
> +
> + return false;
> +}
> +
> +static const struct mmio_range vgic_redist_ranges[] = {
> + { /*
> + * handling CTLR, IIDR, TYPER and STATUSR
> + */
> + .base = GICR_CTLR,
> + .len = 20,
> + .bits_per_irq = 0,
> + .handle_mmio = handle_mmio_misc_redist,
> + },
> + {
> + .base = GICR_WAKER,
> + .len = 4,
> + .bits_per_irq = 0,
> + .handle_mmio = handle_mmio_raz_wi,
> + },
> + {
> + .base = GICR_IDREGS,
> + .len = 0x30,
> + .bits_per_irq = 0,
> + .handle_mmio = handle_mmio_idregs,
> + },
> + {},
> +};
> +
> +/*
> + * this is the stub handling both dist and redist MMIO exits for v3
This
Is this really a stub?
I would suggest spelling out distributor and re-distributor and GICv3.
Full stop after GICv3.
> + * does some vcpu_id calculation on the redist MMIO to use a possibly
> + * different VCPU than the current one
"some vcpu_id calculation" is not very helpful, either explain the magic
sauce, or just say in which way a "different" VCPU is something we need
to pay special attention to.
If I read the code correctly, the comment shoudl simply be:
The GICv3 spec allows any CPU to access any redistributor through the
memory-mapped redistributor registers. We can therefore determine which
reditributor is being accesses by simply looking at the faulting IPA.
> + */
> +static bool vgic_v3_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> + struct kvm_exit_mmio *mmio)
> +{
> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> + unsigned long dbase = dist->vgic_dist_base;
> + unsigned long rdbase = dist->vgic_redist_base;
I'm not crazy about these 'shortcuts', especially given that RD_base is
the base of a specific redistributor, but ok.
> + int nrcpus = atomic_read(&vcpu->kvm->online_vcpus);
> + int vcpu_id;
> + struct kvm_vcpu *target_redist_vcpu;
> +
> + if (is_in_range(mmio->phys_addr, mmio->len, dbase, GIC_V3_DIST_SIZE)) {
> + return vgic_handle_mmio_range(vcpu, run, mmio,
> + vgic_dist_ranges, dbase, NULL);
> + }
> +
> + if (!is_in_range(mmio->phys_addr, mmio->len, rdbase,
> + GIC_V3_REDIST_SIZE * nrcpus))
> + return false;
so this implies that all redistributors will always be in contiguous IPA
space, is this reasonable?
> +
> + vcpu_id = (mmio->phys_addr - rdbase) / GIC_V3_REDIST_SIZE;
> + rdbase += (vcpu_id * GIC_V3_REDIST_SIZE);
> + target_redist_vcpu = kvm_get_vcpu(vcpu->kvm, vcpu_id);
redist_vcpu should be enough
> +
> + if (mmio->phys_addr >= rdbase + 0x10000)
> + return vgic_handle_mmio_range(vcpu, run, mmio,
> + vgic_redist_sgi_ranges,
> + rdbase + 0x10000,
> + target_redist_vcpu);
0x10000 magic number used twice, GICV3_REDIST_SGI_PAGE_OFFSET or
something shorter.
perhaps it is nicer to just adjust rdbase and set a range variable above
and only have a single call to vgic_handle_mmio_range().
> +
> + return vgic_handle_mmio_range(vcpu, run, mmio, vgic_redist_ranges,
> + rdbase, target_redist_vcpu);
> +}
> +
> +static bool vgic_v3_queue_sgi(struct kvm_vcpu *vcpu, int irq)
> +{
> + if (vgic_queue_irq(vcpu, 0, irq)) {
> + vgic_dist_irq_clear_pending(vcpu, irq);
> + vgic_cpu_irq_clear(vcpu, irq);
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static int vgic_v3_init_maps(struct vgic_dist *dist)
> +{
> + int nr_spis = dist->nr_irqs - VGIC_NR_PRIVATE_IRQS;
> +
> + dist->irq_spi_mpidr = kcalloc(nr_spis, sizeof(dist->irq_spi_mpidr[0]),
> + GFP_KERNEL);
> +
> + if (!dist->irq_spi_mpidr)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int vgic_v3_init(struct kvm *kvm, const struct vgic_params *params)
> +{
> + struct vgic_dist *dist = &kvm->arch.vgic;
> + int ret, i;
> + u32 mpidr;
> +
> + if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) ||
> + IS_VGIC_ADDR_UNDEF(dist->vgic_redist_base)) {
> + kvm_err("Need to set vgic distributor addresses first\n");
> + return -ENXIO;
> + }
> +
> + /*
> + * FIXME: this should be moved to init_maps time, and may bite
> + * us when adding save/restore. Add a per-emulation hook?
> + */
What is the plan for this? Can we move it into init_maps or does that
require some more work?
Why can't we do what the gicv2 emulation does?
Not sure what the "Add a per-emulation hook?" question is asking...
> + ret = vgic_v3_init_maps(dist);
> + if (ret) {
> + kvm_err("Unable to allocate maps\n");
> + return ret;
> + }
> +
> + mpidr = compress_mpidr(kvm_vcpu_get_mpidr(kvm_get_vcpu(kvm, 0)));
> + for (i = VGIC_NR_PRIVATE_IRQS; i < dist->nr_irqs; i++) {
> + dist->irq_spi_cpu[i - VGIC_NR_PRIVATE_IRQS] = 0;
> + dist->irq_spi_mpidr[i - VGIC_NR_PRIVATE_IRQS] = mpidr;
> + vgic_bitmap_set_irq_val(dist->irq_spi_target, 0, i, 1);
why do we need 3 different copies of the same value now? ok, we had two
before because of the bitmap "optimization" thingy, but now we have two
other sets of state for the same thing...
> + }
> +
> + return 0;
> +}
> +
> +static void vgic_v3_add_sgi_source(struct kvm_vcpu *vcpu, int irq, int source)
> +{
can you put a one line comment here:
/* The GICv3 spec does away with keeping track of SGI sources */
> +}
> +
> +bool vgic_v3_init_emulation_ops(struct kvm *kvm, int type)
> +{
> + struct vgic_dist *dist = &kvm->arch.vgic;
> +
> + switch (type) {
> + case KVM_DEV_TYPE_ARM_VGIC_V3:
> + dist->vm_ops.handle_mmio = vgic_v3_handle_mmio;
> + dist->vm_ops.queue_sgi = vgic_v3_queue_sgi;
> + dist->vm_ops.add_sgi_source = vgic_v3_add_sgi_source;
> + dist->vm_ops.vgic_init = vgic_v3_init;
> + break;
> + default:
> + return false;
> + }
> + return true;
> +}
> +
> +/*
> + * triggered by a system register access trap, called from the sysregs
Triggered
> + * handling code there.
^^^ there, where, here, and everywhere ?
> + * The register contains the upper three affinity levels of the target
^^^ which register? @reg ?
> + * processors as well as a bitmask of 16 Aff0 CPUs.
Does @reg follow the format from something in the spec? That would be
useful to know...
> + * Iterate over all VCPUs to check for matching ones or signal on
> + * all-but-self if the mode bit is set.
an all-but-self IPI? Is that the architectural term? Otherwise I would
suggest something like: If not VCPUs are found which match reg (in some
way), then send the IPI to all VCPUs in the VM, except the one
performing the system register acces.
> + */
Also, please use the kdocs format here like the rest of the kvm/arm code.
Begin sentences with upper-case, etc.:
/**
* vgic_v3_dispatch_sgi - This function does something with SGIs
* @vcpu: The vcpu pointer
* @reg: Magic
*
* Some nicer version of what you have above.
*/
> +void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
> +{
It's a bit hard to review this when I cannot see how it is called, I'm
assuming that this is on writes to ICC_SGI1R_EL1 and reg is what the
guest tried to write to that register.
I have a feeling that you may want to add this function in a separate patch.
> + struct kvm *kvm = vcpu->kvm;
> + struct kvm_vcpu *c_vcpu;
> + struct vgic_dist *dist = &kvm->arch.vgic;
> + u16 target_cpus;
> + u64 mpidr, mpidr_h, mpidr_l;
> + int sgi, mode, c, vcpu_id;
> + int updated = 0;
> +
> + vcpu_id = vcpu->vcpu_id;
> +
> + sgi = (reg >> 24) & 0xf;
> + mode = (reg >> 40) & 0x1;
perhaps we can call this 'targeted' or something to make it a bit more
clear.
> + target_cpus = reg & 0xffff;
Can you add some defines for these magic shifts? Are there not some
already for the GICv3 host driver we can reuse?
> + mpidr = ((reg >> 48) & 0xff) << MPIDR_LEVEL_SHIFT(3);
> + mpidr |= ((reg >> 32) & 0xff) << MPIDR_LEVEL_SHIFT(2);
> + mpidr |= ((reg >> 16) & 0xff) << MPIDR_LEVEL_SHIFT(1);
> + mpidr &= ~MPIDR_LEVEL_MASK;
(**) note the comment a few lines down.
> +
> + /*
> + * We take the dist lock here, because we come from the sysregs
> + * code path and not from MMIO (where this is already done)
which already takes the lock).
> + */
> + spin_lock(&dist->lock);
> + kvm_for_each_vcpu(c, c_vcpu, kvm) {
I think it would be helpful to document this loop, something like:
We loop through every possible vCPU and check if we need to send an SGI
to that vCPU. If targeting specific vCPUS, we check if the candidate
vCPU is in the target list and if it is, we send an SGI and clear the
bit in the target list. When the target list is empty and we are
targeting specific vCPUs, we are done.
Maybe too verbose, you can tweak it as you like.
> + if (!mode && target_cpus == 0)
> + break;
> + if (mode && c == vcpu_id) /* not to myself */
> + continue;
> + if (!mode) {
> + mpidr_h = kvm_vcpu_get_mpidr(c_vcpu);
> + mpidr_l = MPIDR_AFFINITY_LEVEL(mpidr_h, 0);
> + mpidr_h &= ~MPIDR_LEVEL_MASK;
this is *really* confusing. _h and _l are high and low?
Can you factor this out into a static inline and get rid of that mpidr
mask above (**) ?
> + if (mpidr != mpidr_h)
> + continue;
> + if (!(target_cpus & BIT(mpidr_l)))
> + continue;
> + target_cpus &= ~BIT(mpidr_l);
> + }
> + /* Flag the SGI as pending */
> + vgic_dist_irq_set_pending(c_vcpu, sgi);
> + updated = 1;
> + kvm_debug("SGI%d from CPU%d to CPU%d\n", sgi, vcpu_id, c);
> + }
> + if (updated)
> + vgic_update_state(vcpu->kvm);
> + spin_unlock(&dist->lock);
> + if (updated)
> + vgic_kick_vcpus(vcpu->kvm);
> +}
> +
> +
> +static int vgic_v3_get_attr(struct kvm_device *dev,
> + struct kvm_device_attr *attr)
> +{
> + int ret;
> +
> + ret = vgic_get_common_attr(dev, attr);
So this means we can get the KVM_VGIC_V2_ADDR_TYPE_DIST and
KVM_VGIC_V2_ADDR_TYPE_CPU from an emualted gicv3 without the GICv2
backwards compatibility features?
> + if (ret != -ENXIO)
> + return ret;
> +
> + switch (attr->group) {
> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> + case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> + return -ENXIO;
> + }
> +
> + return -ENXIO;
> +}
> +
> +static int vgic_v3_set_attr(struct kvm_device *dev,
> + struct kvm_device_attr *attr)
> +{
> + int ret;
> +
> + ret = vgic_set_common_attr(dev, attr);
same as above?
> + if (ret != -ENXIO)
> + return ret;
> +
> + switch (attr->group) {
> + case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> + return -ENXIO;
> + }
> +
> + return -ENXIO;
> +}
> +
> +static int vgic_v3_has_attr(struct kvm_device *dev,
> + struct kvm_device_attr *attr)
> +{
> + switch (attr->group) {
> + case KVM_DEV_ARM_VGIC_GRP_ADDR:
> + switch (attr->attr) {
> + case KVM_VGIC_V2_ADDR_TYPE_DIST:
> + case KVM_VGIC_V2_ADDR_TYPE_CPU:
> + return -ENXIO;
> + }
> + break;
> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> + case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> + return -ENXIO;
> + case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
> + return 0;
> + }
> + return -ENXIO;
> +}
> +
> +struct kvm_device_ops kvm_arm_vgic_v3_ops = {
> + .name = "kvm-arm-vgic-v3",
> + .create = vgic_create,
> + .destroy = vgic_destroy,
> + .set_attr = vgic_v3_set_attr,
> + .get_attr = vgic_v3_get_attr,
> + .has_attr = vgic_v3_has_attr,
nit: you could reorder set/get so they're set in the same order they
appear in the code.
> +};
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index a54389b..2867269d 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1228,7 +1228,7 @@ static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> struct kvm_vcpu *vcpu;
> int edge_triggered, level_triggered;
> int enabled;
> - bool ret = true;
> + bool ret = true, can_inject = true;
>
> spin_lock(&dist->lock);
>
> @@ -1243,6 +1243,11 @@ static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>
> if (irq_num >= VGIC_NR_PRIVATE_IRQS) {
> cpuid = dist->irq_spi_cpu[irq_num - VGIC_NR_PRIVATE_IRQS];
> + if (cpuid == VCPU_NOT_ALLOCATED) {
> + /* Pretend we use CPU0, and prevent injection */
> + cpuid = 0;
> + can_inject = false;
> + }
> vcpu = kvm_get_vcpu(kvm, cpuid);
> }
>
> @@ -1264,7 +1269,7 @@ static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>
> enabled = vgic_irq_is_enabled(vcpu, irq_num);
>
> - if (!enabled) {
> + if (!enabled || !can_inject) {
don't you also need to handle the vgic_dist_irq_set_pending() call and
its friends above?
> ret = false;
> goto out;
> }
> @@ -1406,6 +1411,7 @@ void kvm_vgic_destroy(struct kvm *kvm)
> }
> kfree(dist->irq_sgi_sources);
> kfree(dist->irq_spi_cpu);
> + kfree(dist->irq_spi_mpidr);
> kfree(dist->irq_spi_target);
> kfree(dist->irq_pending_on_cpu);
> dist->irq_sgi_sources = NULL;
> @@ -1581,6 +1587,7 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
> kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
> kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
> kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
> + kvm->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
sure, we can write to the same memory twice, why not, it's fun.
>
> if (!init_emulation_ops(kvm, type))
> ret = -ENODEV;
> diff --git a/virt/kvm/arm/vgic.h b/virt/kvm/arm/vgic.h
> index f52db4e..42c20c1 100644
> --- a/virt/kvm/arm/vgic.h
> +++ b/virt/kvm/arm/vgic.h
> @@ -35,6 +35,8 @@
> #define ACCESS_WRITE_VALUE (3 << 1)
> #define ACCESS_WRITE_MASK(x) ((x) & (3 << 1))
>
> +#define VCPU_NOT_ALLOCATED ((u8)-1)
> +
> unsigned long *vgic_bitmap_get_shared_map(struct vgic_bitmap *x);
>
> void vgic_update_state(struct kvm *kvm);
> @@ -121,5 +123,6 @@ int vgic_set_common_attr(struct kvm_device *dev, struct kvm_device_attr *attr);
> int vgic_get_common_attr(struct kvm_device *dev, struct kvm_device_attr *attr);
>
> bool vgic_v2_init_emulation_ops(struct kvm *kvm, int type);
> +bool vgic_v3_init_emulation_ops(struct kvm *kvm, int type);
>
> #endif
> --
> 1.7.9.5
>
More information about the linux-arm-kernel
mailing list