* [PATCH 0/2] KVM: ARM: Get rid of hardcoded VGIC addresses @ 2012-10-20 4:14 Christoffer Dall 2012-10-20 4:14 ` [PATCH 1/2] KVM: ARM: Introduce KVM_SET_DEVICE_ADDRESS ioctl Christoffer Dall 2012-10-20 4:14 ` [PATCH 2/2] KVM: ARM: Defer parts of the vgic init until first KVM_RUN Christoffer Dall 0 siblings, 2 replies; 5+ messages in thread From: Christoffer Dall @ 2012-10-20 4:14 UTC (permalink / raw) To: kvmarm; +Cc: kvm, Christoffer Dall We need a way to specify the address at which we expect VMs to access the interrupt controller (both the emulated distributor and the hardware interface supporting virtualization). User space should decide on this address as user space decides on an emulated board and loads a device tree describing these details directly to the guest. We introduce a new ioctl, KVM_SET_DEVICE_ADDRESS, that lets user space provide a base address for a device based on exported device ids. For now, this is only supported for the ARM vgic. User space provides this address after creating the IRQ chip and KVM performs the required mappings for a VM on the first execution of a VCPU. Christoffer Dall (2): KVM: ARM: Introduce KVM_SET_DEVICE_ADDRESS ioctl KVM: ARM: Defer parts of the vgic init until first KVM_RUN Documentation/virtual/kvm/api.txt | 37 ++++++++++++++ arch/arm/include/asm/kvm.h | 13 +++++ arch/arm/include/asm/kvm_mmu.h | 2 + arch/arm/include/asm/kvm_vgic.h | 27 ++++++++-- arch/arm/kvm/arm.c | 41 ++++++++++++++- arch/arm/kvm/vgic.c | 99 +++++++++++++++++++++++++++++-------- include/linux/kvm.h | 8 +++ 7 files changed, 201 insertions(+), 26 deletions(-) -- 1.7.9.5 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] KVM: ARM: Introduce KVM_SET_DEVICE_ADDRESS ioctl 2012-10-20 4:14 [PATCH 0/2] KVM: ARM: Get rid of hardcoded VGIC addresses Christoffer Dall @ 2012-10-20 4:14 ` Christoffer Dall 2012-10-20 4:14 ` [PATCH 2/2] KVM: ARM: Defer parts of the vgic init until first KVM_RUN Christoffer Dall 1 sibling, 0 replies; 5+ messages in thread From: Christoffer Dall @ 2012-10-20 4:14 UTC (permalink / raw) To: kvmarm; +Cc: kvm, Christoffer Dall On ARM (and possibly other architectures) some bits are specific to the model being emulated for the guest and user space needs a way to tell the kernel about those bits. An example is mmio device base addresses, where KVM must know the base address for a given device to properly emulate mmio accesses within a certain address range or directly map a device with virtualiation extensions into the guest address space. We try to make this API slightly more generic than for our specific use, but so far only the VGIC uses this feature. Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com> --- Documentation/virtual/kvm/api.txt | 37 +++++++++++++++++++++++++++++++++++++ arch/arm/include/asm/kvm.h | 13 +++++++++++++ arch/arm/include/asm/kvm_mmu.h | 2 ++ arch/arm/include/asm/kvm_vgic.h | 6 ++++++ arch/arm/kvm/arm.c | 31 ++++++++++++++++++++++++++++++- arch/arm/kvm/vgic.c | 25 +++++++++++++++++++++++++ include/linux/kvm.h | 8 ++++++++ 7 files changed, 121 insertions(+), 1 deletion(-) diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index 0aa4d83..dae4f05 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -2102,6 +2102,43 @@ This ioctl returns the guest registers that are supported for the KVM_GET_ONE_REG/KVM_SET_ONE_REG calls. +4.80 KVM_SET_DEVICE_ADDRESS + +Capability: KVM_CAP_SET_DEVICE_ADDRESS +Architectures: arm +Type: vm ioctl +Parameters: struct kvm_device_address (in) +Returns: 0 on success, -1 on error +Errors: + ENODEV: The device id is unknown + ENXIO: Device not supported on current system + EEXIST: Address already set + E2BIG: Address outside guest physical address space + +struct kvm_device_address { + __u32 id; + __u64 addr; +}; + +Specify a device address in the guest's physical address space where guests +can access emulated or directly exposed devices, which the host kernel needs +to know about. The id field is an architecture specific identifier for a +specific device. + +ARM divides the id field into two parts, a device id and an address type id +specific to the individual device. + + bits: | 31 ... 16 | 15 ... 0 | + field: | device id | addr type id | + +ARM currently only require this when using the in-kernel GIC support for the +hardware vGIC features, using KVM_ARM_DEVICE_VGIC_V2 as the device id. When +setting the base address for the guest's mapping of the vGIC virtual CPU +and distributor interface, the ioctl 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. + + 5. The kvm_run structure ------------------------ diff --git a/arch/arm/include/asm/kvm.h b/arch/arm/include/asm/kvm.h index fb41608..a7ae073 100644 --- a/arch/arm/include/asm/kvm.h +++ b/arch/arm/include/asm/kvm.h @@ -42,6 +42,19 @@ struct kvm_regs { #define KVM_ARM_TARGET_CORTEX_A15 0 #define KVM_ARM_NUM_TARGETS 1 +/* KVM_SET_DEVICE_ADDRESS ioctl id encoding */ +#define KVM_DEVICE_TYPE_SHIFT 0 +#define KVM_DEVICE_TYPE_MASK (0xffff << KVM_DEVICE_TYPE_SHIFT) +#define KVM_DEVICE_ID_SHIFT 16 +#define KVM_DEVICE_ID_MASK (0xffff << KVM_DEVICE_ID_SHIFT) + +/* Supported device IDs */ +#define KVM_ARM_DEVICE_VGIC_V2 0 + +/* Supported VGIC address types */ +#define KVM_VGIC_V2_ADDR_TYPE_DIST 0 +#define KVM_VGIC_V2_ADDR_TYPE_CPU 1 + struct kvm_vcpu_init { __u32 target; __u32 features[7]; diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h index 9bd0508..0800531 100644 --- a/arch/arm/include/asm/kvm_mmu.h +++ b/arch/arm/include/asm/kvm_mmu.h @@ -26,6 +26,8 @@ * To save a bit of memory and to avoid alignment issues we assume 39-bit IPA * for now, but remember that the level-1 table must be aligned to its size. */ +#define KVM_PHYS_SHIFT (38) +#define KVM_PHYS_MASK ((1ULL << KVM_PHYS_SHIFT) - 1) #define PTRS_PER_PGD2 512 #define PGD2_ORDER get_order(PTRS_PER_PGD2 * sizeof(pgd_t)) diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h index 588c637..a688132 100644 --- a/arch/arm/include/asm/kvm_vgic.h +++ b/arch/arm/include/asm/kvm_vgic.h @@ -242,6 +242,7 @@ struct kvm_exit_mmio; #ifdef CONFIG_KVM_ARM_VGIC int kvm_vgic_hyp_init(void); +int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr); int kvm_vgic_init(struct kvm *kvm); void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu); void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu); @@ -261,6 +262,11 @@ static inline int kvm_vgic_hyp_init(void) return 0; } +static inline int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr) +{ + return 0; +} + static inline int kvm_vgic_init(struct kvm *kvm) { return 0; diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index d552b94..282794e 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -206,6 +206,9 @@ int kvm_dev_ioctl_check_extension(long ext) case KVM_CAP_COALESCED_MMIO: r = KVM_COALESCED_MMIO_PAGE_OFFSET; break; + case KVM_CAP_SET_DEVICE_ADDR: + r = 1; + break; default: r = 0; break; @@ -858,20 +861,46 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log) return -EINVAL; } +static int kvm_vm_ioctl_set_device_address(struct kvm *kvm, + struct kvm_device_address *dev_addr) +{ + unsigned long dev_id, type; + + dev_id = (dev_addr->id & KVM_DEVICE_ID_MASK) >> KVM_DEVICE_ID_SHIFT; + type = (dev_addr->id & KVM_DEVICE_TYPE_MASK) >> KVM_DEVICE_TYPE_SHIFT; + + switch (dev_id) { + case KVM_ARM_DEVICE_VGIC_V2: + if (!vgic_present) + return -ENXIO; + return kvm_vgic_set_addr(kvm, type, dev_addr->addr); + default: + return -ENODEV; + } +} + long kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg) { + struct kvm *kvm = filp->private_data; + void __user *argp = (void __user *)arg; switch (ioctl) { #ifdef CONFIG_KVM_ARM_VGIC case KVM_CREATE_IRQCHIP: { - struct kvm *kvm = filp->private_data; if (vgic_present) return kvm_vgic_init(kvm); else return -EINVAL; } #endif + case KVM_SET_DEVICE_ADDRESS: { + struct kvm_device_address dev_addr; + + if (copy_from_user(&dev_addr, argp, sizeof(dev_addr))) + return -EFAULT; + return kvm_vm_ioctl_set_device_address(kvm, &dev_addr); + } default: return -EINVAL; } diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c index b669b85..d63b7f8 100644 --- a/arch/arm/kvm/vgic.c +++ b/arch/arm/kvm/vgic.c @@ -1140,3 +1140,28 @@ out: return ret; } + +int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr) +{ + int r = 0; + + if (addr & ~KVM_PHYS_MASK) + return -E2BIG; + + mutex_lock(&kvm->lock); + switch (type) { + case KVM_VGIC_V2_ADDR_TYPE_DIST: + if (addr != VGIC_DIST_BASE) + return -EINVAL; + break; + case KVM_VGIC_V2_ADDR_TYPE_CPU: + if (addr != VGIC_CPU_BASE) + return -EINVAL; + break; + default: + r = -ENODEV; + } + + mutex_unlock(&kvm->lock); + return r; +} diff --git a/include/linux/kvm.h b/include/linux/kvm.h index 172cc10..72d5594 100644 --- a/include/linux/kvm.h +++ b/include/linux/kvm.h @@ -626,6 +626,7 @@ struct kvm_ppc_smmu_info { #ifdef __KVM_HAVE_READONLY_MEM #define KVM_CAP_READONLY_MEM 81 #endif +#define KVM_CAP_SET_DEVICE_ADDR 82 #ifdef KVM_CAP_IRQ_ROUTING @@ -764,6 +765,11 @@ struct kvm_msi { __u8 pad[16]; }; +struct kvm_device_address { + __u32 id; + __u64 addr; +}; + /* * ioctls for VM fds */ @@ -844,6 +850,8 @@ struct kvm_s390_ucas_mapping { #define KVM_PPC_GET_SMMU_INFO _IOR(KVMIO, 0xa6, struct kvm_ppc_smmu_info) /* Available with KVM_CAP_PPC_ALLOC_HTAB */ #define KVM_PPC_ALLOCATE_HTAB _IOWR(KVMIO, 0xa7, __u32) +/* Available with KVM_CAP_SET_DEVICE_ADDR */ +#define KVM_SET_DEVICE_ADDRESS _IOW(KVMIO, 0xa8, struct kvm_device_address) /* * ioctls for vcpu fds -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] KVM: ARM: Defer parts of the vgic init until first KVM_RUN 2012-10-20 4:14 [PATCH 0/2] KVM: ARM: Get rid of hardcoded VGIC addresses Christoffer Dall 2012-10-20 4:14 ` [PATCH 1/2] KVM: ARM: Introduce KVM_SET_DEVICE_ADDRESS ioctl Christoffer Dall @ 2012-10-20 4:14 ` Christoffer Dall 2012-10-20 10:27 ` [kvmarm] " Marc Zyngier 1 sibling, 1 reply; 5+ messages in thread From: Christoffer Dall @ 2012-10-20 4:14 UTC (permalink / raw) To: kvmarm; +Cc: kvm, Christoffer Dall The vgic virtual cpu and emulated distributor interfaces must be mapped at a given physical address in the guest. This address is provided through the KVM_SET_DEVICE_ADDRESS ioctl, which happens after the KVM_CREATE_IRQCHIP ioctl is called, but before the first VCPU is excuted thorugh KVM_RUN. We create the vgic on KVM_CREATE_IRQCHIP, but query kvm_vgic_ready(kvm), which checks if the vgic.vctrl_base field has been set, before we execute a VCPU, and if it has not been set, we call kvm_vgic_init, which takes care of the remaining setup. We use the IS_VGIC_ADDR_UNDEF() macro, which compares to the VGIC_ADDR_UNDEF constant, to check if an address has been set; it's unlikely that a device will sit on address 0, but since this is a part of main kernel boot procedure if this stuff is enabled in the config, I'm being paranoid. The distributor and vcpu base addresses used to be a per-host setting global for all VMs, but this is not a requirement and when we want to emulate several boards on a single host, we need the flexibility of storing these guest addresses on a per-VM basis. Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com> --- arch/arm/include/asm/kvm_vgic.h | 21 ++++++++-- arch/arm/kvm/arm.c | 10 ++++- arch/arm/kvm/vgic.c | 82 +++++++++++++++++++++++++++------------ 3 files changed, 84 insertions(+), 29 deletions(-) diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h index a688132..2de167f 100644 --- a/arch/arm/include/asm/kvm_vgic.h +++ b/arch/arm/include/asm/kvm_vgic.h @@ -154,13 +154,14 @@ static inline void vgic_bytemap_set_irq_val(struct vgic_bytemap *x, struct vgic_dist { #ifdef CONFIG_KVM_ARM_VGIC spinlock_t lock; + bool ready; /* Virtual control interface mapping */ void __iomem *vctrl_base; - /* Distributor mapping in the guest */ - unsigned long vgic_dist_base; - unsigned long vgic_dist_size; + /* Distributor and vcpu interface mapping in the guest */ + phys_addr_t vgic_dist_base; + phys_addr_t vgic_cpu_base; /* Distributor enabled */ u32 enabled; @@ -243,6 +244,7 @@ struct kvm_exit_mmio; #ifdef CONFIG_KVM_ARM_VGIC int kvm_vgic_hyp_init(void); int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr); +int kvm_vgic_create(struct kvm *kvm); int kvm_vgic_init(struct kvm *kvm); void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu); void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu); @@ -252,8 +254,9 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num, int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu); bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run, struct kvm_exit_mmio *mmio); +bool irqchip_in_kernel(struct kvm *kvm); -#define irqchip_in_kernel(k) (!!((k)->arch.vgic.vctrl_base)) +#define vgic_initialized(k) ((k)->arch.vgic.ready) #define vgic_active_irq(v) (atomic_read(&(v)->arch.vgic_cpu.irq_active_count) == 0) #else @@ -267,6 +270,11 @@ static inline int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 add return 0; } +static inline int kvm_vgic_create(struct kvm *kvm) +{ + return 0; +} + static inline int kvm_vgic_init(struct kvm *kvm) { return 0; @@ -298,6 +306,11 @@ static inline int irqchip_in_kernel(struct kvm *kvm) return 0; } +static inline bool kvm_vgic_initialized(struct kvm *kvm) +{ + return true; +} + static inline int vgic_active_irq(struct kvm_vcpu *vcpu) { return 0; diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index 282794e..d64783e 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -636,6 +636,14 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) if (unlikely(vcpu->arch.target < 0)) return -ENOEXEC; + /* Initalize the VGIC before running the vcpu */ + if (unlikely(irqchip_in_kernel(vcpu->kvm) && + !vgic_initialized(vcpu->kvm))) { + ret = kvm_vgic_init(vcpu->kvm); + if (ret) + return ret; + } + if (run->exit_reason == KVM_EXIT_MMIO) { ret = kvm_handle_mmio_return(vcpu, vcpu->run); if (ret) @@ -889,7 +897,7 @@ long kvm_arch_vm_ioctl(struct file *filp, #ifdef CONFIG_KVM_ARM_VGIC case KVM_CREATE_IRQCHIP: { if (vgic_present) - return kvm_vgic_init(kvm); + return kvm_vgic_create(kvm); else return -EINVAL; } diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c index d63b7f8..fa591db 100644 --- a/arch/arm/kvm/vgic.c +++ b/arch/arm/kvm/vgic.c @@ -65,12 +65,17 @@ * interrupt line to be sampled again. */ -/* Temporary hacks, need to be provided by userspace emulation */ -#define VGIC_DIST_BASE 0x2c001000 +#define VGIC_ADDR_UNDEF (-1) +#define IS_VGIC_ADDR_UNDEF(_x) ((_x) == (typeof(_x))VGIC_ADDR_UNDEF) + + #define VGIC_DIST_SIZE 0x1000 #define VGIC_CPU_BASE 0x2c002000 #define VGIC_CPU_SIZE 0x2000 +/* Physical address of vgic virtual cpu interface */ +static phys_addr_t vgic_vcpu_base; + /* Virtual control interface base address */ static void __iomem *vgic_vctrl_base; @@ -538,7 +543,7 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run, struct kvm_exi if (!irqchip_in_kernel(vcpu->kvm) || mmio->phys_addr < base || - (mmio->phys_addr + mmio->len) > (base + dist->vgic_dist_size)) + (mmio->phys_addr + mmio->len) > (base + VGIC_DIST_SIZE)) return false; range = find_matching_range(vgic_ranges, mmio, base); @@ -1027,7 +1032,7 @@ void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu) vgic_cpu->vgic_irq_lr_map[i] = LR_EMPTY; } - BUG_ON(!vcpu->kvm->arch.vgic.vctrl_base); + BUG_ON(IS_VGIC_ADDR_UNDEF(vcpu->kvm->arch.vgic.vctrl_base)); reg = readl_relaxed(vcpu->kvm->arch.vgic.vctrl_base + GICH_VTR); vgic_cpu->nr_lr = (reg & 0x1f) + 1; @@ -1101,29 +1106,23 @@ out_free_irq: int kvm_vgic_init(struct kvm *kvm) { - int ret, i; - struct resource vcpu_res; + int ret = 0, i; mutex_lock(&kvm->lock); - if (of_address_to_resource(vgic_node, 3, &vcpu_res)) { - kvm_err("Cannot obtain VCPU resource\n"); - ret = -ENXIO; + if (vgic_initialized(kvm)) goto out; - } - if (atomic_read(&kvm->online_vcpus) || kvm->arch.vgic.vctrl_base) { - ret = -EEXIST; + if (IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_dist_base) || + IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_cpu_base)) { + kvm_err("Need to set vgic cpu and dist addresses first\n"); + ret = -ENXIO; goto out; } - spin_lock_init(&kvm->arch.vgic.lock); - kvm->arch.vgic.vctrl_base = vgic_vctrl_base; - kvm->arch.vgic.vgic_dist_base = VGIC_DIST_BASE; - kvm->arch.vgic.vgic_dist_size = VGIC_DIST_SIZE; + ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base, + vgic_vcpu_base, VGIC_CPU_SIZE); - ret = kvm_phys_addr_ioremap(kvm, VGIC_CPU_BASE, - vcpu_res.start, VGIC_CPU_SIZE); if (ret) { kvm_err("Unable to remap VGIC CPU to VCPU\n"); goto out; @@ -1132,12 +1131,45 @@ int kvm_vgic_init(struct kvm *kvm) for (i = 32; i < VGIC_NR_IRQS; i += 4) vgic_set_target_reg(kvm, 0, i); + kvm_timer_init(kvm); + kvm->arch.vgic.ready = true; out: mutex_unlock(&kvm->lock); + return ret; +} - if (!ret) - kvm_timer_init(kvm); +bool irqchip_in_kernel(struct kvm *kvm) +{ + return !(IS_VGIC_ADDR_UNDEF(vgic_vcpu_base)); +} +int kvm_vgic_create(struct kvm *kvm) +{ + int ret; + struct resource vcpu_res; + + mutex_lock(&kvm->lock); + + if (of_address_to_resource(vgic_node, 3, &vcpu_res)) { + kvm_err("Cannot obtain VCPU resource\n"); + ret = -ENXIO; + goto out; + } + + if (atomic_read(&kvm->online_vcpus) || kvm->arch.vgic.vctrl_base) { + ret = -EEXIST; + 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; + kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF; + + vgic_vcpu_base = vcpu_res.start; + ret = 0; +out: + mutex_unlock(&kvm->lock); return ret; } @@ -1151,12 +1183,14 @@ int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr) mutex_lock(&kvm->lock); switch (type) { case KVM_VGIC_V2_ADDR_TYPE_DIST: - if (addr != VGIC_DIST_BASE) - return -EINVAL; + if (!IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_dist_base)) + return -EEXIST; + kvm->arch.vgic.vgic_dist_base = addr; break; case KVM_VGIC_V2_ADDR_TYPE_CPU: - if (addr != VGIC_CPU_BASE) - return -EINVAL; + if (!IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_cpu_base)) + return -EEXIST; + kvm->arch.vgic.vgic_cpu_base = addr; break; default: r = -ENODEV; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [kvmarm] [PATCH 2/2] KVM: ARM: Defer parts of the vgic init until first KVM_RUN 2012-10-20 4:14 ` [PATCH 2/2] KVM: ARM: Defer parts of the vgic init until first KVM_RUN Christoffer Dall @ 2012-10-20 10:27 ` Marc Zyngier 2012-10-20 13:29 ` Christoffer Dall 0 siblings, 1 reply; 5+ messages in thread From: Marc Zyngier @ 2012-10-20 10:27 UTC (permalink / raw) To: Christoffer Dall; +Cc: kvmarm, kvm On Sat, 20 Oct 2012 00:14:42 -0400, Christoffer Dall <c.dall@virtualopensystems.com> wrote: > The vgic virtual cpu and emulated distributor interfaces must > be mapped at a given physical address in the guest. This address is > provided through the KVM_SET_DEVICE_ADDRESS ioctl, which happens after > the KVM_CREATE_IRQCHIP ioctl is called, but before the first VCPU is > excuted thorugh KVM_RUN. We create the vgic on KVM_CREATE_IRQCHIP, but > query kvm_vgic_ready(kvm), which checks if the vgic.vctrl_base field has > been set, before we execute a VCPU, and if it has not been set, we call > kvm_vgic_init, which takes care of the remaining setup. > > We use the IS_VGIC_ADDR_UNDEF() macro, which compares to the > VGIC_ADDR_UNDEF constant, to check if an address has been set; it's > unlikely that a device will sit on address 0, but since this is a part > of main kernel boot procedure if this stuff is enabled in the config, > I'm being paranoid. > > The distributor and vcpu base addresses used to be a per-host setting > global for all VMs, but this is not a requirement and when we want to > emulate several boards on a single host, we need the flexibility of > storing these guest addresses on a per-VM basis. > > Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com> > --- > arch/arm/include/asm/kvm_vgic.h | 21 ++++++++-- > arch/arm/kvm/arm.c | 10 ++++- > arch/arm/kvm/vgic.c | 82 > +++++++++++++++++++++++++++------------ > 3 files changed, 84 insertions(+), 29 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_vgic.h > b/arch/arm/include/asm/kvm_vgic.h > index a688132..2de167f 100644 > --- a/arch/arm/include/asm/kvm_vgic.h > +++ b/arch/arm/include/asm/kvm_vgic.h > @@ -154,13 +154,14 @@ static inline void vgic_bytemap_set_irq_val(struct > vgic_bytemap *x, > struct vgic_dist { > #ifdef CONFIG_KVM_ARM_VGIC > spinlock_t lock; > + bool ready; > > /* Virtual control interface mapping */ > void __iomem *vctrl_base; > > - /* Distributor mapping in the guest */ > - unsigned long vgic_dist_base; > - unsigned long vgic_dist_size; > + /* Distributor and vcpu interface mapping in the guest */ > + phys_addr_t vgic_dist_base; > + phys_addr_t vgic_cpu_base; > > /* Distributor enabled */ > u32 enabled; > @@ -243,6 +244,7 @@ struct kvm_exit_mmio; > #ifdef CONFIG_KVM_ARM_VGIC > int kvm_vgic_hyp_init(void); > int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr); > +int kvm_vgic_create(struct kvm *kvm); > int kvm_vgic_init(struct kvm *kvm); > void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu); > void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu); > @@ -252,8 +254,9 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, > unsigned int irq_num, > int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu); > bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run, > struct kvm_exit_mmio *mmio); > +bool irqchip_in_kernel(struct kvm *kvm); > > -#define irqchip_in_kernel(k) (!!((k)->arch.vgic.vctrl_base)) > +#define vgic_initialized(k) ((k)->arch.vgic.ready) > #define > vgic_active_irq(v) (atomic_read(&(v)->arch.vgic_cpu.irq_active_count) == > 0) > > #else > @@ -267,6 +270,11 @@ static inline int kvm_vgic_set_addr(struct kvm *kvm, > unsigned long type, u64 add > return 0; > } > > +static inline int kvm_vgic_create(struct kvm *kvm) > +{ > + return 0; > +} > + > static inline int kvm_vgic_init(struct kvm *kvm) > { > return 0; > @@ -298,6 +306,11 @@ static inline int irqchip_in_kernel(struct kvm *kvm) > return 0; > } > > +static inline bool kvm_vgic_initialized(struct kvm *kvm) > +{ > + return true; > +} > + > static inline int vgic_active_irq(struct kvm_vcpu *vcpu) > { > return 0; > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index 282794e..d64783e 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -636,6 +636,14 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, > struct kvm_run *run) > if (unlikely(vcpu->arch.target < 0)) > return -ENOEXEC; > > + /* Initalize the VGIC before running the vcpu */ > + if (unlikely(irqchip_in_kernel(vcpu->kvm) && > + !vgic_initialized(vcpu->kvm))) { > + ret = kvm_vgic_init(vcpu->kvm); > + if (ret) > + return ret; > + } > + > if (run->exit_reason == KVM_EXIT_MMIO) { > ret = kvm_handle_mmio_return(vcpu, vcpu->run); > if (ret) > @@ -889,7 +897,7 @@ long kvm_arch_vm_ioctl(struct file *filp, > #ifdef CONFIG_KVM_ARM_VGIC > case KVM_CREATE_IRQCHIP: { > if (vgic_present) > - return kvm_vgic_init(kvm); > + return kvm_vgic_create(kvm); > else > return -EINVAL; > } > diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c > index d63b7f8..fa591db 100644 > --- a/arch/arm/kvm/vgic.c > +++ b/arch/arm/kvm/vgic.c > @@ -65,12 +65,17 @@ > * interrupt line to be sampled again. > */ > > -/* Temporary hacks, need to be provided by userspace emulation */ > -#define VGIC_DIST_BASE 0x2c001000 > +#define VGIC_ADDR_UNDEF (-1) > +#define IS_VGIC_ADDR_UNDEF(_x) ((_x) == (typeof(_x))VGIC_ADDR_UNDEF) This is an awkward construct. You should really know the type of what you're checking. Consider: #define VGIC_ADDR_UNDEF ((phys_addr_t)(~0UL) #define IS_VGIC_ADDR_UNDEF(x) ((x) == VGIC_ADDR_UNDEF) > + > + > #define VGIC_DIST_SIZE 0x1000 > #define VGIC_CPU_BASE 0x2c002000 > #define VGIC_CPU_SIZE 0x2000 > > +/* Physical address of vgic virtual cpu interface */ > +static phys_addr_t vgic_vcpu_base; > + > /* Virtual control interface base address */ > static void __iomem *vgic_vctrl_base; > > @@ -538,7 +543,7 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct > kvm_run *run, struct kvm_exi > > if (!irqchip_in_kernel(vcpu->kvm) || > mmio->phys_addr < base || > - (mmio->phys_addr + mmio->len) > (base + dist->vgic_dist_size)) > + (mmio->phys_addr + mmio->len) > (base + VGIC_DIST_SIZE)) > return false; > > range = find_matching_range(vgic_ranges, mmio, base); > @@ -1027,7 +1032,7 @@ void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu) > vgic_cpu->vgic_irq_lr_map[i] = LR_EMPTY; > } > > - BUG_ON(!vcpu->kvm->arch.vgic.vctrl_base); > + BUG_ON(IS_VGIC_ADDR_UNDEF(vcpu->kvm->arch.vgic.vctrl_base)); > reg = readl_relaxed(vcpu->kvm->arch.vgic.vctrl_base + GICH_VTR); > vgic_cpu->nr_lr = (reg & 0x1f) + 1; > > @@ -1101,29 +1106,23 @@ out_free_irq: > > int kvm_vgic_init(struct kvm *kvm) > { > - int ret, i; > - struct resource vcpu_res; > + int ret = 0, i; > > mutex_lock(&kvm->lock); > > - if (of_address_to_resource(vgic_node, 3, &vcpu_res)) { > - kvm_err("Cannot obtain VCPU resource\n"); > - ret = -ENXIO; > + if (vgic_initialized(kvm)) > goto out; > - } > > - if (atomic_read(&kvm->online_vcpus) || kvm->arch.vgic.vctrl_base) { > - ret = -EEXIST; > + if (IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_dist_base) || > + IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_cpu_base)) { > + kvm_err("Need to set vgic cpu and dist addresses first\n"); > + ret = -ENXIO; > goto out; > } > > - spin_lock_init(&kvm->arch.vgic.lock); > - kvm->arch.vgic.vctrl_base = vgic_vctrl_base; > - kvm->arch.vgic.vgic_dist_base = VGIC_DIST_BASE; > - kvm->arch.vgic.vgic_dist_size = VGIC_DIST_SIZE; > + ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base, > + vgic_vcpu_base, VGIC_CPU_SIZE); > > - ret = kvm_phys_addr_ioremap(kvm, VGIC_CPU_BASE, > - vcpu_res.start, VGIC_CPU_SIZE); > if (ret) { > kvm_err("Unable to remap VGIC CPU to VCPU\n"); > goto out; > @@ -1132,12 +1131,45 @@ int kvm_vgic_init(struct kvm *kvm) > for (i = 32; i < VGIC_NR_IRQS; i += 4) > vgic_set_target_reg(kvm, 0, i); > > + kvm_timer_init(kvm); > + kvm->arch.vgic.ready = true; > out: > mutex_unlock(&kvm->lock); > + return ret; > +} > > - if (!ret) > - kvm_timer_init(kvm); > +bool irqchip_in_kernel(struct kvm *kvm) > +{ > + return !(IS_VGIC_ADDR_UNDEF(vgic_vcpu_base)); Erm... This check is supposed VM specific. Here, you check something that will be initialized once and for all when the first VM is run. Use one of the vgic *guest* base addresses instead. > +} > > +int kvm_vgic_create(struct kvm *kvm) > +{ > + int ret; > + struct resource vcpu_res; > + > + mutex_lock(&kvm->lock); > + > + if (of_address_to_resource(vgic_node, 3, &vcpu_res)) { > + kvm_err("Cannot obtain VCPU resource\n"); > + ret = -ENXIO; > + goto out; > + } This function is called on every VM creation. Consider moving this lookup and the vgic_vcpu_base assignment to kvm_vgic_hyp_init(). > + if (atomic_read(&kvm->online_vcpus) || kvm->arch.vgic.vctrl_base) { > + ret = -EEXIST; > + 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; > + kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF; > + > + vgic_vcpu_base = vcpu_res.start; > + ret = 0; > +out: > + mutex_unlock(&kvm->lock); > return ret; > } > > @@ -1151,12 +1183,14 @@ int kvm_vgic_set_addr(struct kvm *kvm, unsigned > long type, u64 addr) > mutex_lock(&kvm->lock); > switch (type) { > case KVM_VGIC_V2_ADDR_TYPE_DIST: > - if (addr != VGIC_DIST_BASE) > - return -EINVAL; > + if (!IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_dist_base)) > + return -EEXIST; > + kvm->arch.vgic.vgic_dist_base = addr; > break; > case KVM_VGIC_V2_ADDR_TYPE_CPU: > - if (addr != VGIC_CPU_BASE) > - return -EINVAL; > + if (!IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_cpu_base)) > + return -EEXIST; > + kvm->arch.vgic.vgic_cpu_base = addr; > break; > default: > r = -ENODEV; We need some additional checks here about the validity of the address (at least page aligned, and non overlapping with memory or other in-kernel device models). M. -- Fast, cheap, reliable. Pick two. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [kvmarm] [PATCH 2/2] KVM: ARM: Defer parts of the vgic init until first KVM_RUN 2012-10-20 10:27 ` [kvmarm] " Marc Zyngier @ 2012-10-20 13:29 ` Christoffer Dall 0 siblings, 0 replies; 5+ messages in thread From: Christoffer Dall @ 2012-10-20 13:29 UTC (permalink / raw) To: Marc Zyngier; +Cc: kvmarm, kvm On Sat, Oct 20, 2012 at 6:27 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On Sat, 20 Oct 2012 00:14:42 -0400, Christoffer Dall > <c.dall@virtualopensystems.com> wrote: >> The vgic virtual cpu and emulated distributor interfaces must >> be mapped at a given physical address in the guest. This address is >> provided through the KVM_SET_DEVICE_ADDRESS ioctl, which happens after >> the KVM_CREATE_IRQCHIP ioctl is called, but before the first VCPU is >> excuted thorugh KVM_RUN. We create the vgic on KVM_CREATE_IRQCHIP, but >> query kvm_vgic_ready(kvm), which checks if the vgic.vctrl_base field has >> been set, before we execute a VCPU, and if it has not been set, we call >> kvm_vgic_init, which takes care of the remaining setup. >> >> We use the IS_VGIC_ADDR_UNDEF() macro, which compares to the >> VGIC_ADDR_UNDEF constant, to check if an address has been set; it's >> unlikely that a device will sit on address 0, but since this is a part >> of main kernel boot procedure if this stuff is enabled in the config, >> I'm being paranoid. >> >> The distributor and vcpu base addresses used to be a per-host setting >> global for all VMs, but this is not a requirement and when we want to >> emulate several boards on a single host, we need the flexibility of >> storing these guest addresses on a per-VM basis. >> >> Signed-off-by: Christoffer Dall <c.dall@virtualopensystems.com> >> --- >> arch/arm/include/asm/kvm_vgic.h | 21 ++++++++-- >> arch/arm/kvm/arm.c | 10 ++++- >> arch/arm/kvm/vgic.c | 82 >> +++++++++++++++++++++++++++------------ >> 3 files changed, 84 insertions(+), 29 deletions(-) >> >> diff --git a/arch/arm/include/asm/kvm_vgic.h >> b/arch/arm/include/asm/kvm_vgic.h >> index a688132..2de167f 100644 >> --- a/arch/arm/include/asm/kvm_vgic.h >> +++ b/arch/arm/include/asm/kvm_vgic.h >> @@ -154,13 +154,14 @@ static inline void vgic_bytemap_set_irq_val(struct >> vgic_bytemap *x, >> struct vgic_dist { >> #ifdef CONFIG_KVM_ARM_VGIC >> spinlock_t lock; >> + bool ready; >> >> /* Virtual control interface mapping */ >> void __iomem *vctrl_base; >> >> - /* Distributor mapping in the guest */ >> - unsigned long vgic_dist_base; >> - unsigned long vgic_dist_size; >> + /* Distributor and vcpu interface mapping in the guest */ >> + phys_addr_t vgic_dist_base; >> + phys_addr_t vgic_cpu_base; >> >> /* Distributor enabled */ >> u32 enabled; >> @@ -243,6 +244,7 @@ struct kvm_exit_mmio; >> #ifdef CONFIG_KVM_ARM_VGIC >> int kvm_vgic_hyp_init(void); >> int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr); >> +int kvm_vgic_create(struct kvm *kvm); >> int kvm_vgic_init(struct kvm *kvm); >> void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu); >> void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu); >> @@ -252,8 +254,9 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, >> unsigned int irq_num, >> int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu); >> bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run, >> struct kvm_exit_mmio *mmio); >> +bool irqchip_in_kernel(struct kvm *kvm); >> >> -#define irqchip_in_kernel(k) (!!((k)->arch.vgic.vctrl_base)) >> +#define vgic_initialized(k) ((k)->arch.vgic.ready) >> #define >> vgic_active_irq(v) (atomic_read(&(v)->arch.vgic_cpu.irq_active_count) > == >> 0) >> >> #else >> @@ -267,6 +270,11 @@ static inline int kvm_vgic_set_addr(struct kvm > *kvm, >> unsigned long type, u64 add >> return 0; >> } >> >> +static inline int kvm_vgic_create(struct kvm *kvm) >> +{ >> + return 0; >> +} >> + >> static inline int kvm_vgic_init(struct kvm *kvm) >> { >> return 0; >> @@ -298,6 +306,11 @@ static inline int irqchip_in_kernel(struct kvm > *kvm) >> return 0; >> } >> >> +static inline bool kvm_vgic_initialized(struct kvm *kvm) >> +{ >> + return true; >> +} >> + >> static inline int vgic_active_irq(struct kvm_vcpu *vcpu) >> { >> return 0; >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c >> index 282794e..d64783e 100644 >> --- a/arch/arm/kvm/arm.c >> +++ b/arch/arm/kvm/arm.c >> @@ -636,6 +636,14 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, >> struct kvm_run *run) >> if (unlikely(vcpu->arch.target < 0)) >> return -ENOEXEC; >> >> + /* Initalize the VGIC before running the vcpu */ >> + if (unlikely(irqchip_in_kernel(vcpu->kvm) && >> + !vgic_initialized(vcpu->kvm))) { >> + ret = kvm_vgic_init(vcpu->kvm); >> + if (ret) >> + return ret; >> + } >> + >> if (run->exit_reason == KVM_EXIT_MMIO) { >> ret = kvm_handle_mmio_return(vcpu, vcpu->run); >> if (ret) >> @@ -889,7 +897,7 @@ long kvm_arch_vm_ioctl(struct file *filp, >> #ifdef CONFIG_KVM_ARM_VGIC >> case KVM_CREATE_IRQCHIP: { >> if (vgic_present) >> - return kvm_vgic_init(kvm); >> + return kvm_vgic_create(kvm); >> else >> return -EINVAL; >> } >> diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c >> index d63b7f8..fa591db 100644 >> --- a/arch/arm/kvm/vgic.c >> +++ b/arch/arm/kvm/vgic.c >> @@ -65,12 +65,17 @@ >> * interrupt line to be sampled again. >> */ >> >> -/* Temporary hacks, need to be provided by userspace emulation */ >> -#define VGIC_DIST_BASE 0x2c001000 >> +#define VGIC_ADDR_UNDEF (-1) >> +#define IS_VGIC_ADDR_UNDEF(_x) ((_x) == (typeof(_x))VGIC_ADDR_UNDEF) > > This is an awkward construct. You should really know the type of what > you're checking. Consider: I don't think it's awkward, but a pretty common construct. I can't know which type I check when it's used for both a pointer and a phys_addr_t. > > #define VGIC_ADDR_UNDEF ((phys_addr_t)(~0UL) > #define IS_VGIC_ADDR_UNDEF(x) ((x) == VGIC_ADDR_UNDEF) > >> + >> + >> #define VGIC_DIST_SIZE 0x1000 >> #define VGIC_CPU_BASE 0x2c002000 >> #define VGIC_CPU_SIZE 0x2000 >> >> +/* Physical address of vgic virtual cpu interface */ >> +static phys_addr_t vgic_vcpu_base; >> + >> /* Virtual control interface base address */ >> static void __iomem *vgic_vctrl_base; >> >> @@ -538,7 +543,7 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct >> kvm_run *run, struct kvm_exi >> >> if (!irqchip_in_kernel(vcpu->kvm) || >> mmio->phys_addr < base || >> - (mmio->phys_addr + mmio->len) > (base + dist->vgic_dist_size)) >> + (mmio->phys_addr + mmio->len) > (base + VGIC_DIST_SIZE)) >> return false; >> >> range = find_matching_range(vgic_ranges, mmio, base); >> @@ -1027,7 +1032,7 @@ void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu) >> vgic_cpu->vgic_irq_lr_map[i] = LR_EMPTY; >> } >> >> - BUG_ON(!vcpu->kvm->arch.vgic.vctrl_base); >> + BUG_ON(IS_VGIC_ADDR_UNDEF(vcpu->kvm->arch.vgic.vctrl_base)); >> reg = readl_relaxed(vcpu->kvm->arch.vgic.vctrl_base + GICH_VTR); >> vgic_cpu->nr_lr = (reg & 0x1f) + 1; >> >> @@ -1101,29 +1106,23 @@ out_free_irq: >> >> int kvm_vgic_init(struct kvm *kvm) >> { >> - int ret, i; >> - struct resource vcpu_res; >> + int ret = 0, i; >> >> mutex_lock(&kvm->lock); >> >> - if (of_address_to_resource(vgic_node, 3, &vcpu_res)) { >> - kvm_err("Cannot obtain VCPU resource\n"); >> - ret = -ENXIO; >> + if (vgic_initialized(kvm)) >> goto out; >> - } >> >> - if (atomic_read(&kvm->online_vcpus) || kvm->arch.vgic.vctrl_base) { >> - ret = -EEXIST; >> + if (IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_dist_base) || >> + IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_cpu_base)) { >> + kvm_err("Need to set vgic cpu and dist addresses first\n"); >> + ret = -ENXIO; >> goto out; >> } >> >> - spin_lock_init(&kvm->arch.vgic.lock); >> - kvm->arch.vgic.vctrl_base = vgic_vctrl_base; >> - kvm->arch.vgic.vgic_dist_base = VGIC_DIST_BASE; >> - kvm->arch.vgic.vgic_dist_size = VGIC_DIST_SIZE; >> + ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base, >> + vgic_vcpu_base, VGIC_CPU_SIZE); >> >> - ret = kvm_phys_addr_ioremap(kvm, VGIC_CPU_BASE, >> - vcpu_res.start, VGIC_CPU_SIZE); >> if (ret) { >> kvm_err("Unable to remap VGIC CPU to VCPU\n"); >> goto out; >> @@ -1132,12 +1131,45 @@ int kvm_vgic_init(struct kvm *kvm) >> for (i = 32; i < VGIC_NR_IRQS; i += 4) >> vgic_set_target_reg(kvm, 0, i); >> >> + kvm_timer_init(kvm); >> + kvm->arch.vgic.ready = true; >> out: >> mutex_unlock(&kvm->lock); >> + return ret; >> +} >> >> - if (!ret) >> - kvm_timer_init(kvm); >> +bool irqchip_in_kernel(struct kvm *kvm) >> +{ >> + return !(IS_VGIC_ADDR_UNDEF(vgic_vcpu_base)); > > Erm... This check is supposed VM specific. Here, you check something that > will be initialized once and for all when the first VM is run. Use one of > the vgic *guest* base addresses instead. > Ouch, right you are. (This whole scheme of checking various base addresses for whether something is initialized or not seems quite brittle, but I don't want to rework it before a merge) >> +} >> >> +int kvm_vgic_create(struct kvm *kvm) >> +{ >> + int ret; >> + struct resource vcpu_res; >> + >> + mutex_lock(&kvm->lock); >> + >> + if (of_address_to_resource(vgic_node, 3, &vcpu_res)) { >> + kvm_err("Cannot obtain VCPU resource\n"); >> + ret = -ENXIO; >> + goto out; >> + } > > This function is called on every VM creation. Consider moving this lookup > and the vgic_vcpu_base assignment to kvm_vgic_hyp_init(). > >> + if (atomic_read(&kvm->online_vcpus) || kvm->arch.vgic.vctrl_base) { >> + ret = -EEXIST; >> + 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; >> + kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF; >> + >> + vgic_vcpu_base = vcpu_res.start; >> + ret = 0; >> +out: >> + mutex_unlock(&kvm->lock); >> return ret; >> } >> >> @@ -1151,12 +1183,14 @@ int kvm_vgic_set_addr(struct kvm *kvm, unsigned >> long type, u64 addr) >> mutex_lock(&kvm->lock); >> switch (type) { >> case KVM_VGIC_V2_ADDR_TYPE_DIST: >> - if (addr != VGIC_DIST_BASE) >> - return -EINVAL; >> + if (!IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_dist_base)) >> + return -EEXIST; >> + kvm->arch.vgic.vgic_dist_base = addr; >> break; >> case KVM_VGIC_V2_ADDR_TYPE_CPU: >> - if (addr != VGIC_CPU_BASE) >> - return -EINVAL; >> + if (!IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_cpu_base)) >> + return -EEXIST; >> + kvm->arch.vgic.vgic_cpu_base = addr; >> break; >> default: >> r = -ENODEV; > > We need some additional checks here about the validity of the address (at > least page aligned, and non overlapping with memory or other in-kernel > device models). > since we don't have any other in-kernel device models that are memory mapped, we should cross that bridge when/if we get there. It will not affect stability of the host kernel, merely not properly boot a guest if you supply bogus overlapping io regions. See patch diff: diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h index 2de167f..3c71c79 100644 --- a/arch/arm/include/asm/kvm_vgic.h +++ b/arch/arm/include/asm/kvm_vgic.h @@ -256,6 +256,7 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run, struct kvm_exit_mmio *mmio); bool irqchip_in_kernel(struct kvm *kvm); +#define irqchip_in_kernel(k) ((k)->arch.vgic.vctrl_base) #define vgic_initialized(k) ((k)->arch.vgic.ready) #define vgic_active_irq(v) (atomic_read(&(v)->arch.vgic_cpu.irq_active_count) == 0) diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c index 0ab098e..e5ace0e 100644 --- a/arch/arm/kvm/mmu.c +++ b/arch/arm/kvm/mmu.c @@ -422,7 +422,7 @@ static void stage2_clear_pte(struct kvm *kvm, phys_addr_t addr) } static void stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache, - phys_addr_t addr, const pte_t *new_pte) + phys_addr_t addr, const pte_t *new_pte, bool iomap) { pgd_t *pgd; pud_t *pud; @@ -454,6 +454,9 @@ static void stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache, } else pte = pte_offset_kernel(pmd, addr); + if (iomap && pte_present(old_pte)) + return -EFAULT; + /* Create 2nd stage page table mapping - Level 3 */ old_pte = *pte; set_pte_ext(pte, *new_pte, 0); @@ -489,7 +492,7 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa, if (ret) goto out; spin_lock(&kvm->mmu_lock); - stage2_set_pte(kvm, &cache, addr, &pte); + stage2_set_pte(kvm, &cache, addr, &pte, true); spin_unlock(&kvm->mmu_lock); pfn++; @@ -565,7 +568,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, pte_val(new_pte) |= L_PTE_S2_RDWR; kvm_set_pfn_dirty(pfn); } - stage2_set_pte(vcpu->kvm, memcache, fault_ipa, &new_pte); + stage2_set_pte(vcpu->kvm, memcache, fault_ipa, &new_pte, false); out_unlock: spin_unlock(&vcpu->kvm->mmu_lock); @@ -716,7 +719,7 @@ static void kvm_set_spte_handler(struct kvm *kvm, gpa_t gpa, void *data) { pte_t *pte = (pte_t *)data; - stage2_set_pte(kvm, NULL, gpa, pte); + stage2_set_pte(kvm, NULL, gpa, pte, false); } diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c index fa591db..8fe0e70 100644 --- a/arch/arm/kvm/vgic.c +++ b/arch/arm/kvm/vgic.c @@ -1054,6 +1054,7 @@ int kvm_vgic_hyp_init(void) int ret; unsigned int irq; struct resource vctrl_res; + struct resource vcpu_res; vgic_node = of_find_compatible_node(NULL, NULL, "arm,cortex-a15-gic"); if (!vgic_node) @@ -1094,6 +1095,13 @@ int kvm_vgic_hyp_init(void) kvm_info("%s@%llx IRQ%d\n", vgic_node->name, vctrl_res.start, irq); on_each_cpu(vgic_init_maintenance_interrupt, &irq, 1); + if (of_address_to_resource(vgic_node, 3, &vcpu_res)) { + kvm_err("Cannot obtain VCPU resource\n"); + ret = -ENXIO; + goto out_unmap; + } + vgic_vcpu_base = vcpu_res.start; + return 0; out_unmap: @@ -1138,24 +1146,12 @@ out: return ret; } -bool irqchip_in_kernel(struct kvm *kvm) -{ - return !(IS_VGIC_ADDR_UNDEF(vgic_vcpu_base)); -} - int kvm_vgic_create(struct kvm *kvm) { int ret; - struct resource vcpu_res; mutex_lock(&kvm->lock); - if (of_address_to_resource(vgic_node, 3, &vcpu_res)) { - kvm_err("Cannot obtain VCPU resource\n"); - ret = -ENXIO; - goto out; - } - if (atomic_read(&kvm->online_vcpus) || kvm->arch.vgic.vctrl_base) { ret = -EEXIST; goto out; @@ -1166,36 +1162,62 @@ int kvm_vgic_create(struct kvm *kvm) kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF; kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF; - vgic_vcpu_base = vcpu_res.start; ret = 0; out: mutex_unlock(&kvm->lock); return ret; } +static bool vgic_ioaddr_overlap(struct kvm *kvm) +{ + phys_addr_t dist = kvm->arch.vgic.vgic_dist_base; + phys_addr_t cpu = kvm->arch.vgic.vgic_cpu_base; + + if (IS_VGIC_ADDR_UNDEF(dist) || IS_VGIC_ADDR_UNDEF(cpu)) + return false; + if ((dist <= cpu && dist + VGIC_DIST_SIZE > cpu) || + (cpu <= dist && cpu + VGIC_CPU_SIZE > dist)) + return true; + return false; +} + int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr) { int r = 0; + struct vgic_dist *vgic = &kvm->arch.vgic; if (addr & ~KVM_PHYS_MASK) return -E2BIG; + if (addr & ~PAGE_MASK) + return -EINVAL; + mutex_lock(&kvm->lock); switch (type) { case KVM_VGIC_V2_ADDR_TYPE_DIST: - if (!IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_dist_base)) + if (!IS_VGIC_ADDR_UNDEF(vgic->vgic_dist_base)) return -EEXIST; + if (addr + VGIC_DIST_SIZE < addr) + return -EINVAL; kvm->arch.vgic.vgic_dist_base = addr; break; case KVM_VGIC_V2_ADDR_TYPE_CPU: - if (!IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_cpu_base)) + if (!IS_VGIC_ADDR_UNDEF(vgic->vgic_cpu_base)) return -EEXIST; + if (addr + VGIC_CPU_SIZE < addr) + return -EINVAL; kvm->arch.vgic.vgic_cpu_base = addr; break; default: r = -ENODEV; } + if (vgic_ioaddr_overlap(kvm)) { + kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF; + kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF; + return -EINVAL; + } + mutex_unlock(&kvm->lock); return r; } ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-10-20 13:29 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-20 4:14 [PATCH 0/2] KVM: ARM: Get rid of hardcoded VGIC addresses Christoffer Dall 2012-10-20 4:14 ` [PATCH 1/2] KVM: ARM: Introduce KVM_SET_DEVICE_ADDRESS ioctl Christoffer Dall 2012-10-20 4:14 ` [PATCH 2/2] KVM: ARM: Defer parts of the vgic init until first KVM_RUN Christoffer Dall 2012-10-20 10:27 ` [kvmarm] " Marc Zyngier 2012-10-20 13:29 ` Christoffer Dall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).