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: 9711459 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 6C2A060362 for ; Thu, 4 May 2017 11:22:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5DA392865F for ; Thu, 4 May 2017 11:22:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 5013928683; Thu, 4 May 2017 11:22:55 +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=-1.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id BBFCA2865F for ; Thu, 4 May 2017 11:22:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=6XQ0x6hE2+jaTr2MxrZX+inTSMD1vDvALs6UBbkacmQ=; b=AltHS/EIlr3Bpj nxLT7x6bKmWzDKewCr6n4O6P39xj0bX4VvRSFpRUUaHPHEwB0OJAmyKGgAkgrMqZAhMaigrNBCtP5 EG0N2BUL+KJ69sqi3DIPR+v6ASlkWjP+fWguhFczXq0uNW7x9T1hWoqtZ1Sweh+NBVkGMhgykHoST kvqJkit7teJQCdG2M5DB2sf/6Dnh3QeUdpb1w81EGozwvXA2A7U9ZwoZIqnMQsOC6ri1pcI22zn6c kFZ9AtJBsAc2aCr+f1uymZW4ECg2ErScccm796zNPG40dl/Zsn9qlVJr0K+4GYAp2EQZhgH5tHv2E zG5EodRwuF/fKx9hckyg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1d6EqU-0003ja-2E; Thu, 04 May 2017 11:22:54 +0000 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1d6EqP-0003ez-9N for linux-arm-kernel@lists.infradead.org; Thu, 04 May 2017 11:22:51 +0000 Received: by mail-wm0-x231.google.com with SMTP id 142so7593532wma.1 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=etSxcj9DHYBPJvX6ej2SzVe7uSu2KoT2GyECgGmRuRl63JkJc6HXrOxOpiN/HwzogO 3TDr1n47oojAxn5fLcSxMvS05pgLCf45IYMmQ6T46WqFlyhaH7kRH+Jm7dX2ukGL7c2I SPf819317jP0sTgb8sULV+PSnuzsgTd0bp5LBUREeuut7juVamVp7jCP93+QAofBtcRC SOsdi5eSJCKrI9q6UtvgUDMu7AIYYBtJLc3HIHH6F1qfhT+CVW6AVYvhYQpQwlR2pmdF Jj/7dWdj05Fh4Ex4/oAmavFU8rt9cOeVkSiIE+OpJ8l92bio7WezXWnHOIi04ob/rMbH oDfw== X-Gm-Message-State: AN3rC/6heYSeZKNZWK8ZA5FtrDm7pATuJ6qm6s03mL05rT7T8UY06TiP DLz+2HfB0V06gvC7 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 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) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170504_042249_483932_CB1E8A60 X-CRM114-Status: GOOD ( 40.58 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Maydell , Alexander Graf , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.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