From patchwork Thu May 4 11:22:25 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 9711457 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id A189A60362 for ; Thu, 4 May 2017 11:22:33 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 973D6262FF for ; Thu, 4 May 2017 11:22:33 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8B4B328387; Thu, 4 May 2017 11:22:33 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.5 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id AFB4F262FF for ; Thu, 4 May 2017 11:22:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752465AbdEDLWa (ORCPT ); Thu, 4 May 2017 07:22:30 -0400 Received: from mail-wm0-f51.google.com ([74.125.82.51]:35402 "EHLO mail-wm0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752450AbdEDLW3 (ORCPT ); Thu, 4 May 2017 07:22:29 -0400 Received: by mail-wm0-f51.google.com with SMTP id w64so15167429wma.0 for ; Thu, 04 May 2017 04:22:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=H05D/vPFk0hvKYwGfHS7z1uq4znQfN9PxyHxjNcV6Bs=; b=b5G8CNuOSDv73QD8Qj71VYu5J6fbqZP7RYRGQvZnZYDQgaEdbT6OLK07cbZevArhOF fshS6Eo9sGU2zila6ByP0n5dWgaJuuydGAwSscC8xrZDqmqb9Z2Pj41A+fskv78NVqtD TcdpTxi1gOxLHHPVzyQJHDGPsxn0CLPlEtBFo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=H05D/vPFk0hvKYwGfHS7z1uq4znQfN9PxyHxjNcV6Bs=; b=NN1aBGE6tY/Gky4GrpIwTeLA1X2eieQDoPfN80ASu84KG4oPmVkHBT2GLZB/7e1oG6 MoSAfY3I9WpaZz/TxyWH+8045PjDTBwf01X6WXi3Mm4dxbkeKc/rEiKCKtRTPP6kUE5m rmmVF2/wE/5b/Xa42v0zq4QYDoMpx6OfywMm6fOL+wUuKhfxe5lUEXwscZ6F1ENcEpXe BTSPfYrmUjF4WoguNjxTK6t2acwCAuMhnbvTWJmCmfq8xeCi5rYWV/dY8f4IOnZIGxIx rx+pcgqqKDs7lRQCQIytUSjjeXHdtUvCoyRAp2gxFPqhIE+GBVwLNzamsEKTIrd2kacW quJg== X-Gm-Message-State: AN3rC/4J67qmbAic4itW9UBjsi4aFe92p0O0yXaSZ31/5DF9E5XAUZNH YvGmX+PMilKS2pZ0RMJ2zg== X-Received: by 10.80.214.215 with SMTP id l23mr3863869edj.147.1493896947408; Thu, 04 May 2017 04:22:27 -0700 (PDT) Received: from localhost (xd93ddc2d.cust.hiper.dk. [217.61.220.45]) by smtp.gmail.com with ESMTPSA id k36sm485689edb.11.2017.05.04.04.22.26 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Thu, 04 May 2017 04:22:26 -0700 (PDT) Date: Thu, 4 May 2017 13:22:25 +0200 From: Christoffer Dall To: Marc Zyngier Cc: kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, Alexander Graf , Peter Maydell Subject: Re: [PATCH 5/5] KVM: arm/arm64: Allow setting the timer IRQ numbers from userspace Message-ID: <20170504112225.GM5923@cbox> References: <20170503183300.4618-1-cdall@linaro.org> <20170503183300.4618-6-cdall@linaro.org> <20170504095918.GJ5923@cbox> <79ec69f2-d727-46ec-b1d5-4c535a904e63@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <79ec69f2-d727-46ec-b1d5-4c535a904e63@arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, May 04, 2017 at 11:54:06AM +0100, Marc Zyngier wrote: > On 04/05/17 10:59, Christoffer Dall wrote: > > On Thu, May 04, 2017 at 10:34:32AM +0100, Marc Zyngier wrote: > >> On 03/05/17 19:33, Christoffer Dall wrote: > >>> First we define an ABI using the vcpu devices that lets userspace set > >>> the interrupt numbers for the various timers on both the 32-bit and > >>> 64-bit KVM/ARM implementations. > >>> > >>> Second, we add the definitions for the groups and attributes introduced > >>> by the above ABI. (We add the PMU define on the 32-bit side as well for > >>> symmetry and it may get used some day.) > >>> > >>> Third, we set up the arch-specific vcpu device operation handlers to > >>> call into the timer code for anything related to the > >>> KVM_ARM_VCPU_TIMER_CTRL group. > >>> > >>> Fourth, we implement support for getting and setting the timer interrupt > >>> numbers using the above defined ABI in the arch timer code. > >>> > >>> Fifth, we introduce error checking upon enabling the arch timer (which > >>> is called when first running a VCPU) to check that all VCPUs are > >>> configured to use the same PPI for the timer (as mandated by the > >>> architecture) and that the virtual and physical timers are not > >>> configured to use the same IRQ number. > >>> > >>> Signed-off-by: Christoffer Dall > >>> --- > >>> Documentation/virtual/kvm/devices/vcpu.txt | 25 +++++++ > >>> arch/arm/include/uapi/asm/kvm.h | 8 +++ > >>> arch/arm/kvm/guest.c | 9 +++ > >>> arch/arm64/include/uapi/asm/kvm.h | 3 + > >>> arch/arm64/kvm/guest.c | 9 +++ > >>> include/kvm/arm_arch_timer.h | 4 ++ > >>> virt/kvm/arm/arch_timer.c | 104 +++++++++++++++++++++++++++++ > >>> 7 files changed, 162 insertions(+) > >>> > >>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt > >>> index 352af6e..013e3f1 100644 > >>> --- a/Documentation/virtual/kvm/devices/vcpu.txt > >>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt > >>> @@ -35,3 +35,28 @@ Returns: -ENODEV: PMUv3 not supported or GIC not initialized > >>> Request the initialization of the PMUv3. If using the PMUv3 with an in-kernel > >>> virtual GIC implementation, this must be done after initializing the in-kernel > >>> irqchip. > >>> + > >>> + > >>> +2. GROUP: KVM_ARM_VCPU_TIMER_CTRL > >>> +Architectures: ARM,ARM64 > >>> + > >>> +2.1. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_VTIMER > >>> +2.2. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_PTIMER > >>> +Parameters: in kvm_device_attr.addr the address for the timer interrupt is a > >>> + pointer to an int > >>> +Returns: -EINVAL: Invalid timer interrupt number > >>> + -EBUSY: One or more VCPUs has already run > >>> + > >>> +A value describing the architected timer interrupt number when connected to an > >>> +in-kernel virtual GIC. These must be a PPI (16 <= intid < 32). If an > >>> +attribute is not set, a default value is applied (see below). > >>> + > >>> +KVM_ARM_VCPU_TIMER_IRQ_VTIMER: The EL1 virtual timer intid (default: 27) > >>> +KVM_ARM_VCPU_TIMER_IRQ_PTIMER: The EL1 physical timer intid (default: 30) > >> > >> uber nit: my reading of the code is that the default is set at VCPU > >> creation time. So setting the attribute overrides the default, not that > >> the default is applied if no attribute is set (i.e. there is always a > >> valid value). > >> > > > > uh, right, I don't see the distinction though, so not sure how to > > correct the text. > > > > Would "Setting the attribute overrides the default values (see below)." > > work instead? > > Looks good to me. > > [...] > > >> > >> Another issue that just popped in my head: how do we ensure that we > >> don't clash between the PMU and the timers? Yes, that's a foolish thing > >> to do, but that's no different from avoiding the same interrupt on the > >> timers... > >> > > Argh, good point. > > > > So actually, nothing *really bad* happens from using the same IRQ number > > except that the VM gets confused. However, it's living life dangerously > > because we use the HW bit for the vtimer, so we if ever start using it > > for other timers or the PMU, then you could end up in a situation where > > you unmap the phys mapping on behalf of another device, and the physical > > interrupt could be left in an active state. > > > > On the other hand, the vtimer currently belongs only to VMs and we set > > the required physical active state before entering the VM, so maybe it > > doesn't matter. > > So far, we always assume that there is never more than a single source > per interrupt. We'll end-up with weird behaviours because our line_level > field is not an OR of the various inputs, but a direct assignment > (device A and B raise the line, then A drops it -> B's interrupt is gone). > > I think that only the guest will be confused by it, but accepting it may > be interpreted as "this should work correctly". Would documenting that > it is a bad idea be enough? > > > Should we just forego these checks and let the user shoot itself in the > > foot if he/she wants to? > > If the documentation is enough, why not. Otherwise, we need some form of > allocator. Boring. ;-) > Well, it's not that bad really (untested): The problem is that it doesn't help that much, because userspace can still change the level of an IRQ which is connected to an in-kernel device, unless we also introduce checking the owner field in the injection path. I don't see it blowing up the host with/without an allocator, so I'm fine with documentation, but I can also include the above. Thoughts? Thanks, -Christoffer diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index 1541f5d..122f9d3 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -121,6 +121,9 @@ struct vgic_irq { u8 source; /* GICv2 SGIs only */ u8 priority; enum vgic_irq_config config; /* Level or edge */ + + void *owner; /* Opaque pointer to reserve an interrupt + for in-kernel devices. */ }; struct vgic_register_region; diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c index 3d0979c..7561d2d 100644 --- a/virt/kvm/arm/vgic/vgic.c +++ b/virt/kvm/arm/vgic/vgic.c @@ -429,6 +429,42 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq) } /** + * kvm_vgic_set_owner - Set the owner of an interrupt for a VM + * + * @kvm: Pointer to the VM structure + * @intid: The virtual INTID identifying the interrupt (PPI or SPI) + * @owner: Opaque pointer to the owner + * + * Returns 0 if intid is not already used by another in-kernel device and the + * owner is set, otherwise returns an error code. + * + * We only set the owner for VCPU 0 for PPIs. + */ +int kvm_vgic_set_owner(struct kvm *kvm, unsigned int intid, void *owner) +{ + struct vgic_irq *irq; + struct kvm_vcpu *vcpu = kvm_get_vcpu(kvm, 0); + int ret = 0; + + if (!vgic_initialized(kvm)) + return -EAGAIN; + + /* SGIs and LPIs cannot be wired up to any device */ + if (!irq_is_ppi(intid) && !vgic_valid_spi(kvm, intid)) + return -EINVAL; + + irq = vgic_get_irq(kvm, vcpu, intid); + spin_lock(&irq->irq_lock); + if (irq->owner && irq->owner != owner) + ret = -EEXIST; + else + irq->owner = owner; + spin_unlock(&irq->irq_lock); + + return ret; +} + +/** * vgic_prune_ap_list - Remove non-relevant interrupts from the list * * @vcpu: The VCPU pointer