From patchwork Thu May 4 09:44:04 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 9711311 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 3A1F060362 for ; Thu, 4 May 2017 09:44:36 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 38EFA285BA for ; Thu, 4 May 2017 09:44:36 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2DA2E2866D; Thu, 4 May 2017 09:44:36 +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 93C28285BA for ; Thu, 4 May 2017 09:44:35 +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=hThZG0PCDnvX0v/qrHKujrGeZWvoR68aUwl6wJKEj/M=; b=dhs3U3PajhNdhE tNRezZN7KxCLt7ct5T31RkUPjhdpEcPW4zveMDxzgJ3SoxMAuMjx1W6R+/M9jw9pIpe0dQry3k5xK MgSpWZti9mKiqUXzCiP6THMozGoBoiYp2I/uTEKrAkkvxcfxKeyjRiybCG+RCN6J/x9kQH/Rx3R2S feRxpGopNK48ao42xx9PW+FhP9pGfgIXQSJdmrl4CkebuX9D1i97yOJHd012giL/tz7oRnIF851nC qn73JWz1C5SUrZvGn7qcWYf56/F0CISXVo3iNzFd+MvnrNhkXduamJEET08qWY1qsfY3MyfngBznu 2DUD4AEfOCRkmldOjTlg==; 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 1d6DJL-000216-1S; Thu, 04 May 2017 09:44:35 +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 1d6DJE-0001xb-9j for linux-arm-kernel@lists.infradead.org; Thu, 04 May 2017 09:44:31 +0000 Received: by mail-wm0-x231.google.com with SMTP id w64so12788482wma.0 for ; Thu, 04 May 2017 02:44:07 -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=iFHAdqTlEfBx+ZJOZTZ9rkUE/G3quNwCOjtrptlGuR0=; b=VE2iThP7+zu5xOpd8YI9Jhdg/cI6K4U1dHRMYvIsq7iwNprZ6bjcNmRtxHx4Fc6+K4 WTNOrNV7SQdyBm5CepjscGnbbd+cazyDl2erdGEb2yK1iBHMxSTUTTSUdqivTZs+fxYF v5nkH6QyFiPfwcaods5q+df3AsiYOUHiuVXTk= 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=iFHAdqTlEfBx+ZJOZTZ9rkUE/G3quNwCOjtrptlGuR0=; b=TnBUBSZVs5tgsaZ6zCp9Dg846nIbKcn7cyp80LVew1OkY45CZ80uNnNLzS2FasRMWM pBX8iQYqXCou7r8uUFJXJShObh/aSkfC80481opF1lqquzKJI7PP021AcvHeWYl1p79U ECnUjPu5CNG3mecUZWp9XF6kREFNnmwFbD/GAbnUDguP2pjYlpWMMhrzhbPyxAgKjcyh FwAfISPgSpjfjTRG+gXtw/KfYtSDEHK6PLGxSffSW68pjEYC4AMdHG7MwU/iSKgI5Oju ZB6NJ/C6EJB+9qsDX5FseoZ3rJIPQtXsuTxPHz4vB622E6euKPEjT1aOsJ/7LcS2sSad lX3g== X-Gm-Message-State: AN3rC/4AKXH+przHiACIkuSJoKE2sfTRVXp4zejYMRWb7jhagoaPXutX z4eqYy62N6cuDDRu X-Received: by 10.80.164.178 with SMTP id w47mr29158590edb.19.1493891046397; Thu, 04 May 2017 02:44:06 -0700 (PDT) Received: from localhost (xd93ddc2d.cust.hiper.dk. [217.61.220.45]) by smtp.gmail.com with ESMTPSA id h29sm1027062eda.45.2017.05.04.02.44.05 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Thu, 04 May 2017 02:44:05 -0700 (PDT) Date: Thu, 4 May 2017 11:44:04 +0200 From: Christoffer Dall To: Marc Zyngier Subject: Re: [PATCH 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC Message-ID: <20170504094404.GI5923@cbox> References: <20170503183300.4618-1-cdall@linaro.org> <20170503183300.4618-2-cdall@linaro.org> <20170504081337.GF5923@cbox> <20170504083845.GH5923@cbox> <17cb39a1-f0a9-a1f1-fef5-78379d99de2f@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <17cb39a1-f0a9-a1f1-fef5-78379d99de2f@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_024428_775759_C5680F5D X-CRM114-Status: GOOD ( 43.41 ) 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 10:05:43AM +0100, Marc Zyngier wrote: > On 04/05/17 09:38, Christoffer Dall wrote: > > On Thu, May 04, 2017 at 09:28:50AM +0100, Marc Zyngier wrote: > >> On 04/05/17 09:13, Christoffer Dall wrote: > >>> On Thu, May 04, 2017 at 09:09:47AM +0100, Marc Zyngier wrote: > >>>> On 03/05/17 19:32, Christoffer Dall wrote: > >>>>> Since we got support for devices in userspace which allows reporting the > >>>>> PMU overflow output status to userspace, we should actually allow > >>>>> creating the PMU on systems without an in-kernel irqchip, which in turn > >>>>> requires us to slightly clarify error codes for the ABI and move things > >>>>> around for the initialization phase. > >>>>> > >>>>> Signed-off-by: Christoffer Dall > >>>>> --- > >>>>> Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++------- > >>>>> virt/kvm/arm/pmu.c | 27 +++++++++++++++++---------- > >>>>> 2 files changed, 26 insertions(+), 17 deletions(-) > >>>>> > >>>>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt > >>>>> index 02f5068..352af6e 100644 > >>>>> --- a/Documentation/virtual/kvm/devices/vcpu.txt > >>>>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt > >>>>> @@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a > >>>>> Returns: -EBUSY: The PMU overflow interrupt is already set > >>>>> -ENXIO: The overflow interrupt not set when attempting to get it > >>>>> -ENODEV: PMUv3 not supported > >>>>> - -EINVAL: Invalid PMU overflow interrupt number supplied > >>>>> + -EINVAL: Invalid PMU overflow interrupt number supplied or > >>>>> + trying to set the IRQ number without using an in-kernel > >>>>> + irqchip. > >>>>> > >>>>> A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt > >>>>> number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt > >>>>> @@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu. > >>>>> > >>>>> 1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT > >>>>> Parameters: no additional parameter in kvm_device_attr.addr > >>>>> -Returns: -ENODEV: PMUv3 not supported > >>>>> - -ENXIO: PMUv3 not properly configured as required prior to calling this > >>>>> - attribute > >>>>> +Returns: -ENODEV: PMUv3 not supported or GIC not initialized > >>>>> + -ENXIO: PMUv3 not properly configured or in-kernel irqchip not > >>>>> + conigured as required prior to calling this attribute > >>>>> -EBUSY: PMUv3 already initialized > >>>>> > >>>>> -Request the initialization of the PMUv3. This must be done after creating the > >>>>> -in-kernel irqchip. Creating a PMU with a userspace irqchip is currently not > >>>>> -supported. > >>>>> +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. > >>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c > >>>>> index 4b43e7f..f046b08 100644 > >>>>> --- a/virt/kvm/arm/pmu.c > >>>>> +++ b/virt/kvm/arm/pmu.c > >>>>> @@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu) > >>>>> if (!kvm_arm_support_pmu_v3()) > >>>>> return -ENODEV; > >>>>> > >>>>> - /* > >>>>> - * We currently require an in-kernel VGIC to use the PMU emulation, > >>>>> - * because we do not support forwarding PMU overflow interrupts to > >>>>> - * userspace yet. > >>>>> - */ > >>>>> - if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm)) > >>>>> - return -ENODEV; > >>>>> - > >>>>> - if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) || > >>>>> - !kvm_arm_pmu_irq_initialized(vcpu)) > >>>>> + if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features)) > >>>>> return -ENXIO; > >>>>> > >>>>> if (kvm_arm_pmu_v3_ready(vcpu)) > >>>>> return -EBUSY; > >>>>> > >>>>> + if (irqchip_in_kernel(vcpu->kvm)) { > >>>>> + /* > >>>>> + * If using the PMU with an in-kernel virtual GIC > >>>>> + * implementation, we require the GIC to be already > >>>>> + * initialized when initializing the PMU. > >>>>> + */ > >>>>> + if (!vgic_initialized(vcpu->kvm)) > >>>>> + return -ENODEV; > >>>>> + > >>>>> + if (!kvm_arm_pmu_irq_initialized(vcpu)) > >>>>> + return -ENXIO; > >>>>> + } > >>>>> + > >>>>> kvm_pmu_vcpu_reset(vcpu); > >>>>> vcpu->arch.pmu.ready = true; > >>>>> > >>>>> @@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) > >>>>> int __user *uaddr = (int __user *)(long)attr->addr; > >>>>> int irq; > >>>>> > >>>>> + if (!irqchip_in_kernel(vcpu->kvm)) > >>>>> + return -EINVAL; > >>>>> + > >>>> > >>>> Shouldn't we fail the same way for {get,has}_attr? get_attr is going to > >>>> generate a -ENXIO, and has_attr is going to lie about the availability > >>>> of KVM_ARM_VCPU_PMU_V3_IRQ... > >>>> > >>> > >>> Here's the text from api.txt: > >>> > >>> Tests whether a device supports a particular attribute. A successful > >>> return indicates the attribute is implemented. It does not necessarily > >>> indicate that the attribute can be read or written in the device's > >>> current state. "addr" is ignored. > >>> > >>> My interpretation therefore is that QEMU can use this ioctl to figure > >>> out if the feature is supported (sort of like a capability), but that > >>> doesn't mean that the configuration of the VM is such that the attribute > >>> can be get or set at that moment. > >>> > >>> For example, there will also alway be situations where you can get an > >>> attr, but not set an attr, what should the has_attr return then? > >> > >> My issue here is that whether we can get/set the interrupt or not is not > >> a function of the device itself, but of the way it is "wired". No matter > >> what "the device's current state" is, we'll never be able to get/set the > >> interrupt. > >> > >> I'd tend to err on the side of caution and return something that is > >> unambiguous, be maybe I have too strict an interpretation of the API. > >> > > > > Hmm, I see the has_attr as a method for userspace to discover "does this > > kernel have this feature". If we make has_attr return a value depending > > on the VM having an in-kernel gic or not, we implicitly require > > userspace to create a VM with an in-kernel GIC to discover if this > > kernel has that feature, and therefore also impose an ordering > > requirement of figuring out the capabilities of the kernel (i.e. create > > the GIC before checking this API). > > > > I think QEMU should be able to do: > > > > if (has_attr()) { > > kvm_supports_set_timer_irq = true; > > vtimer_irq = foo; > > } else { > > kvm_supports_set_timer_irq = false; > > vtimer_irq = 27; /* default, we're stuck with it */ > > } > > > > create_board_definition(); > > create_dt(); > > create_acpi(); > > > > /* do whatever */ > > > > if (kvm_supports_set_timer_irq && kvm_irqchip_in_kernel()) { > > kvm_arm_timer_set_irq(...); > > } > > > > And all this should not be coupled to when we create the irqchip device. > > > > But I may be failing to see the case where the current implementation > > creates a problem for userspace, in which case we should document the > > ordering requirement. > > I'm not sure it would create any problem, at least not for the PMU > (there is no working code that would have created a PMU without an irqchip). > > It is just that we have a slightly diverging interpretation of what > has_attr means. You see it as "attribute that the device supports", and > I see it as "attribute that the device supports in this configuration". > I'm happy to use your semantics from now on. In either case, we should make sure this is clear in the ABI. I thought that the "It does not necessarily indicate that the attribute can be read or written in the device's current state." implied my interpretation, but maybe I'm missing some subtlety there? Do you think we should clarify the API? By the way, I now realize that we are not maintining the same understanding between get/set_attr, which I really think we should, so I'll add the following: I was only talking about has_attr above. Let me know. Thanks, -Christoffer diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c index 9b3e3ea..0cf62b7 100644 --- a/virt/kvm/arm/pmu.c +++ b/virt/kvm/arm/pmu.c @@ -551,6 +551,9 @@ int kvm_arm_pmu_v3_get_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) int __user *uaddr = (int __user *)(long)attr->addr; int irq; + if (!irqchip_in_kernel(vcpu->kvm)) + return -EINVAL; + if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features)) return -ENODEV;