Implement save/restore of the VGIC state using the newer KVM Device Control API. This requries some number of changes to existing code in addition to actually supporting save/restore of the necessary state.
The first patches (01-03) support creating the VGIC using the Device Control API. This change is necessary because there are no other suitable KVM APIs that we can leverage to access the VGIC state from user space and the device control API was crafted exactly for this purpose.
Subsequent patches add the missing infrastructure and user space API pieces necessary to actually save and restore the VGIC state. The GIC v2.0 architecture specification already specifies registers that can be used to save and restore the complete VGIC state for suspend/resume purposes on real hardware, and we can resuse this interface for the VGIC. The API is therefore based on the memory-mapped register accesses defined in the specs. See the individual patches for details.
The patches rely on a number of small fixes sent separately to actually work: git://git.linaro.org/people/cdall/linux-kvm-arm.git vgic-migrate-prereq
This patch series based on the above can be cloned from: git://git.linaro.org/people/cdall/linux-kvm-arm.git vgic-migrate
User space patches for QEMU will follow shortly. Tested on Versatile Express TC2.
Christoffer Dall (8): ARM: KVM: Allow creating the VGIC after VCPUs KVM: arm-vgic: Support KVM_CREATE_DEVICE for VGIC KVM: arm-vgic: Set base addr through device API irqchip: arm-gic: Define additional MMIO offsets and masks KVM: arm-vgic: Make vgic mmio functions more generic KVM: arm-vgic: Add vgic reg access from dev attr KVM: arm-vgic: Add GICD_SPENDSGIR and GICD_CPENDSGIR handlers KVM: arm-vgic: Support CPU interface reg access
Documentation/virtual/kvm/api.txt | 6 +- Documentation/virtual/kvm/devices/arm-vgic.txt | 56 +++ arch/arm/include/uapi/asm/kvm.h | 8 + arch/arm/kvm/arm.c | 10 +- include/kvm/arm_vgic.h | 2 +- include/linux/irqchip/arm-gic.h | 14 + include/linux/kvm_host.h | 1 + include/uapi/linux/kvm.h | 1 + virt/kvm/arm/vgic.c | 479 ++++++++++++++++++++++-- virt/kvm/kvm_main.c | 5 + 10 files changed, 554 insertions(+), 28 deletions(-) create mode 100644 Documentation/virtual/kvm/devices/arm-vgic.txt
Rework the VGIC initialization slightly to allow initialization of the vgic cpu-specific state even if the irqchip (the VGIC) hasn't been created by user space yet. This is safe, because the vgic data structures are already allocated when the CPU is allocated if VGIC support is compiled into the kernel. Further, the init process does not depend on any other information and the sacrifice is a slight performance degradation for creating VMs in the no-VGIC case.
The reason is that the new device control API doesn't mandate creating the VGIC before creating the VCPU and it is unreasonable to require user space to create the VGIC before creating the VCPUs.
At the same time move the irqchip_in_kernel check out of kvm_vcpu_first_run_init and into the init function to make the per-vcpu and global init functions symmetric and add comments on the exported functions making it a bit easier to understand the init flow by only looking at vgic.c.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org --- arch/arm/kvm/arm.c | 7 ++++--- virt/kvm/arm/vgic.c | 22 +++++++++++++++++++--- 2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index 741f66a..9358050 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -457,6 +457,8 @@ static void update_vttbr(struct kvm *kvm)
static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) { + int ret; + if (likely(vcpu->arch.has_run_once)) return 0;
@@ -466,9 +468,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) * Initialize the VGIC before running a vcpu the first time on * this VM. */ - if (irqchip_in_kernel(vcpu->kvm) && - unlikely(!vgic_initialized(vcpu->kvm))) { - int ret = kvm_vgic_init(vcpu->kvm); + if (unlikely(!vgic_initialized(vcpu->kvm))) { + ret = kvm_vgic_init(vcpu->kvm); if (ret) return ret; } diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 96d7aa4..7355f30 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1245,15 +1245,19 @@ static irqreturn_t vgic_maintenance_handler(int irq, void *data) return IRQ_HANDLED; }
+/** + * kvm_vgic_vcpu_init - Initialize per-vcpu VGIC state + * @vcpu: pointer to the vcpu struct + * + * Initialize the vgic_cpu struct and vgic_dist struct fields pertaining to + * this vcpu and enable the VGIC for this VCPU + */ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu) { struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; struct vgic_dist *dist = &vcpu->kvm->arch.vgic; int i;
- if (!irqchip_in_kernel(vcpu->kvm)) - return 0; - if (vcpu->vcpu_id >= VGIC_MAX_CPUS) return -EBUSY;
@@ -1385,10 +1389,22 @@ out: return ret; }
+/** + * kvm_vgic_init - Initialize global VGIC state before running any VCPUs + * @kvm: pointer to the kvm struct + * + * Map the virtual CPU interface into the VM before running any VCPUs. We + * can't do this at creation time, because user space must first set the + * virtual CPU interface address in the guest physical address space. Also + * initialize the ITARGETSRn regs to 0 on the emulated distributor. + */ int kvm_vgic_init(struct kvm *kvm) { int ret = 0, i;
+ if (!irqchip_in_kernel(kvm)) + return 0; + mutex_lock(&kvm->lock);
if (vgic_initialized(kvm))
Support creating the ARM VGIC device through the KVM_CREATE_DEVICE ioctl, which can then later be leveraged to use the KVM_{GET/SET}_DEVICE_ATTR, which is useful both for setting addresses in a more generic API than the ARM-specific one and is useful for save/restore of VGIC state.
Adds KVM_CAP_DEVICE_CTRL to ARM capabilities.
Note that we change the check for creating a VGIC from bailing out if any VCPUs were created to bailing if any VCPUs were ever run. This is an important distinction that doesn't break anything, but allows creating the VGIC after the VCPUs have been created.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org --- Documentation/virtual/kvm/devices/arm-vgic.txt | 10 ++++++ arch/arm/include/uapi/asm/kvm.h | 1 - arch/arm/kvm/arm.c | 1 + include/linux/kvm_host.h | 1 + include/uapi/linux/kvm.h | 1 + virt/kvm/arm/vgic.c | 46 ++++++++++++++++++++++-- virt/kvm/kvm_main.c | 5 +++ 7 files changed, 62 insertions(+), 3 deletions(-) create mode 100644 Documentation/virtual/kvm/devices/arm-vgic.txt
diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt new file mode 100644 index 0000000..38f27f7 --- /dev/null +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt @@ -0,0 +1,10 @@ +ARM Virtual Generic Interrupt Controller (VGIC) +=============================================== + +Device types supported: + KVM_DEV_TYPE_ARM_VGIC_V2 ARM Generic Interrupt Controller v2.0 + +Only one VGIC instance may be instantiated through either this API or the +legacy KVM_CREATE_IRQCHIP api. The created VGIC will act as the VM interrupt +controller, requiring emulated user-space devices to inject interrupts to the +VGIC instead of directly to CPUs. diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h index c1ee007..1c85102 100644 --- a/arch/arm/include/uapi/asm/kvm.h +++ b/arch/arm/include/uapi/asm/kvm.h @@ -142,7 +142,6 @@ struct kvm_arch_memory_slot { #define KVM_REG_ARM_VFP_FPINST 0x1009 #define KVM_REG_ARM_VFP_FPINST2 0x100A
- /* KVM_IRQ_LINE irq field index values */ #define KVM_ARM_IRQ_TYPE_SHIFT 24 #define KVM_ARM_IRQ_TYPE_MASK 0xff diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index 9358050..c7b7474 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -187,6 +187,7 @@ int kvm_dev_ioctl_check_extension(long ext) case KVM_CAP_IRQCHIP: r = vgic_present; break; + case KVM_CAP_DEVICE_CTRL: case KVM_CAP_USER_MEMORY: case KVM_CAP_SYNC_MMU: case KVM_CAP_DESTROY_MEMORY_REGION_WORKS: diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index a63d83e..618fc4e 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1052,6 +1052,7 @@ struct kvm_device *kvm_device_from_filp(struct file *filp);
extern struct kvm_device_ops kvm_mpic_ops; extern struct kvm_device_ops kvm_xics_ops; +extern struct kvm_device_ops kvm_arm_vgic_ops;
#ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index acccd08..ecda3aa 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -842,6 +842,7 @@ struct kvm_device_attr { #define KVM_DEV_TYPE_FSL_MPIC_20 1 #define KVM_DEV_TYPE_FSL_MPIC_42 2 #define KVM_DEV_TYPE_XICS 3 +#define KVM_DEV_TYPE_ARM_VGIC_V2 4
/* * ioctls for VM fds diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 7355f30..140608a 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1436,15 +1436,23 @@ out:
int kvm_vgic_create(struct kvm *kvm) { - int ret = 0; + int i, ret = 0; + struct kvm_vcpu *vcpu;
mutex_lock(&kvm->lock);
- if (atomic_read(&kvm->online_vcpus) || kvm->arch.vgic.vctrl_base) { + if (kvm->arch.vgic.vctrl_base) { ret = -EEXIST; goto out; }
+ kvm_for_each_vcpu(i, vcpu, kvm) { + if (vcpu->arch.has_run_once) { + ret = -EBUSY; + goto out; + } + } + spin_lock_init(&kvm->arch.vgic.lock); kvm->arch.vgic.vctrl_base = vgic_vctrl_base; kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF; @@ -1513,3 +1521,37 @@ int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr) mutex_unlock(&kvm->lock); return r; } + +static int vgic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr) +{ + return -ENXIO; +} + +static int vgic_get_attr(struct kvm_device *dev, struct kvm_device_attr *attr) +{ + return -ENXIO; +} + +static int vgic_has_attr(struct kvm_device *dev, struct kvm_device_attr *attr) +{ + return -ENXIO; +} + +static void vgic_destroy(struct kvm_device *dev) +{ + kfree(dev); +} + +static int vgic_create(struct kvm_device *dev, u32 type) +{ + return kvm_vgic_create(dev->kvm); +} + +struct kvm_device_ops kvm_arm_vgic_ops = { + .name = "kvm-arm-vgic", + .create = vgic_create, + .destroy = vgic_destroy, + .set_attr = vgic_set_attr, + .get_attr = vgic_get_attr, + .has_attr = vgic_has_attr, +}; diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 1580dd4..ec88c79 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2282,6 +2282,11 @@ static int kvm_ioctl_create_device(struct kvm *kvm, ops = &kvm_xics_ops; break; #endif +#ifdef CONFIG_KVM_ARM_VGIC + case KVM_DEV_TYPE_ARM_VGIC_V2: + ops = &kvm_arm_vgic_ops; + break; +#endif default: return -ENODEV; }
Support setting the distributor and cpu interface base addresses in the VM physical address space through the KVM_{SET,GET}_DEVICE_ATTR API in addition to the ARM specific API.
This has the added benefit of being able to share more code in user space and do things in a uniform maner.
Also deprecate the older API at the same time, but backwards compatibility will be maintained.
Reviewed-by: Alexander Graf agraf@suse.de Signed-off-by: Christoffer Dall christoffer.dall@linaro.org --- Documentation/virtual/kvm/api.txt | 6 +- Documentation/virtual/kvm/devices/arm-vgic.txt | 11 +++ arch/arm/include/uapi/asm/kvm.h | 9 +++ arch/arm/kvm/arm.c | 2 +- include/kvm/arm_vgic.h | 2 +- virt/kvm/arm/vgic.c | 90 ++++++++++++++++++++---- 6 files changed, 105 insertions(+), 15 deletions(-)
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index ef925ea..594a3be 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -2324,7 +2324,7 @@ This ioctl returns the guest registers that are supported for the KVM_GET_ONE_REG/KVM_SET_ONE_REG calls.
-4.84 KVM_ARM_SET_DEVICE_ADDR +4.84 KVM_ARM_SET_DEVICE_ADDR (deprecated)
Capability: KVM_CAP_ARM_SET_DEVICE_ADDR Architectures: arm, arm64 @@ -2362,6 +2362,10 @@ must be called after calling KVM_CREATE_IRQCHIP, but before calling KVM_RUN on any of the VCPUs. Calling this ioctl twice for any of the base addresses will return -EEXIST.
+Note, this IOCTL is deprecated and the more flexible SET/GET_DEVICE_ATTR API +should be used instead. + + 4.85 KVM_PPC_RTAS_DEFINE_TOKEN
Capability: KVM_CAP_PPC_RTAS diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt index 38f27f7..c9febb2 100644 --- a/Documentation/virtual/kvm/devices/arm-vgic.txt +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt @@ -8,3 +8,14 @@ Only one VGIC instance may be instantiated through either this API or the legacy KVM_CREATE_IRQCHIP api. The created VGIC will act as the VM interrupt controller, requiring emulated user-space devices to inject interrupts to the VGIC instead of directly to CPUs. + +Groups: + KVM_DEV_ARM_VGIC_GRP_ADDR + Attributes: + KVM_VGIC_V2_ADDR_TYPE_DIST (rw, 64-bit) + Base address in the guest physical address space of the GIC distributor + register mappings. + + KVM_VGIC_V2_ADDR_TYPE_CPU (rw, 64-bit) + Base address in the guest physical address space of the GIC virtual cpu + interface register mappings. diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h index 1c85102..587f1ae 100644 --- a/arch/arm/include/uapi/asm/kvm.h +++ b/arch/arm/include/uapi/asm/kvm.h @@ -142,6 +142,15 @@ struct kvm_arch_memory_slot { #define KVM_REG_ARM_VFP_FPINST 0x1009 #define KVM_REG_ARM_VFP_FPINST2 0x100A
+/* Device Control API: ARM VGIC */ +#define KVM_DEV_ARM_VGIC_GRP_ADDR 0 +#define KVM_DEV_ARM_VGIC_GRP_DIST_REGS 1 +#define KVM_DEV_ARM_VGIC_GRP_CPU_REGS 2 +#define KVM_DEV_ARM_VGIC_CPUID_SHIFT 32 +#define KVM_DEV_ARM_VGIC_CPUID_MASK (0xffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT) +#define KVM_DEV_ARM_VGIC_OFFSET_SHIFT 0 +#define KVM_DEV_ARM_VGIC_OFFSET_MASK (0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT) + /* KVM_IRQ_LINE irq field index values */ #define KVM_ARM_IRQ_TYPE_SHIFT 24 #define KVM_ARM_IRQ_TYPE_MASK 0xff diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index c7b7474..0208ed1 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -769,7 +769,7 @@ static int kvm_vm_ioctl_set_device_addr(struct kvm *kvm, case KVM_ARM_DEVICE_VGIC_V2: if (!vgic_present) return -ENXIO; - return kvm_vgic_set_addr(kvm, type, dev_addr->addr); + return kvm_vgic_addr(kvm, type, &dev_addr->addr, true); default: return -ENODEV; } diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index 7e2d158..be85127 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -144,7 +144,7 @@ struct kvm_run; struct kvm_exit_mmio;
#ifdef CONFIG_KVM_ARM_VGIC -int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr); +int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write); int kvm_vgic_hyp_init(void); int kvm_vgic_init(struct kvm *kvm); int kvm_vgic_create(struct kvm *kvm); diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 140608a..a51efb8 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1481,6 +1481,12 @@ static int vgic_ioaddr_assign(struct kvm *kvm, phys_addr_t *ioaddr, { int ret;
+ if (addr & ~KVM_PHYS_MASK) + return -E2BIG; + + if (addr & (SZ_4K - 1)) + return -EINVAL; + if (!IS_VGIC_ADDR_UNDEF(*ioaddr)) return -EEXIST; if (addr + size < addr) @@ -1493,26 +1499,41 @@ static int vgic_ioaddr_assign(struct kvm *kvm, phys_addr_t *ioaddr, return ret; }
-int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr) +/** + * kvm_vgic_addr - set or get vgic VM base addresses + * @kvm: pointer to the vm struct + * @type: the VGIC addr type, one of KVM_VGIC_V2_ADDR_TYPE_XXX + * @addr: pointer to address value + * @write: if true set the address in the VM address space, if false read the + * address + * + * Set or get the vgic base addresses for the distributor and the virtual CPU + * interface in the VM physical address space. These addresses are properties + * of the emulated core/SoC and therefore user space initially knows this + * information. + */ +int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) { int r = 0; struct vgic_dist *vgic = &kvm->arch.vgic;
- if (addr & ~KVM_PHYS_MASK) - return -E2BIG; - - if (addr & (SZ_4K - 1)) - return -EINVAL; - mutex_lock(&kvm->lock); switch (type) { case KVM_VGIC_V2_ADDR_TYPE_DIST: - r = vgic_ioaddr_assign(kvm, &vgic->vgic_dist_base, - addr, KVM_VGIC_V2_DIST_SIZE); + if (write) { + r = vgic_ioaddr_assign(kvm, &vgic->vgic_dist_base, + *addr, KVM_VGIC_V2_DIST_SIZE); + } else { + *addr = vgic->vgic_dist_base; + } break; case KVM_VGIC_V2_ADDR_TYPE_CPU: - r = vgic_ioaddr_assign(kvm, &vgic->vgic_cpu_base, - addr, KVM_VGIC_V2_CPU_SIZE); + if (write) { + r = vgic_ioaddr_assign(kvm, &vgic->vgic_cpu_base, + *addr, KVM_VGIC_V2_CPU_SIZE); + } else { + *addr = vgic->vgic_cpu_base; + } break; default: r = -ENODEV; @@ -1524,16 +1545,61 @@ int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr)
static int vgic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr) { + int r; + + switch (attr->group) { + case KVM_DEV_ARM_VGIC_GRP_ADDR: { + u64 __user *uaddr = (u64 __user *)(long)attr->addr; + u64 addr; + unsigned long type = (unsigned long)attr->attr; + + if (copy_from_user(&addr, uaddr, sizeof(addr))) + return -EFAULT; + + r = kvm_vgic_addr(dev->kvm, type, &addr, true); + return (r == -ENODEV) ? -ENXIO : r; + } + } + return -ENXIO; }
static int vgic_get_attr(struct kvm_device *dev, struct kvm_device_attr *attr) { - return -ENXIO; + int r = ENXIO; + + switch (attr->group) { + case KVM_DEV_ARM_VGIC_GRP_ADDR: { + u64 __user *uaddr = (u64 __user *)(long)attr->addr; + u64 addr; + unsigned long type = (unsigned long)attr->attr; + + r = kvm_vgic_addr(dev->kvm, type, &addr, false); + if (r) + return (r == -ENODEV) ? -ENXIO : r; + + r = 0; + if (copy_to_user(uaddr, &addr, sizeof(addr))) + return -EFAULT; + } + } + + return r; }
static int vgic_has_attr(struct kvm_device *dev, struct kvm_device_attr *attr) { + phys_addr_t offset; + + 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 0; + } + break; + } return -ENXIO; }
Define CPU interface offsets for the GICC_ABPR, GICC_APR, and GICC_IIDR registers. Define distributor registers for the GICD_SPENDSGIR and the GICD_CPENDSGIR. KVM/ARM needs to know about these definitions to fully support save/restore of the VGIC.
Also define some masks and shifts for the various GICH_VMCR fields.
Cc: Thomas Gleixner tglx@linutronix.de Signed-off-by: Christoffer Dall christoffer.dall@linaro.org --- include/linux/irqchip/arm-gic.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h index 3e203eb..e95c00e 100644 --- a/include/linux/irqchip/arm-gic.h +++ b/include/linux/irqchip/arm-gic.h @@ -17,6 +17,9 @@ #define GIC_CPU_EOI 0x10 #define GIC_CPU_RUNNINGPRI 0x14 #define GIC_CPU_HIGHPRI 0x18 +#define GIC_CPU_ALIAS_BINPOINT 0x1c +#define GIC_CPU_ACTIVEPRIO 0xd0 +#define GIC_CPU_IDENT 0xfc
#define GIC_DIST_CTRL 0x000 #define GIC_DIST_CTR 0x004 @@ -31,6 +34,8 @@ #define GIC_DIST_TARGET 0x800 #define GIC_DIST_CONFIG 0xc00 #define GIC_DIST_SOFTINT 0xf00 +#define GIC_DIST_SGI_CLEAR 0xf10 +#define GIC_DIST_SGI_SET 0xf20
#define GICH_HCR 0x0 #define GICH_VTR 0x4 @@ -54,6 +59,15 @@ #define GICH_LR_ACTIVE_BIT (1 << 29) #define GICH_LR_EOI (1 << 19)
+#define GICH_VMCR_CTRL_SHIFT 0 +#define GICH_VMCR_CTRL_MASK (0x21f << GICH_VMCR_CTRL_SHIFT) +#define GICH_VMCR_PRIMASK_SHIFT 27 +#define GICH_VMCR_PRIMASK_MASK (0x1f << GICH_VMCR_PRIMASK_SHIFT) +#define GICH_VMCR_BINPOINT_SHIFT 21 +#define GICH_VMCR_BINPOINT_MASK (0x7 << GICH_VMCR_BINPOINT_SHIFT) +#define GICH_VMCR_ALIAS_BINPOINT_SHIFT 18 +#define GICH_VMCR_ALIAS_BINPOINT_MASK (0x7 << GICH_VMCR_ALIAS_BINPOINT_SHIFT) + #define GICH_MISR_EOI (1 << 0) #define GICH_MISR_U (1 << 1)
Rename the vgic_ranges array to vgic_dist_ranges to be more specific and to prepare for handling CPU interface register access as well (for save/restore of VGIC state).
Pass offset from distributor or interface MMIO base to find_matching_range function instead of the physical address of the access in the VM memory map. This allows other callers unaware of the VM specifics, but with generic VGIC knowledge to reuse the function.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org --- virt/kvm/arm/vgic.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index a51efb8..629caeb 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -604,7 +604,7 @@ struct mmio_range { phys_addr_t offset); };
-static const struct mmio_range vgic_ranges[] = { +static const struct mmio_range vgic_dist_ranges[] = { { .base = GIC_DIST_CTRL, .len = 12, @@ -671,14 +671,13 @@ static const struct mmio_range vgic_ranges[] = { static const struct mmio_range *find_matching_range(const struct mmio_range *ranges, struct kvm_exit_mmio *mmio, - phys_addr_t base) + phys_addr_t offset) { const struct mmio_range *r = ranges; - phys_addr_t addr = mmio->phys_addr - base;
while (r->len) { - if (addr >= r->base && - (addr + mmio->len) <= (r->base + r->len)) + if (offset >= r->base && + (offset + mmio->len) <= (r->base + r->len)) return r; r++; } @@ -715,7 +714,8 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run, return true; }
- range = find_matching_range(vgic_ranges, mmio, base); + offset = mmio->phys_addr - base; + range = find_matching_range(vgic_dist_ranges, mmio, offset); if (unlikely(!range || !range->handle_mmio)) { pr_warn("Unhandled access %d %08llx %d\n", mmio->is_write, mmio->phys_addr, mmio->len);
Add infrastructure to handle distributor and cpu interface register accesses through the KVM_{GET/SET}_DEVICE_ATTR interface by adding the KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_CPU_REGS groups and defining the semantics of the attr field to be the MMIO offset as specified in the GICv2 specs.
Missing register accesses or other changes in individual register access functions to support save/restore of the VGIC state is added in subsequent patches.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org --- Documentation/virtual/kvm/devices/arm-vgic.txt | 35 ++++++ virt/kvm/arm/vgic.c | 143 ++++++++++++++++++++++++ 2 files changed, 178 insertions(+)
diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt index c9febb2..1b68475 100644 --- a/Documentation/virtual/kvm/devices/arm-vgic.txt +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt @@ -19,3 +19,38 @@ Groups: KVM_VGIC_V2_ADDR_TYPE_CPU (rw, 64-bit) Base address in the guest physical address space of the GIC virtual cpu interface register mappings. + + KVM_DEV_ARM_VGIC_GRP_DIST_REGS + Attributes: + The attr field of kvm_device_attr encodes two values: + bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 | + values: | reserved | cpu id | offset | + + All distributor regs are (rw, 32-bit) + + The offset is relative to the "Distributor base address" as defined in the + GICv2 specs. Getting or setting such a register has the same effect as + reading or writing the register on the actual hardware from the cpu + specified with cpu id field. Note that most distributor fields are not + banked, but return the same value regardless of the cpu id used to access + the register. + Limitations: + - Priorities are not implemented, and registers are RAZ/WI + Errors: + - ENODEV: Getting or setting this register is not yet supported + + KVM_DEV_ARM_VGIC_GRP_CPU_REGS + Attributes: + The attr field of kvm_device_attr encodes two values: + bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 | + values: | reserved | cpu id | offset | + + All CPU regs are (rw, 32-bit) + + The offsetspecifies the offset from the "CPU interface base address" as + defined in the GICv2 specs. Getting or setting such a register has the + same effect as reading or writing the register on the actual hardware. + Limitations: + - Priorities are not implemented, and registers are RAZ/WI + Errors: + - ENODEV: Getting or setting this register is not yet supported diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 629caeb..e31625c 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -591,11 +591,29 @@ static bool handle_mmio_sgi_reg(struct kvm_vcpu *vcpu, return false; }
+static bool handle_mmio_sgi_clear(struct kvm_vcpu *vcpu, + struct kvm_exit_mmio *mmio, + phys_addr_t offset) +{ + return false; +} + +static bool handle_mmio_sgi_set(struct kvm_vcpu *vcpu, + struct kvm_exit_mmio *mmio, + phys_addr_t offset) +{ + return false; +} + /* * I would have liked to use the kvm_bus_io_*() API instead, but it * cannot cope with banked registers (only the VM pointer is passed * around, and we need the vcpu). One of these days, someone please * fix it! + * + * Note that the handle_mmio implementations should not use the phys_addr + * field from the kvm_exit_mmio struct as this will not have any sane values + * when used to save/restore state from user space. */ struct mmio_range { phys_addr_t base; @@ -665,6 +683,16 @@ static const struct mmio_range vgic_dist_ranges[] = { .len = 4, .handle_mmio = handle_mmio_sgi_reg, }, + { + .base = GIC_DIST_SGI_CLEAR, + .len = VGIC_NR_SGIS, + .handle_mmio = handle_mmio_sgi_clear, + }, + { + .base = GIC_DIST_SGI_SET, + .len = VGIC_NR_SGIS, + .handle_mmio = handle_mmio_sgi_set, + }, {} };
@@ -1543,6 +1571,80 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) return r; }
+static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu, + struct kvm_exit_mmio *mmio, phys_addr_t offset) +{ + return true; +} + +static const struct mmio_range vgic_cpu_ranges[] = { + { + .base = GIC_CPU_CTRL, + .len = 12, + .handle_mmio = handle_cpu_mmio_misc, + }, + { + .base = GIC_CPU_ALIAS_BINPOINT, + .len = 4, + .handle_mmio = handle_cpu_mmio_misc, + }, + { + .base = GIC_CPU_ACTIVEPRIO, + .len = 16, + .handle_mmio = handle_cpu_mmio_misc, + }, + { + .base = GIC_CPU_IDENT, + .len = 4, + .handle_mmio = handle_cpu_mmio_misc, + }, +}; + +static struct kvm_exit_mmio dev_attr_mmio = { .len = 4 }; + +static int vgic_attr_regs_access(struct kvm_device *dev, + struct kvm_device_attr *attr, + u32 *reg, bool is_write) +{ + const struct mmio_range *r = NULL; + phys_addr_t offset; + int cpuid; + struct kvm_vcpu *vcpu; + struct kvm_exit_mmio mmio; + + offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK; + cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >> + KVM_DEV_ARM_VGIC_CPUID_SHIFT; + + if (cpuid >= atomic_read(&dev->kvm->online_vcpus)) + return -EINVAL; + + vcpu = kvm_get_vcpu(dev->kvm, cpuid); + + mmio.len = 4; + mmio.is_write = is_write; + if (is_write) + memcpy(mmio.data, reg, sizeof(*reg)); + + if (attr->group == KVM_DEV_ARM_VGIC_GRP_DIST_REGS) + r = find_matching_range(vgic_dist_ranges, &mmio, offset); + else if (attr->group == KVM_DEV_ARM_VGIC_GRP_CPU_REGS) + r = find_matching_range(vgic_cpu_ranges, &mmio, offset); + + if (unlikely(!r || !r->handle_mmio)) + return -ENXIO; + + spin_lock(&vcpu->kvm->arch.vgic.lock); + offset -= r->base; + r->handle_mmio(vcpu, &mmio, offset); + spin_unlock(&vcpu->kvm->arch.vgic.lock); + + if (!is_write) + memcpy(reg, mmio.data, sizeof(*reg)); + + return 0; +} + static int vgic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr) { int r; @@ -1559,6 +1661,18 @@ static int vgic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr) r = kvm_vgic_addr(dev->kvm, type, &addr, true); return (r == -ENODEV) ? -ENXIO : r; } + + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: + case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: { + u32 __user *uaddr = (u32 __user *)(long)attr->addr; + u32 reg; + + if (get_user(reg, uaddr)) + return -EFAULT; + + return vgic_attr_regs_access(dev, attr, ®, true); + } + }
return -ENXIO; @@ -1581,12 +1695,35 @@ static int vgic_get_attr(struct kvm_device *dev, struct kvm_device_attr *attr) r = 0; if (copy_to_user(uaddr, &addr, sizeof(addr))) return -EFAULT; + break; } + + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: + case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: { + u32 __user *uaddr = (u32 __user *)(long)attr->addr; + u32 reg = 0; + + r = vgic_attr_regs_access(dev, attr, ®, false); + if (r) + return r; + r = put_user(reg, uaddr); + break; + } + }
return r; }
+static int vgic_has_attr_regs(const struct mmio_range *ranges, + phys_addr_t offset) +{ + if (find_matching_range(ranges, &dev_attr_mmio, offset)) + return 0; + else + return -ENXIO; +} + static int vgic_has_attr(struct kvm_device *dev, struct kvm_device_attr *attr) { phys_addr_t offset; @@ -1599,6 +1736,12 @@ static int vgic_has_attr(struct kvm_device *dev, struct kvm_device_attr *attr) return 0; } break; + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS: + offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK; + return vgic_has_attr_regs(vgic_dist_ranges, offset); + case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: + offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK; + return vgic_has_attr_regs(vgic_cpu_ranges, offset); } return -ENXIO; }
On 23.08.2013, at 20:20, Christoffer Dall wrote:
Add infrastructure to handle distributor and cpu interface register accesses through the KVM_{GET/SET}_DEVICE_ATTR interface by adding the KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_CPU_REGS groups and defining the semantics of the attr field to be the MMIO offset as specified in the GICv2 specs.
Missing register accesses or other changes in individual register access functions to support save/restore of the VGIC state is added in subsequent patches.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org
Documentation/virtual/kvm/devices/arm-vgic.txt | 35 ++++++ virt/kvm/arm/vgic.c | 143 ++++++++++++++++++++++++ 2 files changed, 178 insertions(+)
diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt index c9febb2..1b68475 100644 --- a/Documentation/virtual/kvm/devices/arm-vgic.txt +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt @@ -19,3 +19,38 @@ Groups: KVM_VGIC_V2_ADDR_TYPE_CPU (rw, 64-bit) Base address in the guest physical address space of the GIC virtual cpu interface register mappings.
- KVM_DEV_ARM_VGIC_GRP_DIST_REGS
- Attributes:
- The attr field of kvm_device_attr encodes two values:
- bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 |
- values: | reserved | cpu id | offset |
- All distributor regs are (rw, 32-bit)
- The offset is relative to the "Distributor base address" as defined in the
- GICv2 specs. Getting or setting such a register has the same effect as
- reading or writing the register on the actual hardware from the cpu
- specified with cpu id field. Note that most distributor fields are not
- banked, but return the same value regardless of the cpu id used to access
- the register.
- Limitations:
- Priorities are not implemented, and registers are RAZ/WI
- Errors:
- ENODEV: Getting or setting this register is not yet supported
- KVM_DEV_ARM_VGIC_GRP_CPU_REGS
- Attributes:
- The attr field of kvm_device_attr encodes two values:
- bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 |
- values: | reserved | cpu id | offset |
- All CPU regs are (rw, 32-bit)
- The offsetspecifies the offset from the "CPU interface base address" as
offset specifies
- defined in the GICv2 specs. Getting or setting such a register has the
- same effect as reading or writing the register on the actual hardware.
- Limitations:
- Priorities are not implemented, and registers are RAZ/WI
- Errors:
- ENODEV: Getting or setting this register is not yet supported
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 629caeb..e31625c 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -591,11 +591,29 @@ static bool handle_mmio_sgi_reg(struct kvm_vcpu *vcpu, return false; }
+static bool handle_mmio_sgi_clear(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- return false;
+}
+static bool handle_mmio_sgi_set(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- return false;
+}
/*
- I would have liked to use the kvm_bus_io_*() API instead, but it
- cannot cope with banked registers (only the VM pointer is passed
- around, and we need the vcpu). One of these days, someone please
- fix it!
- Note that the handle_mmio implementations should not use the phys_addr
- field from the kvm_exit_mmio struct as this will not have any sane values
- when used to save/restore state from user space.
*/ struct mmio_range { phys_addr_t base; @@ -665,6 +683,16 @@ static const struct mmio_range vgic_dist_ranges[] = { .len = 4, .handle_mmio = handle_mmio_sgi_reg, },
- {
.base = GIC_DIST_SGI_CLEAR,.len = VGIC_NR_SGIS,.handle_mmio = handle_mmio_sgi_clear,- },
- {
.base = GIC_DIST_SGI_SET,.len = VGIC_NR_SGIS,.handle_mmio = handle_mmio_sgi_set,- }, {}
};
@@ -1543,6 +1571,80 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) return r; }
+static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio, phys_addr_t offset)+{
- return true;
+}
+static const struct mmio_range vgic_cpu_ranges[] = {
- {
.base = GIC_CPU_CTRL,.len = 12,.handle_mmio = handle_cpu_mmio_misc,- },
- {
.base = GIC_CPU_ALIAS_BINPOINT,.len = 4,.handle_mmio = handle_cpu_mmio_misc,- },
- {
.base = GIC_CPU_ACTIVEPRIO,.len = 16,.handle_mmio = handle_cpu_mmio_misc,- },
- {
.base = GIC_CPU_IDENT,.len = 4,.handle_mmio = handle_cpu_mmio_misc,- },
+};
+static struct kvm_exit_mmio dev_attr_mmio = { .len = 4 };
+static int vgic_attr_regs_access(struct kvm_device *dev,
struct kvm_device_attr *attr,u32 *reg, bool is_write)+{
- const struct mmio_range *r = NULL;
- phys_addr_t offset;
- int cpuid;
- struct kvm_vcpu *vcpu;
- struct kvm_exit_mmio mmio;
- offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
- cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
KVM_DEV_ARM_VGIC_CPUID_SHIFT;- if (cpuid >= atomic_read(&dev->kvm->online_vcpus))
return -EINVAL;- vcpu = kvm_get_vcpu(dev->kvm, cpuid);
- mmio.len = 4;
- mmio.is_write = is_write;
- if (is_write)
memcpy(mmio.data, reg, sizeof(*reg));
Is this endianness safe?
Alex
On Sun, Aug 25, 2013 at 04:21:58PM +0100, Alexander Graf wrote:
On 23.08.2013, at 20:20, Christoffer Dall wrote:
Add infrastructure to handle distributor and cpu interface register accesses through the KVM_{GET/SET}_DEVICE_ATTR interface by adding the KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_CPU_REGS groups and defining the semantics of the attr field to be the MMIO offset as specified in the GICv2 specs.
Missing register accesses or other changes in individual register access functions to support save/restore of the VGIC state is added in subsequent patches.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org
Documentation/virtual/kvm/devices/arm-vgic.txt | 35 ++++++ virt/kvm/arm/vgic.c | 143 ++++++++++++++++++++++++ 2 files changed, 178 insertions(+)
diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt index c9febb2..1b68475 100644 --- a/Documentation/virtual/kvm/devices/arm-vgic.txt +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt @@ -19,3 +19,38 @@ Groups: KVM_VGIC_V2_ADDR_TYPE_CPU (rw, 64-bit) Base address in the guest physical address space of the GIC virtual cpu interface register mappings.
- KVM_DEV_ARM_VGIC_GRP_DIST_REGS
- Attributes:
- The attr field of kvm_device_attr encodes two values:
- bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 |
- values: | reserved | cpu id | offset |
- All distributor regs are (rw, 32-bit)
- The offset is relative to the "Distributor base address" as defined in the
- GICv2 specs. Getting or setting such a register has the same effect as
- reading or writing the register on the actual hardware from the cpu
- specified with cpu id field. Note that most distributor fields are not
- banked, but return the same value regardless of the cpu id used to access
- the register.
- Limitations:
- Priorities are not implemented, and registers are RAZ/WI
- Errors:
- ENODEV: Getting or setting this register is not yet supported
- KVM_DEV_ARM_VGIC_GRP_CPU_REGS
- Attributes:
- The attr field of kvm_device_attr encodes two values:
- bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 |
- values: | reserved | cpu id | offset |
- All CPU regs are (rw, 32-bit)
- The offsetspecifies the offset from the "CPU interface base address" as
offset specifies
- defined in the GICv2 specs. Getting or setting such a register has the
- same effect as reading or writing the register on the actual hardware.
- Limitations:
- Priorities are not implemented, and registers are RAZ/WI
- Errors:
- ENODEV: Getting or setting this register is not yet supported
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 629caeb..e31625c 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -591,11 +591,29 @@ static bool handle_mmio_sgi_reg(struct kvm_vcpu *vcpu, return false; }
+static bool handle_mmio_sgi_clear(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- return false;
+}
+static bool handle_mmio_sgi_set(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- return false;
+}
/*
- I would have liked to use the kvm_bus_io_*() API instead, but it
- cannot cope with banked registers (only the VM pointer is passed
- around, and we need the vcpu). One of these days, someone please
- fix it!
- Note that the handle_mmio implementations should not use the phys_addr
- field from the kvm_exit_mmio struct as this will not have any sane values
- when used to save/restore state from user space.
*/ struct mmio_range { phys_addr_t base; @@ -665,6 +683,16 @@ static const struct mmio_range vgic_dist_ranges[] = { .len = 4, .handle_mmio = handle_mmio_sgi_reg, },
- {
.base = GIC_DIST_SGI_CLEAR,.len = VGIC_NR_SGIS,.handle_mmio = handle_mmio_sgi_clear,- },
- {
.base = GIC_DIST_SGI_SET,.len = VGIC_NR_SGIS,.handle_mmio = handle_mmio_sgi_set,- }, {}
};
@@ -1543,6 +1571,80 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) return r; }
+static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio, phys_addr_t offset)+{
- return true;
+}
+static const struct mmio_range vgic_cpu_ranges[] = {
- {
.base = GIC_CPU_CTRL,.len = 12,.handle_mmio = handle_cpu_mmio_misc,- },
- {
.base = GIC_CPU_ALIAS_BINPOINT,.len = 4,.handle_mmio = handle_cpu_mmio_misc,- },
- {
.base = GIC_CPU_ACTIVEPRIO,.len = 16,.handle_mmio = handle_cpu_mmio_misc,- },
- {
.base = GIC_CPU_IDENT,.len = 4,.handle_mmio = handle_cpu_mmio_misc,- },
+};
+static struct kvm_exit_mmio dev_attr_mmio = { .len = 4 };
+static int vgic_attr_regs_access(struct kvm_device *dev,
struct kvm_device_attr *attr,u32 *reg, bool is_write)+{
- const struct mmio_range *r = NULL;
- phys_addr_t offset;
- int cpuid;
- struct kvm_vcpu *vcpu;
- struct kvm_exit_mmio mmio;
- offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
- cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
KVM_DEV_ARM_VGIC_CPUID_SHIFT;- if (cpuid >= atomic_read(&dev->kvm->online_vcpus))
return -EINVAL;- vcpu = kvm_get_vcpu(dev->kvm, cpuid);
- mmio.len = 4;
- mmio.is_write = is_write;
- if (is_write)
memcpy(mmio.data, reg, sizeof(*reg));Is this endianness safe?
With a big-endian kernel, no. But I suspect that breaks KVM elsewhere too. However, this is actually nicer:
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index ea7fb5c..191ff9f 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1622,7 +1622,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, mmio.len = 4; mmio.is_write = is_write; if (is_write) - memcpy(mmio.data, reg, sizeof(*reg)); + mmio_data_write(mmio, ~0, *reg);
if (attr->group == KVM_DEV_ARM_VGIC_GRP_DIST_REGS) r = find_matching_range(vgic_dist_ranges, &mmio, offset); @@ -1638,7 +1638,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, spin_unlock(&vcpu->kvm->arch.vgic.lock);
if (!is_write) - memcpy(reg, mmio.data, sizeof(*reg)); + *reg = mmio_data_read(mmio, ~0);
return 0; }
Thanks, -Christoffer
On Wed, Sep 25, 2013 at 01:38:03PM -0700, Christoffer Dall wrote:
On Sun, Aug 25, 2013 at 04:21:58PM +0100, Alexander Graf wrote:
On 23.08.2013, at 20:20, Christoffer Dall wrote:
Add infrastructure to handle distributor and cpu interface register accesses through the KVM_{GET/SET}_DEVICE_ATTR interface by adding the KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_CPU_REGS groups and defining the semantics of the attr field to be the MMIO offset as specified in the GICv2 specs.
Missing register accesses or other changes in individual register access functions to support save/restore of the VGIC state is added in subsequent patches.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org
Documentation/virtual/kvm/devices/arm-vgic.txt | 35 ++++++ virt/kvm/arm/vgic.c | 143 ++++++++++++++++++++++++ 2 files changed, 178 insertions(+)
diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt index c9febb2..1b68475 100644 --- a/Documentation/virtual/kvm/devices/arm-vgic.txt +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt @@ -19,3 +19,38 @@ Groups: KVM_VGIC_V2_ADDR_TYPE_CPU (rw, 64-bit) Base address in the guest physical address space of the GIC virtual cpu interface register mappings.
- KVM_DEV_ARM_VGIC_GRP_DIST_REGS
- Attributes:
- The attr field of kvm_device_attr encodes two values:
- bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 |
- values: | reserved | cpu id | offset |
- All distributor regs are (rw, 32-bit)
- The offset is relative to the "Distributor base address" as defined in the
- GICv2 specs. Getting or setting such a register has the same effect as
- reading or writing the register on the actual hardware from the cpu
- specified with cpu id field. Note that most distributor fields are not
- banked, but return the same value regardless of the cpu id used to access
- the register.
- Limitations:
- Priorities are not implemented, and registers are RAZ/WI
- Errors:
- ENODEV: Getting or setting this register is not yet supported
- KVM_DEV_ARM_VGIC_GRP_CPU_REGS
- Attributes:
- The attr field of kvm_device_attr encodes two values:
- bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 |
- values: | reserved | cpu id | offset |
- All CPU regs are (rw, 32-bit)
- The offsetspecifies the offset from the "CPU interface base address" as
offset specifies
- defined in the GICv2 specs. Getting or setting such a register has the
- same effect as reading or writing the register on the actual hardware.
- Limitations:
- Priorities are not implemented, and registers are RAZ/WI
- Errors:
- ENODEV: Getting or setting this register is not yet supported
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 629caeb..e31625c 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -591,11 +591,29 @@ static bool handle_mmio_sgi_reg(struct kvm_vcpu *vcpu, return false; }
+static bool handle_mmio_sgi_clear(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- return false;
+}
+static bool handle_mmio_sgi_set(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- return false;
+}
/*
- I would have liked to use the kvm_bus_io_*() API instead, but it
- cannot cope with banked registers (only the VM pointer is passed
- around, and we need the vcpu). One of these days, someone please
- fix it!
- Note that the handle_mmio implementations should not use the phys_addr
- field from the kvm_exit_mmio struct as this will not have any sane values
- when used to save/restore state from user space.
*/ struct mmio_range { phys_addr_t base; @@ -665,6 +683,16 @@ static const struct mmio_range vgic_dist_ranges[] = { .len = 4, .handle_mmio = handle_mmio_sgi_reg, },
- {
.base = GIC_DIST_SGI_CLEAR,.len = VGIC_NR_SGIS,.handle_mmio = handle_mmio_sgi_clear,- },
- {
.base = GIC_DIST_SGI_SET,.len = VGIC_NR_SGIS,.handle_mmio = handle_mmio_sgi_set,- }, {}
};
@@ -1543,6 +1571,80 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) return r; }
+static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio, phys_addr_t offset)+{
- return true;
+}
+static const struct mmio_range vgic_cpu_ranges[] = {
- {
.base = GIC_CPU_CTRL,.len = 12,.handle_mmio = handle_cpu_mmio_misc,- },
- {
.base = GIC_CPU_ALIAS_BINPOINT,.len = 4,.handle_mmio = handle_cpu_mmio_misc,- },
- {
.base = GIC_CPU_ACTIVEPRIO,.len = 16,.handle_mmio = handle_cpu_mmio_misc,- },
- {
.base = GIC_CPU_IDENT,.len = 4,.handle_mmio = handle_cpu_mmio_misc,- },
+};
+static struct kvm_exit_mmio dev_attr_mmio = { .len = 4 };
+static int vgic_attr_regs_access(struct kvm_device *dev,
struct kvm_device_attr *attr,u32 *reg, bool is_write)+{
- const struct mmio_range *r = NULL;
- phys_addr_t offset;
- int cpuid;
- struct kvm_vcpu *vcpu;
- struct kvm_exit_mmio mmio;
- offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
- cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
KVM_DEV_ARM_VGIC_CPUID_SHIFT;- if (cpuid >= atomic_read(&dev->kvm->online_vcpus))
return -EINVAL;- vcpu = kvm_get_vcpu(dev->kvm, cpuid);
- mmio.len = 4;
- mmio.is_write = is_write;
- if (is_write)
memcpy(mmio.data, reg, sizeof(*reg));Is this endianness safe?
With a big-endian kernel, no. But I suspect that breaks KVM elsewhere too. However, this is actually nicer:
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index ea7fb5c..191ff9f 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1622,7 +1622,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, mmio.len = 4; mmio.is_write = is_write; if (is_write)
memcpy(mmio.data, reg, sizeof(*reg));
mmio_data_write(mmio, ~0, *reg);
ahem, that's &mmio
if (attr->group == KVM_DEV_ARM_VGIC_GRP_DIST_REGS) r = find_matching_range(vgic_dist_ranges, &mmio, offset); @@ -1638,7 +1638,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev, spin_unlock(&vcpu->kvm->arch.vgic.lock); if (!is_write)
memcpy(reg, mmio.data, sizeof(*reg));
*reg = mmio_data_read(mmio, ~0);
ahem, that's &mmio
return 0; }
Thanks, -Christoffer
Handle MMIO accesses to the two registers which should support both the case where the VMs want to read/write either of these registers and the case where user space reads/writes these registers to do save/restore of the VGIC state.
Note that the added complexity compared to simple set/clear enable registers stems from the bookkeping of source cpu ids. It may be possible to change the underlying data structure to simplify the complexity, but since this is not in the critical path, at all, this is left as an interesting excercise to the reader.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org --- virt/kvm/arm/vgic.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 112 insertions(+), 2 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index e31625c..d44b5a1 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -591,18 +591,128 @@ static bool handle_mmio_sgi_reg(struct kvm_vcpu *vcpu, return false; }
+static void read_sgi_set_clear(struct kvm_vcpu *vcpu, + struct kvm_exit_mmio *mmio, + phys_addr_t offset) +{ + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; + int i, sgi, cpu; + int min_sgi = (offset & ~0x3) * 4; + int max_sgi = min_sgi + 3; + int vcpu_id = vcpu->vcpu_id; + u32 lr, reg = 0; + + /* Copy source SGIs from distributor side */ + for (sgi = min_sgi; sgi <= max_sgi; sgi++) { + int shift = 8 * (sgi - min_sgi); + reg |= (u32)dist->irq_sgi_sources[vcpu_id][sgi] << shift; + } + + /* Copy source SGIs already on LRs */ + for_each_set_bit(i, vgic_cpu->lr_used, vgic_cpu->nr_lr) { + lr = vgic_cpu->vgic_lr[i]; + sgi = lr & GICH_LR_VIRTUALID; + cpu = (lr & GICH_LR_PHYSID_CPUID) >> GICH_LR_PHYSID_CPUID_SHIFT; + if (sgi >= min_sgi && sgi <= max_sgi) { + if (lr & GICH_LR_STATE) + reg |= (1 << cpu) << (8 * (sgi - min_sgi)); + } + } + + memcpy(mmio->data, ®, sizeof(reg)); +} + static bool handle_mmio_sgi_clear(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, phys_addr_t offset) { - return false; + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; + int i, sgi, cpu; + int min_sgi = (offset & ~0x3) * 4; + int max_sgi = min_sgi + 3; + int vcpu_id = vcpu->vcpu_id; + u32 *lr, reg; + bool updated = false; + + if (!mmio->is_write) { + read_sgi_set_clear(vcpu, mmio, offset); + return false; + } + + memcpy(®, mmio->data, sizeof(reg)); + + /* Clear pending SGIs on distributor side */ + for (sgi = min_sgi; sgi <= max_sgi; sgi++) { + u8 mask = reg >> (8 * (sgi - min_sgi)); + if (dist->irq_sgi_sources[vcpu_id][sgi] & mask) + updated = true; + dist->irq_sgi_sources[vcpu_id][sgi] &= ~mask; + } + + /* Clear SGIs already on LRs */ + for_each_set_bit(i, vgic_cpu->lr_used, vgic_cpu->nr_lr) { + lr = &vgic_cpu->vgic_lr[i]; + sgi = *lr & GICH_LR_VIRTUALID; + cpu = (*lr & GICH_LR_PHYSID_CPUID) >> GICH_LR_PHYSID_CPUID_SHIFT; + + if (sgi >= min_sgi && sgi <= max_sgi) { + if (reg & ((1 << cpu) << (8 * (sgi - min_sgi)))) { + if (*lr & GICH_LR_PENDING_BIT) + updated = true; + *lr &= GICH_LR_PENDING_BIT; + } + } + } + + return updated; }
static bool handle_mmio_sgi_set(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, phys_addr_t offset) { - return false; + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; + int i, sgi, cpu; + int min_sgi = (offset & ~0x3) * 4; + int max_sgi = min_sgi + 3; + int vcpu_id = vcpu->vcpu_id; + u32 *lr, reg; + bool updated = false; + + if (!mmio->is_write) { + read_sgi_set_clear(vcpu, mmio, offset); + return false; + } + + memcpy(®, mmio->data, sizeof(reg)); + + /* Set pending SGIs on distributor side */ + for (sgi = min_sgi; sgi <= max_sgi; sgi++) { + u8 mask = reg >> (8 * (sgi - min_sgi)); + if ((dist->irq_sgi_sources[vcpu_id][sgi] & mask) != mask) + updated = true; + dist->irq_sgi_sources[vcpu_id][sgi] |= mask; + } + + /* Set active SGIs already on LRs to pending and active */ + for_each_set_bit(i, vgic_cpu->lr_used, vgic_cpu->nr_lr) { + lr = &vgic_cpu->vgic_lr[i]; + sgi = *lr & GICH_LR_VIRTUALID; + cpu = (*lr & GICH_LR_PHYSID_CPUID) >> GICH_LR_PHYSID_CPUID_SHIFT; + + if (sgi >= min_sgi && sgi <= max_sgi) { + if (reg & ((1 << cpu) << (8 * (sgi - min_sgi)))) { + if (!(*lr & GICH_LR_PENDING_BIT)) + updated = true; + *lr |= GICH_LR_PENDING_BIT; + } + } + } + + return updated; }
/*
Implement support for the CPU interface register access driven by MMIO address offsets from the CPU interface base address. Useful for user space to support save/restore of the VGIC state.
This commit adds support only for the same logic as the current VGIC support, and no more. For example, the active priority registers are handled as RAZ/WI, just like setting priorities on the emulated distributor.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org --- virt/kvm/arm/vgic.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index d44b5a1..257dbae 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1684,9 +1684,67 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, phys_addr_t offset) { - return true; + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; + u32 reg, mask = 0, shift = 0; + bool updated = false; + + switch (offset & ~0x3) { + case GIC_CPU_CTRL: + mask = GICH_VMCR_CTRL_MASK; + shift = GICH_VMCR_CTRL_SHIFT; + break; + case GIC_CPU_PRIMASK: + mask = GICH_VMCR_PRIMASK_MASK; + shift = GICH_VMCR_PRIMASK_SHIFT; + break; + case GIC_CPU_BINPOINT: + mask = GICH_VMCR_BINPOINT_MASK; + shift = GICH_VMCR_BINPOINT_SHIFT; + break; + case GIC_CPU_ALIAS_BINPOINT: + mask = GICH_VMCR_ALIAS_BINPOINT_MASK; + shift = GICH_VMCR_ALIAS_BINPOINT_SHIFT; + break; + } + + if (!mmio->is_write) { + reg = (vgic_cpu->vgic_vmcr & mask) >> shift; + memcpy(mmio->data, ®, sizeof(reg)); + } else { + memcpy(®, mmio->data, sizeof(reg)); + reg = (reg << shift) & mask; + if (reg != (vgic_cpu->vgic_vmcr & mask)) + updated = true; + vgic_cpu->vgic_vmcr &= ~mask; + vgic_cpu->vgic_vmcr |= reg; + } + return updated; +} + +static bool handle_mmio_abpr(struct kvm_vcpu *vcpu, + struct kvm_exit_mmio *mmio, phys_addr_t offset) +{ + return handle_cpu_mmio_misc(vcpu, mmio, GIC_CPU_ALIAS_BINPOINT); +} + +static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu, + struct kvm_exit_mmio *mmio, + phys_addr_t offset) +{ + u32 reg; + + if (mmio->is_write) + return false; + + reg = 0x0002043B; + memcpy(mmio->data, ®, sizeof(reg)); + return false; }
+/* + * CPU Interface Register accesses - these are not accessed by the VM, but by + * user space for saving and restoring VGIC state. + */ static const struct mmio_range vgic_cpu_ranges[] = { { .base = GIC_CPU_CTRL, @@ -1696,17 +1754,17 @@ static const struct mmio_range vgic_cpu_ranges[] = { { .base = GIC_CPU_ALIAS_BINPOINT, .len = 4, - .handle_mmio = handle_cpu_mmio_misc, + .handle_mmio = handle_mmio_abpr, }, { .base = GIC_CPU_ACTIVEPRIO, .len = 16, - .handle_mmio = handle_cpu_mmio_misc, + .handle_mmio = handle_mmio_raz_wi, }, { .base = GIC_CPU_IDENT, .len = 4, - .handle_mmio = handle_cpu_mmio_misc, + .handle_mmio = handle_cpu_mmio_ident, }, };
On 23.08.2013, at 20:20, Christoffer Dall wrote:
Implement support for the CPU interface register access driven by MMIO address offsets from the CPU interface base address. Useful for user space to support save/restore of the VGIC state.
This commit adds support only for the same logic as the current VGIC support, and no more. For example, the active priority registers are handled as RAZ/WI, just like setting priorities on the emulated distributor.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org
virt/kvm/arm/vgic.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index d44b5a1..257dbae 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1684,9 +1684,67 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, phys_addr_t offset) {
- return true;
- struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
- u32 reg, mask = 0, shift = 0;
- bool updated = false;
- switch (offset & ~0x3) {
- case GIC_CPU_CTRL:
mask = GICH_VMCR_CTRL_MASK;shift = GICH_VMCR_CTRL_SHIFT;break;- case GIC_CPU_PRIMASK:
mask = GICH_VMCR_PRIMASK_MASK;shift = GICH_VMCR_PRIMASK_SHIFT;break;- case GIC_CPU_BINPOINT:
mask = GICH_VMCR_BINPOINT_MASK;shift = GICH_VMCR_BINPOINT_SHIFT;break;- case GIC_CPU_ALIAS_BINPOINT:
mask = GICH_VMCR_ALIAS_BINPOINT_MASK;shift = GICH_VMCR_ALIAS_BINPOINT_SHIFT;break;- }
- if (!mmio->is_write) {
reg = (vgic_cpu->vgic_vmcr & mask) >> shift;memcpy(mmio->data, ®, sizeof(reg));- } else {
memcpy(®, mmio->data, sizeof(reg));reg = (reg << shift) & mask;if (reg != (vgic_cpu->vgic_vmcr & mask))updated = true;vgic_cpu->vgic_vmcr &= ~mask;vgic_cpu->vgic_vmcr |= reg;- }
- return updated;
+}
+static bool handle_mmio_abpr(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio, phys_addr_t offset)+{
- return handle_cpu_mmio_misc(vcpu, mmio, GIC_CPU_ALIAS_BINPOINT);
+}
+static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- u32 reg;
- if (mmio->is_write)
return false;- reg = 0x0002043B;
This wants a comment and probably also a #define :).
Alex
On Sun, Aug 25, 2013 at 04:24:20PM +0100, Alexander Graf wrote:
On 23.08.2013, at 20:20, Christoffer Dall wrote:
Implement support for the CPU interface register access driven by MMIO address offsets from the CPU interface base address. Useful for user space to support save/restore of the VGIC state.
This commit adds support only for the same logic as the current VGIC support, and no more. For example, the active priority registers are handled as RAZ/WI, just like setting priorities on the emulated distributor.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org
virt/kvm/arm/vgic.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index d44b5a1..257dbae 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1684,9 +1684,67 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, phys_addr_t offset) {
- return true;
- struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
- u32 reg, mask = 0, shift = 0;
- bool updated = false;
- switch (offset & ~0x3) {
- case GIC_CPU_CTRL:
mask = GICH_VMCR_CTRL_MASK;shift = GICH_VMCR_CTRL_SHIFT;break;- case GIC_CPU_PRIMASK:
mask = GICH_VMCR_PRIMASK_MASK;shift = GICH_VMCR_PRIMASK_SHIFT;break;- case GIC_CPU_BINPOINT:
mask = GICH_VMCR_BINPOINT_MASK;shift = GICH_VMCR_BINPOINT_SHIFT;break;- case GIC_CPU_ALIAS_BINPOINT:
mask = GICH_VMCR_ALIAS_BINPOINT_MASK;shift = GICH_VMCR_ALIAS_BINPOINT_SHIFT;break;- }
- if (!mmio->is_write) {
reg = (vgic_cpu->vgic_vmcr & mask) >> shift;memcpy(mmio->data, ®, sizeof(reg));- } else {
memcpy(®, mmio->data, sizeof(reg));reg = (reg << shift) & mask;if (reg != (vgic_cpu->vgic_vmcr & mask))updated = true;vgic_cpu->vgic_vmcr &= ~mask;vgic_cpu->vgic_vmcr |= reg;- }
- return updated;
+}
+static bool handle_mmio_abpr(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio, phys_addr_t offset)+{
- return handle_cpu_mmio_misc(vcpu, mmio, GIC_CPU_ALIAS_BINPOINT);
+}
+static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- u32 reg;
- if (mmio->is_write)
return false;- reg = 0x0002043B;
This wants a comment and probably also a #define :).
Marc, where does the 0x4b0 product id code come from for the distributor IIDR?
Would this be satisfying?
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 5214424..558be38 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -71,6 +71,9 @@ #define VGIC_ADDR_UNDEF (-1) #define IS_VGIC_ADDR_UNDEF(_x) ((_x) == VGIC_ADDR_UNDEF)
+#define GIC_PRODUCT_ID 0x4b0 +#define ARM_JEP106_IMPLEMENTER 0x43b + /* Physical address of vgic virtual cpu interface */ static phys_addr_t vgic_vcpu_base;
@@ -331,7 +334,7 @@ static bool handle_mmio_misc(struct kvm_vcpu *vcpu, break;
case 8: /* IIDR */ - reg = 0x4B00043B; + reg = (GIC_PRODUCT_ID << 20) | ARM_JEP106_IMPLEMENTER; vgic_reg_access(mmio, ®, word_offset, ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED); break; @@ -1734,7 +1737,7 @@ static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu, if (mmio->is_write) return false;
- reg = 0x0002043B; + reg = (GIC_PRODUCT_ID << 20) | (0x2 << 16) | ARM_JEP106_IMPLEMENTER; mmio_data_write(mmio, ~0, reg); return false; }
Thanks, -Christoffer
On 25.09.2013, at 23:30, Christoffer Dall wrote:
On Sun, Aug 25, 2013 at 04:24:20PM +0100, Alexander Graf wrote:
On 23.08.2013, at 20:20, Christoffer Dall wrote:
Implement support for the CPU interface register access driven by MMIO address offsets from the CPU interface base address. Useful for user space to support save/restore of the VGIC state.
This commit adds support only for the same logic as the current VGIC support, and no more. For example, the active priority registers are handled as RAZ/WI, just like setting priorities on the emulated distributor.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org
virt/kvm/arm/vgic.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index d44b5a1..257dbae 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1684,9 +1684,67 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, phys_addr_t offset) {
- return true;
- struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
- u32 reg, mask = 0, shift = 0;
- bool updated = false;
- switch (offset & ~0x3) {
- case GIC_CPU_CTRL:
mask = GICH_VMCR_CTRL_MASK;shift = GICH_VMCR_CTRL_SHIFT;break;- case GIC_CPU_PRIMASK:
mask = GICH_VMCR_PRIMASK_MASK;shift = GICH_VMCR_PRIMASK_SHIFT;break;- case GIC_CPU_BINPOINT:
mask = GICH_VMCR_BINPOINT_MASK;shift = GICH_VMCR_BINPOINT_SHIFT;break;- case GIC_CPU_ALIAS_BINPOINT:
mask = GICH_VMCR_ALIAS_BINPOINT_MASK;shift = GICH_VMCR_ALIAS_BINPOINT_SHIFT;break;- }
- if (!mmio->is_write) {
reg = (vgic_cpu->vgic_vmcr & mask) >> shift;memcpy(mmio->data, ®, sizeof(reg));- } else {
memcpy(®, mmio->data, sizeof(reg));reg = (reg << shift) & mask;if (reg != (vgic_cpu->vgic_vmcr & mask))updated = true;vgic_cpu->vgic_vmcr &= ~mask;vgic_cpu->vgic_vmcr |= reg;- }
- return updated;
+}
+static bool handle_mmio_abpr(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio, phys_addr_t offset)+{
- return handle_cpu_mmio_misc(vcpu, mmio, GIC_CPU_ALIAS_BINPOINT);
+}
+static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- u32 reg;
- if (mmio->is_write)
return false;- reg = 0x0002043B;
This wants a comment and probably also a #define :).
Marc, where does the 0x4b0 product id code come from for the distributor IIDR?
Would this be satisfying?
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 5214424..558be38 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -71,6 +71,9 @@ #define VGIC_ADDR_UNDEF (-1) #define IS_VGIC_ADDR_UNDEF(_x) ((_x) == VGIC_ADDR_UNDEF)
+#define GIC_PRODUCT_ID 0x4b0
This is a specific GIC version. PL390 for example is 0x3b0:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0416b/Beiggeb...
That should be reflected in the #define. If it means "GICv2" then it should be GICV2_PRODUCT_ID for example.
+#define ARM_JEP106_IMPLEMENTER 0x43b
I think naming this JEP106_IMPLEMENTER_ARM makes it more obvious that we're talking about the implementer code for ARM. Or maybe just only IMPLEMENTER_ARM.
/* Physical address of vgic virtual cpu interface */ static phys_addr_t vgic_vcpu_base;
@@ -331,7 +334,7 @@ static bool handle_mmio_misc(struct kvm_vcpu *vcpu, break;
case 8: /* IIDR */
reg = 0x4B00043B;
reg = (GIC_PRODUCT_ID << 20) | ARM_JEP106_IMPLEMENTER;@@ -1734,7 +1737,7 @@ static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu, if (mmio->is_write) return false;
- reg = 0x0002043B;
- reg = (GIC_PRODUCT_ID << 20) | (0x2 << 16) | ARM_JEP106_IMPLEMENTER;
What is the 0x2 here?
Alex
mmio_data_write(mmio, ~0, reg); return false; }
Thanks, -Christoffer
On Thu, Sep 26, 2013 at 12:37:03AM +0200, Alexander Graf wrote:
On 25.09.2013, at 23:30, Christoffer Dall wrote:
On Sun, Aug 25, 2013 at 04:24:20PM +0100, Alexander Graf wrote:
On 23.08.2013, at 20:20, Christoffer Dall wrote:
Implement support for the CPU interface register access driven by MMIO address offsets from the CPU interface base address. Useful for user space to support save/restore of the VGIC state.
This commit adds support only for the same logic as the current VGIC support, and no more. For example, the active priority registers are handled as RAZ/WI, just like setting priorities on the emulated distributor.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org
virt/kvm/arm/vgic.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index d44b5a1..257dbae 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1684,9 +1684,67 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, phys_addr_t offset) {
- return true;
- struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
- u32 reg, mask = 0, shift = 0;
- bool updated = false;
- switch (offset & ~0x3) {
- case GIC_CPU_CTRL:
mask = GICH_VMCR_CTRL_MASK;shift = GICH_VMCR_CTRL_SHIFT;break;- case GIC_CPU_PRIMASK:
mask = GICH_VMCR_PRIMASK_MASK;shift = GICH_VMCR_PRIMASK_SHIFT;break;- case GIC_CPU_BINPOINT:
mask = GICH_VMCR_BINPOINT_MASK;shift = GICH_VMCR_BINPOINT_SHIFT;break;- case GIC_CPU_ALIAS_BINPOINT:
mask = GICH_VMCR_ALIAS_BINPOINT_MASK;shift = GICH_VMCR_ALIAS_BINPOINT_SHIFT;break;- }
- if (!mmio->is_write) {
reg = (vgic_cpu->vgic_vmcr & mask) >> shift;memcpy(mmio->data, ®, sizeof(reg));- } else {
memcpy(®, mmio->data, sizeof(reg));reg = (reg << shift) & mask;if (reg != (vgic_cpu->vgic_vmcr & mask))updated = true;vgic_cpu->vgic_vmcr &= ~mask;vgic_cpu->vgic_vmcr |= reg;- }
- return updated;
+}
+static bool handle_mmio_abpr(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio, phys_addr_t offset)+{
- return handle_cpu_mmio_misc(vcpu, mmio, GIC_CPU_ALIAS_BINPOINT);
+}
+static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- u32 reg;
- if (mmio->is_write)
return false;- reg = 0x0002043B;
This wants a comment and probably also a #define :).
Marc, where does the 0x4b0 product id code come from for the distributor IIDR?
Would this be satisfying?
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 5214424..558be38 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -71,6 +71,9 @@ #define VGIC_ADDR_UNDEF (-1) #define IS_VGIC_ADDR_UNDEF(_x) ((_x) == VGIC_ADDR_UNDEF)
+#define GIC_PRODUCT_ID 0x4b0
This is a specific GIC version. PL390 for example is 0x3b0:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0416b/Beiggeb...
That should be reflected in the #define. If it means "GICv2" then it should be GICV2_PRODUCT_ID for example.
I know what field in the register it is thanks :)
But I couldn't find 0x4b0 anywhere in the docs, so I'm asking Marc where he got it from. I don't believe it means GICv2, but a specific implementation of a GICv2, and once I have more info I can change the define name, I suspect this is potentially something made-up to indicate that this is the KVM VGIC...
+#define ARM_JEP106_IMPLEMENTER 0x43b
I think naming this JEP106_IMPLEMENTER_ARM makes it more obvious that we're talking about the implementer code for ARM. Or maybe just only IMPLEMENTER_ARM.
ok
/* Physical address of vgic virtual cpu interface */ static phys_addr_t vgic_vcpu_base;
@@ -331,7 +334,7 @@ static bool handle_mmio_misc(struct kvm_vcpu *vcpu, break;
case 8: /* IIDR */
reg = 0x4B00043B;
reg = (GIC_PRODUCT_ID << 20) | ARM_JEP106_IMPLEMENTER;@@ -1734,7 +1737,7 @@ static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu, if (mmio->is_write) return false;
- reg = 0x0002043B;
- reg = (GIC_PRODUCT_ID << 20) | (0x2 << 16) | ARM_JEP106_IMPLEMENTER;
What is the 0x2 here?
GICv2, and now you're going to tell me to create a define for it right?
Which of course I can, but it's getting a bit silly, because then you can ask me what is the field shifted at 16 bits, and the next question is what is the GICC_IIDR, and at some point you're just going to have to look up the definition of this specific register in the GIC specs.
-Christoffer
On 26.09.2013, at 02:54, Christoffer Dall wrote:
On Thu, Sep 26, 2013 at 12:37:03AM +0200, Alexander Graf wrote:
On 25.09.2013, at 23:30, Christoffer Dall wrote:
On Sun, Aug 25, 2013 at 04:24:20PM +0100, Alexander Graf wrote:
On 23.08.2013, at 20:20, Christoffer Dall wrote:
Implement support for the CPU interface register access driven by MMIO address offsets from the CPU interface base address. Useful for user space to support save/restore of the VGIC state.
This commit adds support only for the same logic as the current VGIC support, and no more. For example, the active priority registers are handled as RAZ/WI, just like setting priorities on the emulated distributor.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org
virt/kvm/arm/vgic.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index d44b5a1..257dbae 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1684,9 +1684,67 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, phys_addr_t offset) {
- return true;
- struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
- u32 reg, mask = 0, shift = 0;
- bool updated = false;
- switch (offset & ~0x3) {
- case GIC_CPU_CTRL:
mask = GICH_VMCR_CTRL_MASK;shift = GICH_VMCR_CTRL_SHIFT;break;- case GIC_CPU_PRIMASK:
mask = GICH_VMCR_PRIMASK_MASK;shift = GICH_VMCR_PRIMASK_SHIFT;break;- case GIC_CPU_BINPOINT:
mask = GICH_VMCR_BINPOINT_MASK;shift = GICH_VMCR_BINPOINT_SHIFT;break;- case GIC_CPU_ALIAS_BINPOINT:
mask = GICH_VMCR_ALIAS_BINPOINT_MASK;shift = GICH_VMCR_ALIAS_BINPOINT_SHIFT;break;- }
- if (!mmio->is_write) {
reg = (vgic_cpu->vgic_vmcr & mask) >> shift;memcpy(mmio->data, ®, sizeof(reg));- } else {
memcpy(®, mmio->data, sizeof(reg));reg = (reg << shift) & mask;if (reg != (vgic_cpu->vgic_vmcr & mask))updated = true;vgic_cpu->vgic_vmcr &= ~mask;vgic_cpu->vgic_vmcr |= reg;- }
- return updated;
+}
+static bool handle_mmio_abpr(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio, phys_addr_t offset)+{
- return handle_cpu_mmio_misc(vcpu, mmio, GIC_CPU_ALIAS_BINPOINT);
+}
+static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- u32 reg;
- if (mmio->is_write)
return false;- reg = 0x0002043B;
This wants a comment and probably also a #define :).
Marc, where does the 0x4b0 product id code come from for the distributor IIDR?
Would this be satisfying?
^
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 5214424..558be38 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -71,6 +71,9 @@ #define VGIC_ADDR_UNDEF (-1) #define IS_VGIC_ADDR_UNDEF(_x) ((_x) == VGIC_ADDR_UNDEF)
+#define GIC_PRODUCT_ID 0x4b0
This is a specific GIC version. PL390 for example is 0x3b0:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0416b/Beiggeb...
That should be reflected in the #define. If it means "GICv2" then it should be GICV2_PRODUCT_ID for example.
I know what field in the register it is thanks :)
But I couldn't find 0x4b0 anywhere in the docs, so I'm asking Marc where he got it from. I don't believe it means GICv2, but a
Ah, ok. Then the answer to your question above is a simple "no" as the name doesn't really tell us everything we want to know yet :).
specific implementation of a GICv2, and once I have more info I can change the define name, I suspect this is potentially something made-up to indicate that this is the KVM VGIC...
Hrm, makes sense. So that also explains why there's a special version field.
+#define ARM_JEP106_IMPLEMENTER 0x43b
I think naming this JEP106_IMPLEMENTER_ARM makes it more obvious that we're talking about the implementer code for ARM. Or maybe just only IMPLEMENTER_ARM.
ok
/* Physical address of vgic virtual cpu interface */ static phys_addr_t vgic_vcpu_base;
@@ -331,7 +334,7 @@ static bool handle_mmio_misc(struct kvm_vcpu *vcpu, break;
case 8: /* IIDR */
reg = 0x4B00043B;
reg = (GIC_PRODUCT_ID << 20) | ARM_JEP106_IMPLEMENTER;@@ -1734,7 +1737,7 @@ static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu, if (mmio->is_write) return false;
- reg = 0x0002043B;
- reg = (GIC_PRODUCT_ID << 20) | (0x2 << 16) | ARM_JEP106_IMPLEMENTER;
What is the 0x2 here?
GICv2, and now you're going to tell me to create a define for it right?
Which of course I can, but it's getting a bit silly, because then you can ask me what is the field shifted at 16 bits, and the next question is what is the GICC_IIDR, and at some point you're just going to have to look up the definition of this specific register in the GIC specs.
The main point I'm trying to make is that someone should be able to figure out what these things mean without constantly having the spec next to him.
Having names to shifts helps with that too, yes. In other cases we try to use structs to make it easier for readers to understand what's going on, but that's quite hard with a u32 number :).
In fact, I even asked you despite having the spec open next to me. So yes, defines for the shifts do make sense.
I don't quite see the connection on why I would ask you what GICC_IIDR is though. I can easily google that or search in the spec for it. I can however not google 0x43b or 0x2 << 16.
Alex
On 26.09.2013, at 03:15, Alexander Graf wrote:
On 26.09.2013, at 02:54, Christoffer Dall wrote:
On Thu, Sep 26, 2013 at 12:37:03AM +0200, Alexander Graf wrote:
On 25.09.2013, at 23:30, Christoffer Dall wrote:
On Sun, Aug 25, 2013 at 04:24:20PM +0100, Alexander Graf wrote:
On 23.08.2013, at 20:20, Christoffer Dall wrote:
Implement support for the CPU interface register access driven by MMIO address offsets from the CPU interface base address. Useful for user space to support save/restore of the VGIC state.
This commit adds support only for the same logic as the current VGIC support, and no more. For example, the active priority registers are handled as RAZ/WI, just like setting priorities on the emulated distributor.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org
virt/kvm/arm/vgic.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index d44b5a1..257dbae 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1684,9 +1684,67 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, phys_addr_t offset) {
- return true;
- struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
- u32 reg, mask = 0, shift = 0;
- bool updated = false;
- switch (offset & ~0x3) {
- case GIC_CPU_CTRL:
mask = GICH_VMCR_CTRL_MASK;shift = GICH_VMCR_CTRL_SHIFT;break;- case GIC_CPU_PRIMASK:
mask = GICH_VMCR_PRIMASK_MASK;shift = GICH_VMCR_PRIMASK_SHIFT;break;- case GIC_CPU_BINPOINT:
mask = GICH_VMCR_BINPOINT_MASK;shift = GICH_VMCR_BINPOINT_SHIFT;break;- case GIC_CPU_ALIAS_BINPOINT:
mask = GICH_VMCR_ALIAS_BINPOINT_MASK;shift = GICH_VMCR_ALIAS_BINPOINT_SHIFT;break;- }
- if (!mmio->is_write) {
reg = (vgic_cpu->vgic_vmcr & mask) >> shift;memcpy(mmio->data, ®, sizeof(reg));- } else {
memcpy(®, mmio->data, sizeof(reg));reg = (reg << shift) & mask;if (reg != (vgic_cpu->vgic_vmcr & mask))updated = true;vgic_cpu->vgic_vmcr &= ~mask;vgic_cpu->vgic_vmcr |= reg;- }
- return updated;
+}
+static bool handle_mmio_abpr(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio, phys_addr_t offset)+{
- return handle_cpu_mmio_misc(vcpu, mmio, GIC_CPU_ALIAS_BINPOINT);
+}
+static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- u32 reg;
- if (mmio->is_write)
return false;- reg = 0x0002043B;
This wants a comment and probably also a #define :).
Marc, where does the 0x4b0 product id code come from for the distributor IIDR?
Would this be satisfying?
^
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 5214424..558be38 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -71,6 +71,9 @@ #define VGIC_ADDR_UNDEF (-1) #define IS_VGIC_ADDR_UNDEF(_x) ((_x) == VGIC_ADDR_UNDEF)
+#define GIC_PRODUCT_ID 0x4b0
This is a specific GIC version. PL390 for example is 0x3b0:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0416b/Beiggeb...
That should be reflected in the #define. If it means "GICv2" then it should be GICV2_PRODUCT_ID for example.
I know what field in the register it is thanks :)
But I couldn't find 0x4b0 anywhere in the docs, so I'm asking Marc where he got it from. I don't believe it means GICv2, but a
Ah, ok. Then the answer to your question above is a simple "no" as the name doesn't really tell us everything we want to know yet :).
specific implementation of a GICv2, and once I have more info I can change the define name, I suspect this is potentially something made-up to indicate that this is the KVM VGIC...
Hrm, makes sense. So that also explains why there's a special version field.
It doesn't explain why it only gets set in one of the IIDR variants though. Is this on purpose? From what I can tell, the CPU and Distributor interfaces both should return the same number here.
Alex
+#define ARM_JEP106_IMPLEMENTER 0x43b
I think naming this JEP106_IMPLEMENTER_ARM makes it more obvious that we're talking about the implementer code for ARM. Or maybe just only IMPLEMENTER_ARM.
ok
/* Physical address of vgic virtual cpu interface */ static phys_addr_t vgic_vcpu_base;
@@ -331,7 +334,7 @@ static bool handle_mmio_misc(struct kvm_vcpu *vcpu, break;
case 8: /* IIDR */
reg = 0x4B00043B;
reg = (GIC_PRODUCT_ID << 20) | ARM_JEP106_IMPLEMENTER;@@ -1734,7 +1737,7 @@ static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu, if (mmio->is_write) return false;
- reg = 0x0002043B;
- reg = (GIC_PRODUCT_ID << 20) | (0x2 << 16) | ARM_JEP106_IMPLEMENTER;
What is the 0x2 here?
GICv2, and now you're going to tell me to create a define for it right?
Which of course I can, but it's getting a bit silly, because then you can ask me what is the field shifted at 16 bits, and the next question is what is the GICC_IIDR, and at some point you're just going to have to look up the definition of this specific register in the GIC specs.
The main point I'm trying to make is that someone should be able to figure out what these things mean without constantly having the spec next to him.
Having names to shifts helps with that too, yes. In other cases we try to use structs to make it easier for readers to understand what's going on, but that's quite hard with a u32 number :).
In fact, I even asked you despite having the spec open next to me. So yes, defines for the shifts do make sense.
I don't quite see the connection on why I would ask you what GICC_IIDR is though. I can easily google that or search in the spec for it. I can however not google 0x43b or 0x2 << 16.
Alex
kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
On 26.09.2013, at 03:36, Alexander Graf wrote:
On 26.09.2013, at 03:15, Alexander Graf wrote:
On 26.09.2013, at 02:54, Christoffer Dall wrote:
On Thu, Sep 26, 2013 at 12:37:03AM +0200, Alexander Graf wrote:
On 25.09.2013, at 23:30, Christoffer Dall wrote:
On Sun, Aug 25, 2013 at 04:24:20PM +0100, Alexander Graf wrote:
On 23.08.2013, at 20:20, Christoffer Dall wrote:
> Implement support for the CPU interface register access driven by MMIO > address offsets from the CPU interface base address. Useful for user > space to support save/restore of the VGIC state. > > This commit adds support only for the same logic as the current VGIC > support, and no more. For example, the active priority registers are > handled as RAZ/WI, just like setting priorities on the emulated > distributor. > > Signed-off-by: Christoffer Dall christoffer.dall@linaro.org > --- > virt/kvm/arm/vgic.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 62 insertions(+), 4 deletions(-) > > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > index d44b5a1..257dbae 100644 > --- a/virt/kvm/arm/vgic.c > +++ b/virt/kvm/arm/vgic.c > @@ -1684,9 +1684,67 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) > static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu, > struct kvm_exit_mmio *mmio, phys_addr_t offset) > { > - return true; > + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > + u32 reg, mask = 0, shift = 0; > + bool updated = false; > + > + switch (offset & ~0x3) { > + case GIC_CPU_CTRL: > + mask = GICH_VMCR_CTRL_MASK; > + shift = GICH_VMCR_CTRL_SHIFT; > + break; > + case GIC_CPU_PRIMASK: > + mask = GICH_VMCR_PRIMASK_MASK; > + shift = GICH_VMCR_PRIMASK_SHIFT; > + break; > + case GIC_CPU_BINPOINT: > + mask = GICH_VMCR_BINPOINT_MASK; > + shift = GICH_VMCR_BINPOINT_SHIFT; > + break; > + case GIC_CPU_ALIAS_BINPOINT: > + mask = GICH_VMCR_ALIAS_BINPOINT_MASK; > + shift = GICH_VMCR_ALIAS_BINPOINT_SHIFT; > + break; > + } > + > + if (!mmio->is_write) { > + reg = (vgic_cpu->vgic_vmcr & mask) >> shift; > + memcpy(mmio->data, ®, sizeof(reg)); > + } else { > + memcpy(®, mmio->data, sizeof(reg)); > + reg = (reg << shift) & mask; > + if (reg != (vgic_cpu->vgic_vmcr & mask)) > + updated = true; > + vgic_cpu->vgic_vmcr &= ~mask; > + vgic_cpu->vgic_vmcr |= reg; > + } > + return updated; > +} > + > +static bool handle_mmio_abpr(struct kvm_vcpu *vcpu, > + struct kvm_exit_mmio *mmio, phys_addr_t offset) > +{ > + return handle_cpu_mmio_misc(vcpu, mmio, GIC_CPU_ALIAS_BINPOINT); > +} > + > +static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu, > + struct kvm_exit_mmio *mmio, > + phys_addr_t offset) > +{ > + u32 reg; > + > + if (mmio->is_write) > + return false; > + > + reg = 0x0002043B;
This wants a comment and probably also a #define :).
Marc, where does the 0x4b0 product id code come from for the distributor IIDR?
Would this be satisfying?
^
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 5214424..558be38 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -71,6 +71,9 @@ #define VGIC_ADDR_UNDEF (-1) #define IS_VGIC_ADDR_UNDEF(_x) ((_x) == VGIC_ADDR_UNDEF)
+#define GIC_PRODUCT_ID 0x4b0
This is a specific GIC version. PL390 for example is 0x3b0:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0416b/Beiggeb...
That should be reflected in the #define. If it means "GICv2" then it should be GICV2_PRODUCT_ID for example.
I know what field in the register it is thanks :)
But I couldn't find 0x4b0 anywhere in the docs, so I'm asking Marc where he got it from. I don't believe it means GICv2, but a
Ah, ok. Then the answer to your question above is a simple "no" as the name doesn't really tell us everything we want to know yet :).
specific implementation of a GICv2, and once I have more info I can change the define name, I suspect this is potentially something made-up to indicate that this is the KVM VGIC...
Hrm, makes sense. So that also explains why there's a special version field.
It doesn't explain why it only gets set in one of the IIDR variants though. Is this on purpose? From what I can tell, the CPU and Distributor interfaces both should return the same number here.
Hrm. Curious. According to
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0438i/BABGBHB...
the proper values for IIDR on an A15 are:
GICD_IIDR 0x0000043B GICC_IIDR 0x0002043B
So what do the fields mean in each register?
GICD_IIDR:
[31:24] ProductID [23:20] - [19:16] Variant [15:12] Revision [11:0] Implementer
GICC_IIDR:
[31:20] ProductID [19:16] Architecture version [15:12] Revision [11:0] Implementer
So while 19:16 in GICC denotes the "Architecture version" (GICv2 for us), it means "Variant" for GICD. What a mess.
Alex
On Thu, Sep 26, 2013 at 03:15:47AM +0200, Alexander Graf wrote:
On 26.09.2013, at 02:54, Christoffer Dall wrote:
On Thu, Sep 26, 2013 at 12:37:03AM +0200, Alexander Graf wrote:
On 25.09.2013, at 23:30, Christoffer Dall wrote:
On Sun, Aug 25, 2013 at 04:24:20PM +0100, Alexander Graf wrote:
On 23.08.2013, at 20:20, Christoffer Dall wrote:
Implement support for the CPU interface register access driven by MMIO address offsets from the CPU interface base address. Useful for user space to support save/restore of the VGIC state.
This commit adds support only for the same logic as the current VGIC support, and no more. For example, the active priority registers are handled as RAZ/WI, just like setting priorities on the emulated distributor.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org
virt/kvm/arm/vgic.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index d44b5a1..257dbae 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1684,9 +1684,67 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, phys_addr_t offset) {
- return true;
- struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
- u32 reg, mask = 0, shift = 0;
- bool updated = false;
- switch (offset & ~0x3) {
- case GIC_CPU_CTRL:
mask = GICH_VMCR_CTRL_MASK;shift = GICH_VMCR_CTRL_SHIFT;break;- case GIC_CPU_PRIMASK:
mask = GICH_VMCR_PRIMASK_MASK;shift = GICH_VMCR_PRIMASK_SHIFT;break;- case GIC_CPU_BINPOINT:
mask = GICH_VMCR_BINPOINT_MASK;shift = GICH_VMCR_BINPOINT_SHIFT;break;- case GIC_CPU_ALIAS_BINPOINT:
mask = GICH_VMCR_ALIAS_BINPOINT_MASK;shift = GICH_VMCR_ALIAS_BINPOINT_SHIFT;break;- }
- if (!mmio->is_write) {
reg = (vgic_cpu->vgic_vmcr & mask) >> shift;memcpy(mmio->data, ®, sizeof(reg));- } else {
memcpy(®, mmio->data, sizeof(reg));reg = (reg << shift) & mask;if (reg != (vgic_cpu->vgic_vmcr & mask))updated = true;vgic_cpu->vgic_vmcr &= ~mask;vgic_cpu->vgic_vmcr |= reg;- }
- return updated;
+}
+static bool handle_mmio_abpr(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio, phys_addr_t offset)+{
- return handle_cpu_mmio_misc(vcpu, mmio, GIC_CPU_ALIAS_BINPOINT);
+}
+static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- u32 reg;
- if (mmio->is_write)
return false;- reg = 0x0002043B;
This wants a comment and probably also a #define :).
Marc, where does the 0x4b0 product id code come from for the distributor IIDR?
Would this be satisfying?
^
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 5214424..558be38 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -71,6 +71,9 @@ #define VGIC_ADDR_UNDEF (-1) #define IS_VGIC_ADDR_UNDEF(_x) ((_x) == VGIC_ADDR_UNDEF)
+#define GIC_PRODUCT_ID 0x4b0
This is a specific GIC version. PL390 for example is 0x3b0:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0416b/Beiggeb...
That should be reflected in the #define. If it means "GICv2" then it should be GICV2_PRODUCT_ID for example.
I know what field in the register it is thanks :)
But I couldn't find 0x4b0 anywhere in the docs, so I'm asking Marc where he got it from. I don't believe it means GICv2, but a
Ah, ok. Then the answer to your question above is a simple "no" as the name doesn't really tell us everything we want to know yet :).
specific implementation of a GICv2, and once I have more info I can change the define name, I suspect this is potentially something made-up to indicate that this is the KVM VGIC...
Hrm, makes sense. So that also explains why there's a special version field.
+#define ARM_JEP106_IMPLEMENTER 0x43b
I think naming this JEP106_IMPLEMENTER_ARM makes it more obvious that we're talking about the implementer code for ARM. Or maybe just only IMPLEMENTER_ARM.
ok
/* Physical address of vgic virtual cpu interface */ static phys_addr_t vgic_vcpu_base;
@@ -331,7 +334,7 @@ static bool handle_mmio_misc(struct kvm_vcpu *vcpu, break;
case 8: /* IIDR */
reg = 0x4B00043B;
reg = (GIC_PRODUCT_ID << 20) | ARM_JEP106_IMPLEMENTER;@@ -1734,7 +1737,7 @@ static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu, if (mmio->is_write) return false;
- reg = 0x0002043B;
- reg = (GIC_PRODUCT_ID << 20) | (0x2 << 16) | ARM_JEP106_IMPLEMENTER;
What is the 0x2 here?
GICv2, and now you're going to tell me to create a define for it right?
Which of course I can, but it's getting a bit silly, because then you can ask me what is the field shifted at 16 bits, and the next question is what is the GICC_IIDR, and at some point you're just going to have to look up the definition of this specific register in the GIC specs.
The main point I'm trying to make is that someone should be able to figure out what these things mean without constantly having the spec next to him.
Having names to shifts helps with that too, yes. In other cases we try to use structs to make it easier for readers to understand what's going on, but that's quite hard with a u32 number :).
In fact, I even asked you despite having the spec open next to me. So yes, defines for the shifts do make sense.
I don't quite see the connection on why I would ask you what GICC_IIDR is though. I can easily google that or search in the spec for it. I can however not google 0x43b or 0x2 << 16.
So, this essentially boils down to your "what a mess" comment in the other e-mail. If there were some generic or consistent or clear approach to this, or these values could be used elsewhere in the kernel or would tell anyone reading the code a lot about the functionality, api, etc. by introducing the defines or adding comments, you know I'm the first to advocate this.
However, these two specific registers are super low-level thingys, with fields and values defined in the spec, and are simply returning ID registers that we don't even have an OS that checks for, afaict. Therefore, it's not like you'll see, VARIANT_SHIFT, and go, oh, therefore group-x interrrupts does FIQs (made-up stupid example), but the only take away here is that, hmm, this is an ID register, I wonder what the scheme is, let me look it up anyways.
My point about what IIDR is just that we have to draw the line somewhere, and I actually was OK with the hard-coded value because this is arcane and specific by the nature of the beast, and apparently the other people who reviewed the GICD_IIDR code before was of some similar state of mind.
The most helpful thing actually, would be to put a pointer in the comment to the document where you can read this, and explain the product id thingy. But then again, if you can't lookup the IIDR bit fields, then you have much bigger problems if you try to understand this code :)
This is, by the way, getting a bit out hand. I'll see what the story behind the product id is tomorrow, and see if Marc wants to chip in here, and then I will add as many defines for shifts and values as I can possibly think of for these value for v2;) Sound good?
PS: I have a cold.
-Christoffer
Am 26.09.2013 um 04:49 schrieb Christoffer Dall christoffer.dall@linaro.org:
On Thu, Sep 26, 2013 at 03:15:47AM +0200, Alexander Graf wrote:
On 26.09.2013, at 02:54, Christoffer Dall wrote:
On Thu, Sep 26, 2013 at 12:37:03AM +0200, Alexander Graf wrote:
On 25.09.2013, at 23:30, Christoffer Dall wrote:
On Sun, Aug 25, 2013 at 04:24:20PM +0100, Alexander Graf wrote:
On 23.08.2013, at 20:20, Christoffer Dall wrote:
> Implement support for the CPU interface register access driven by MMIO > address offsets from the CPU interface base address. Useful for user > space to support save/restore of the VGIC state. > > This commit adds support only for the same logic as the current VGIC > support, and no more. For example, the active priority registers are > handled as RAZ/WI, just like setting priorities on the emulated > distributor. > > Signed-off-by: Christoffer Dall christoffer.dall@linaro.org > --- > virt/kvm/arm/vgic.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 62 insertions(+), 4 deletions(-) > > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > index d44b5a1..257dbae 100644 > --- a/virt/kvm/arm/vgic.c > +++ b/virt/kvm/arm/vgic.c > @@ -1684,9 +1684,67 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) > static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu, > struct kvm_exit_mmio *mmio, phys_addr_t offset) > { > - return true; > + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > + u32 reg, mask = 0, shift = 0; > + bool updated = false; > + > + switch (offset & ~0x3) { > + case GIC_CPU_CTRL: > + mask = GICH_VMCR_CTRL_MASK; > + shift = GICH_VMCR_CTRL_SHIFT; > + break; > + case GIC_CPU_PRIMASK: > + mask = GICH_VMCR_PRIMASK_MASK; > + shift = GICH_VMCR_PRIMASK_SHIFT; > + break; > + case GIC_CPU_BINPOINT: > + mask = GICH_VMCR_BINPOINT_MASK; > + shift = GICH_VMCR_BINPOINT_SHIFT; > + break; > + case GIC_CPU_ALIAS_BINPOINT: > + mask = GICH_VMCR_ALIAS_BINPOINT_MASK; > + shift = GICH_VMCR_ALIAS_BINPOINT_SHIFT; > + break; > + } > + > + if (!mmio->is_write) { > + reg = (vgic_cpu->vgic_vmcr & mask) >> shift; > + memcpy(mmio->data, ®, sizeof(reg)); > + } else { > + memcpy(®, mmio->data, sizeof(reg)); > + reg = (reg << shift) & mask; > + if (reg != (vgic_cpu->vgic_vmcr & mask)) > + updated = true; > + vgic_cpu->vgic_vmcr &= ~mask; > + vgic_cpu->vgic_vmcr |= reg; > + } > + return updated; > +} > + > +static bool handle_mmio_abpr(struct kvm_vcpu *vcpu, > + struct kvm_exit_mmio *mmio, phys_addr_t offset) > +{ > + return handle_cpu_mmio_misc(vcpu, mmio, GIC_CPU_ALIAS_BINPOINT); > +} > + > +static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu, > + struct kvm_exit_mmio *mmio, > + phys_addr_t offset) > +{ > + u32 reg; > + > + if (mmio->is_write) > + return false; > + > + reg = 0x0002043B;
This wants a comment and probably also a #define :).
Marc, where does the 0x4b0 product id code come from for the distributor IIDR?
Would this be satisfying?
^
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 5214424..558be38 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -71,6 +71,9 @@ #define VGIC_ADDR_UNDEF (-1) #define IS_VGIC_ADDR_UNDEF(_x) ((_x) == VGIC_ADDR_UNDEF)
+#define GIC_PRODUCT_ID 0x4b0
This is a specific GIC version. PL390 for example is 0x3b0:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0416b/Beiggeb...
That should be reflected in the #define. If it means "GICv2" then it should be GICV2_PRODUCT_ID for example.
I know what field in the register it is thanks :)
But I couldn't find 0x4b0 anywhere in the docs, so I'm asking Marc where he got it from. I don't believe it means GICv2, but a
Ah, ok. Then the answer to your question above is a simple "no" as the name doesn't really tell us everything we want to know yet :).
specific implementation of a GICv2, and once I have more info I can change the define name, I suspect this is potentially something made-up to indicate that this is the KVM VGIC...
Hrm, makes sense. So that also explains why there's a special version field.
+#define ARM_JEP106_IMPLEMENTER 0x43b
I think naming this JEP106_IMPLEMENTER_ARM makes it more obvious that we're talking about the implementer code for ARM. Or maybe just only IMPLEMENTER_ARM.
ok
/* Physical address of vgic virtual cpu interface */ static phys_addr_t vgic_vcpu_base;
@@ -331,7 +334,7 @@ static bool handle_mmio_misc(struct kvm_vcpu *vcpu, break;
case 8: /* IIDR */
reg = 0x4B00043B;
reg = (GIC_PRODUCT_ID << 20) | ARM_JEP106_IMPLEMENTER; vgic_reg_access(mmio, ®, word_offset, ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED); break;@@ -1734,7 +1737,7 @@ static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu, if (mmio->is_write) return false;
- reg = 0x0002043B;
- reg = (GIC_PRODUCT_ID << 20) | (0x2 << 16) | ARM_JEP106_IMPLEMENTER;
What is the 0x2 here?
GICv2, and now you're going to tell me to create a define for it right?
Which of course I can, but it's getting a bit silly, because then you can ask me what is the field shifted at 16 bits, and the next question is what is the GICC_IIDR, and at some point you're just going to have to look up the definition of this specific register in the GIC specs.
The main point I'm trying to make is that someone should be able to figure out what these things mean without constantly having the spec next to him.
Having names to shifts helps with that too, yes. In other cases we try to use structs to make it easier for readers to understand what's going on, but that's quite hard with a u32 number :).
In fact, I even asked you despite having the spec open next to me. So yes, defines for the shifts do make sense.
I don't quite see the connection on why I would ask you what GICC_IIDR is though. I can easily google that or search in the spec for it. I can however not google 0x43b or 0x2 << 16.
So, this essentially boils down to your "what a mess" comment in the other e-mail. If there were some generic or consistent or clear approach to this, or these values could be used elsewhere in the kernel or would tell anyone reading the code a lot about the functionality, api, etc. by introducing the defines or adding comments, you know I'm the first to advocate this.
However, these two specific registers are super low-level thingys, with fields and values defined in the spec, and are simply returning ID registers that we don't even have an OS that checks for, afaict. Therefore, it's not like you'll see, VARIANT_SHIFT, and go, oh, therefore group-x interrrupts does FIQs (made-up stupid example), but the only take away here is that, hmm, this is an ID register, I wonder what the scheme is, let me look it up anyways.
My point about what IIDR is just that we have to draw the line somewhere, and I actually was OK with the hard-coded value because this is arcane and specific by the nature of the beast, and apparently the other people who reviewed the GICD_IIDR code before was of some similar state of mind.
The most helpful thing actually, would be to put a pointer in the comment to the document where you can read this, and explain the product id thingy. But then again, if you can't lookup the IIDR bit fields, then you have much bigger problems if you try to understand this code :)
This is, by the way, getting a bit out hand. I'll see what the story behind the product id is tomorrow, and see if Marc wants to chip in here, and then I will add as many defines for shifts and values as I can possibly think of for these value for v2;) Sound good?
Heh :). Works for me. The fact that even you don't know what the product id means really shows that there's missing documentation here. And defines are a nice way to solve that.
PS: I have a cold.
Yeah, me too.
Alex
On 25/09/13 22:30, Christoffer Dall wrote:
On Sun, Aug 25, 2013 at 04:24:20PM +0100, Alexander Graf wrote:
On 23.08.2013, at 20:20, Christoffer Dall wrote:
Implement support for the CPU interface register access driven by MMIO address offsets from the CPU interface base address. Useful for user space to support save/restore of the VGIC state.
This commit adds support only for the same logic as the current VGIC support, and no more. For example, the active priority registers are handled as RAZ/WI, just like setting priorities on the emulated distributor.
Signed-off-by: Christoffer Dall christoffer.dall@linaro.org
virt/kvm/arm/vgic.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index d44b5a1..257dbae 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -1684,9 +1684,67 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write) static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, phys_addr_t offset) {
- return true;
- struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
- u32 reg, mask = 0, shift = 0;
- bool updated = false;
- switch (offset & ~0x3) {
- case GIC_CPU_CTRL:
mask = GICH_VMCR_CTRL_MASK;shift = GICH_VMCR_CTRL_SHIFT;break;- case GIC_CPU_PRIMASK:
mask = GICH_VMCR_PRIMASK_MASK;shift = GICH_VMCR_PRIMASK_SHIFT;break;- case GIC_CPU_BINPOINT:
mask = GICH_VMCR_BINPOINT_MASK;shift = GICH_VMCR_BINPOINT_SHIFT;break;- case GIC_CPU_ALIAS_BINPOINT:
mask = GICH_VMCR_ALIAS_BINPOINT_MASK;shift = GICH_VMCR_ALIAS_BINPOINT_SHIFT;break;- }
- if (!mmio->is_write) {
reg = (vgic_cpu->vgic_vmcr & mask) >> shift;memcpy(mmio->data, ®, sizeof(reg));- } else {
memcpy(®, mmio->data, sizeof(reg));reg = (reg << shift) & mask;if (reg != (vgic_cpu->vgic_vmcr & mask))updated = true;vgic_cpu->vgic_vmcr &= ~mask;vgic_cpu->vgic_vmcr |= reg;- }
- return updated;
+}
+static bool handle_mmio_abpr(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio, phys_addr_t offset)+{
- return handle_cpu_mmio_misc(vcpu, mmio, GIC_CPU_ALIAS_BINPOINT);
+}
+static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu,
struct kvm_exit_mmio *mmio,phys_addr_t offset)+{
- u32 reg;
- if (mmio->is_write)
return false;- reg = 0x0002043B;
This wants a comment and probably also a #define :).
Marc, where does the 0x4b0 product id code come from for the distributor IIDR?
0x4B is the ASCII value for 'K', and the whole thing is intended to read as KVM/ARM. Call that a feeble attempt at an Easter Egg.
I'd very much refrain from pretending we emulate an existing GICv2 implementation, as the damned thing is behaving very differently from the hardware.
M.
On 23.08.2013, at 20:19, Christoffer Dall wrote:
Implement save/restore of the VGIC state using the newer KVM Device Control API. This requries some number of changes to existing code in addition to actually supporting save/restore of the necessary state.
The first patches (01-03) support creating the VGIC using the Device Control API. This change is necessary because there are no other suitable KVM APIs that we can leverage to access the VGIC state from user space and the device control API was crafted exactly for this purpose.
Subsequent patches add the missing infrastructure and user space API pieces necessary to actually save and restore the VGIC state. The GIC v2.0 architecture specification already specifies registers that can be used to save and restore the complete VGIC state for suspend/resume purposes on real hardware, and we can resuse this interface for the VGIC. The API is therefore based on the memory-mapped register accesses defined in the specs. See the individual patches for details.
The patches rely on a number of small fixes sent separately to actually work: git://git.linaro.org/people/cdall/linux-kvm-arm.git vgic-migrate-prereq
This patch series based on the above can be cloned from: git://git.linaro.org/people/cdall/linux-kvm-arm.git vgic-migrate
User space patches for QEMU will follow shortly. Tested on Versatile Express TC2.
Looks quite straight forward and nice.
Reviewed-by: Alexander Graf agraf@suse.de
Alex
linaro-kernel@lists.linaro.org
-
Alexander Graf -
Christoffer Dall -
Marc Zyngier