On Mon, May 14, 2012 at 9:06 AM, Marc Zyngier <marc.zyngier at arm.com> wrote:
> Change the vgic_ranges table to be relative to a per VM
> vgic_dist_base. It makes it faster to bail out if the
> access is out of range, and makes it possible to have multiple
> VMs using different GIC base addresses.
>
> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
> ---
> ?arch/arm/include/asm/kvm_vgic.h | ? ?2 +
> ?arch/arm/kvm/vgic.c ? ? ? ? ? ? | ? 49 +++++++++++++++++++++++---------------
> ?2 files changed, 32 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h
> index 354e528..4e7f6c3 100644
> --- a/arch/arm/include/asm/kvm_vgic.h
> +++ b/arch/arm/include/asm/kvm_vgic.h
> @@ -85,6 +85,8 @@ struct vgic_dist {
> ? ? ? ?spinlock_t ? ? ? ? ? ? ?lock;
>
> ? ? ? ?void __iomem ? ? ? ? ? ?*vctrl_base;
> + ? ? ? unsigned long ? ? ? ? ? vgic_dist_base;
> + ? ? ? unsigned long ? ? ? ? ? vgic_dist_size;
>
> ? ? ? ?u32 ? ? ? ? ? ? ? ? ? ? enabled;
>
> diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
> index f26b091..1b4f6b7 100644
> --- a/arch/arm/kvm/vgic.c
> +++ b/arch/arm/kvm/vgic.c
> @@ -290,62 +290,62 @@ struct mmio_range {
>
> ?static const struct mmio_range vgic_ranges[] = {
> ? ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? /* CTRL, TYPER, IIDR */
> - ? ? ? ? ? ? ? .base ? ? ? ? ? = VGIC_DIST_BASE,
> + ? ? ? ? ? ? ? .base ? ? ? ? ? = 0,
nit: field could be named offset of base_offset instead
> ? ? ? ? ? ? ? ?.len ? ? ? ? ? ?= 12,
> ? ? ? ? ? ? ? ?.handle_mmio ? ?= handle_mmio_misc,
> ? ? ? ?},
> ? ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? /* IGROUPRn */
> - ? ? ? ? ? ? ? .base ? ? ? ? ? = VGIC_DIST_BASE + 0x80,
> + ? ? ? ? ? ? ? .base ? ? ? ? ? = 0x80,
> ? ? ? ? ? ? ? ?.len ? ? ? ? ? ?= VGIC_NR_IRQS / 8,
> ? ? ? ? ? ? ? ?.handle_mmio ? ?= handle_mmio_group_reg,
> ? ? ? ?},
> ? ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? /* ISENABLERn */
> - ? ? ? ? ? ? ? .base ? ? ? ? ? = VGIC_DIST_BASE + 0x100,
> + ? ? ? ? ? ? ? .base ? ? ? ? ? = 0x100,
> ? ? ? ? ? ? ? ?.len ? ? ? ? ? ?= VGIC_NR_IRQS / 8,
> ? ? ? ? ? ? ? ?.handle_mmio ? ?= handle_mmio_set_enable_reg,
> ? ? ? ?},
> ? ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? /* ICENABLERn */
> - ? ? ? ? ? ? ? .base ? ? ? ? ? = VGIC_DIST_BASE + 0x180,
> + ? ? ? ? ? ? ? .base ? ? ? ? ? = 0x180,
> ? ? ? ? ? ? ? ?.len ? ? ? ? ? ?= VGIC_NR_IRQS / 8,
> ? ? ? ? ? ? ? ?.handle_mmio ? ?= handle_mmio_clear_enable_reg,
> ? ? ? ?},
> ? ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? /* ISPENDRn */
> - ? ? ? ? ? ? ? .base ? ? ? ? ? = VGIC_DIST_BASE + 0x200,
> + ? ? ? ? ? ? ? .base ? ? ? ? ? = 0x200,
> ? ? ? ? ? ? ? ?.len ? ? ? ? ? ?= VGIC_NR_IRQS / 8,
> ? ? ? ? ? ? ? ?.handle_mmio ? ?= handle_mmio_set_pending_reg,
> ? ? ? ?},
> ? ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? /* ICPENDRn */
> - ? ? ? ? ? ? ? .base ? ? ? ? ? = VGIC_DIST_BASE + 0x280,
> + ? ? ? ? ? ? ? .base ? ? ? ? ? = 0x280,
> ? ? ? ? ? ? ? ?.len ? ? ? ? ? ?= VGIC_NR_IRQS / 8,
> ? ? ? ? ? ? ? ?.handle_mmio ? ?= handle_mmio_clear_pending_reg,
> ? ? ? ?},
> ? ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? /* ISACTIVERn */
> - ? ? ? ? ? ? ? .base ? ? ? ? ? = VGIC_DIST_BASE + 0x300,
> + ? ? ? ? ? ? ? .base ? ? ? ? ? = 0x300,
> ? ? ? ? ? ? ? ?.len ? ? ? ? ? ?= VGIC_NR_IRQS / 8,
> ? ? ? ? ? ? ? ?.handle_mmio ? ?= handle_mmio_set_active_reg,
> ? ? ? ?},
> ? ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? /* ICACTIVERn */
> - ? ? ? ? ? ? ? .base ? ? ? ? ? = VGIC_DIST_BASE + 0x380,
> + ? ? ? ? ? ? ? .base ? ? ? ? ? = 0x380,
> ? ? ? ? ? ? ? ?.len ? ? ? ? ? ?= VGIC_NR_IRQS / 8,
> ? ? ? ? ? ? ? ?.handle_mmio ? ?= handle_mmio_clear_active_reg,
> ? ? ? ?},
> ? ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? /* IPRIORITYRn */
> - ? ? ? ? ? ? ? .base ? ? ? ? ? = VGIC_DIST_BASE + 0x400,
> + ? ? ? ? ? ? ? .base ? ? ? ? ? = 0x400,
> ? ? ? ? ? ? ? ?.len ? ? ? ? ? ?= VGIC_NR_IRQS,
> ? ? ? ? ? ? ? ?.handle_mmio ? ?= handle_mmio_priority_reg,
> ? ? ? ?},
> ? ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? /* ITARGETSRn */
> - ? ? ? ? ? ? ? .base ? ? ? ? ? = VGIC_DIST_BASE + 0x800,
> + ? ? ? ? ? ? ? .base ? ? ? ? ? = 0x800,
> ? ? ? ? ? ? ? ?.len ? ? ? ? ? ?= VGIC_NR_IRQS,
> ? ? ? ? ? ? ? ?.handle_mmio ? ?= handle_mmio_target_reg,
> ? ? ? ?},
> ? ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? /* ICFGRn */
> - ? ? ? ? ? ? ? .base ? ? ? ? ? = VGIC_DIST_BASE + 0xC00,
> + ? ? ? ? ? ? ? .base ? ? ? ? ? = 0xC00,
> ? ? ? ? ? ? ? ?.len ? ? ? ? ? ?= VGIC_NR_IRQS / 4,
> ? ? ? ? ? ? ? ?.handle_mmio ? ?= handle_mmio_cfg_reg,
> ? ? ? ?},
> ? ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? /* SGIRn */
> - ? ? ? ? ? ? ? .base ? ? ? ? ? = VGIC_DIST_BASE + 0xF00,
> + ? ? ? ? ? ? ? .base ? ? ? ? ? = 0xF00,
> ? ? ? ? ? ? ? ?.len ? ? ? ? ? ?= 4,
> ? ? ? ? ? ? ? ?.handle_mmio ? ?= handle_mmio_sgi_reg,
> ? ? ? ?},
> @@ -354,12 +354,14 @@ static const struct mmio_range vgic_ranges[] = {
>
> ?static const
> ?struct mmio_range *find_matching_range(const struct mmio_range *ranges,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct kvm_run *run)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct kvm_run *run, unsigned long base)
> ?{
> ? ? ? ?const struct mmio_range *r = ranges;
> + ? ? ? unsigned long addr = run->mmio.phys_addr - base;
> +
> ? ? ? ?while (r->len) {
> - ? ? ? ? ? ? ? if (run->mmio.phys_addr >= r->base &&
> - ? ? ? ? ? ? ? ? ? (run->mmio.phys_addr + run->mmio.len) <= (r->base + r->len))
> + ? ? ? ? ? ? ? if (addr >= r->base &&
> + ? ? ? ? ? ? ? ? ? (addr + run->mmio.len) <= (r->base + r->len))
> ? ? ? ? ? ? ? ? ? ? ? ?return r;
> ? ? ? ? ? ? ? ?r++;
> ? ? ? ?}
> @@ -370,18 +372,25 @@ struct mmio_range *find_matching_range(const struct mmio_range *ranges,
> ?int vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run)
> ?{
> ? ? ? ?const struct mmio_range *range;
> + ? ? ? struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> + ? ? ? unsigned long base = dist->vgic_dist_base;
>
> - ? ? ? if (!irqchip_in_kernel(vcpu->kvm))
> + ? ? ? if (!irqchip_in_kernel(vcpu->kvm) ||
> + ? ? ? ? ? run->mmio.phys_addr < base ||
> + ? ? ? ? ? (run->mmio.phys_addr + run->mmio.len) > (base + dist->vgic_dist_size))
the last line is unsafe in the very unlikely case that
run->mmio.phys_addr + run->mmio.len overflows right?
but then again, it will be caught underneath from the missing range,
so it's an optimization check.
> ? ? ? ? ? ? ? ?return KVM_EXIT_MMIO;
>
> - ? ? ? range = find_matching_range(vgic_ranges, run);
> - ? ? ? if (!range || !range->handle_mmio)
> + ? ? ? range = find_matching_range(vgic_ranges, run, base);
> + ? ? ? if (unlikely(!range || !range->handle_mmio)) {
> + ? ? ? ? ? ? ? pr_warn("Unhandled access %d %08llx %d\n",
> + ? ? ? ? ? ? ? ? ? ? ? run->mmio.is_write, run->mmio.phys_addr, run->mmio.len);
add a kvm_warn to include/linux/kvm_host.h and call kvm_warn
it's not apparent that this comes from KVM by looking at the error message.
> ? ? ? ? ? ? ? ?return KVM_EXIT_MMIO;
> + ? ? ? }
>
> ? ? ? ?spin_lock(&vcpu->kvm->arch.vgic.lock);
> ? ? ? ?pr_debug("emulating %d %08llx %d\n", run->mmio.is_write,
> ? ? ? ? ? ? ? ? run->mmio.phys_addr, run->mmio.len);
> - ? ? ? range->handle_mmio(vcpu, run, run->mmio.phys_addr - range->base);
> + ? ? ? range->handle_mmio(vcpu, run, run->mmio.phys_addr - range->base - base);
nit: I prefer - (range->base + base) to not force people to think
about the associativity
> ? ? ? ?kvm_handle_mmio_return(vcpu, run);
> ? ? ? ?spin_unlock(&vcpu->kvm->arch.vgic.lock);
>
> @@ -857,6 +866,8 @@ int kvm_vgic_init(struct kvm *kvm)
>
> ? ? ? ?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, VGIC_CPU_BASE,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?vcpu_res.start, VGIC_CPU_SIZE);
> --
> 1.7.7.1
>
Looks good (you could merge this down before releasing a new version
of the patches).
Thanks,
-Christoffer
