From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH v7 19/19] KVM: ARM64: Add a new kvm ARM PMU device Date: Wed, 16 Dec 2015 21:33:36 +0100 Message-ID: <20151216203336.GE24889@cbox> References: <1450169379-12336-1-git-send-email-zhaoshenglong@huawei.com> <1450169379-12336-20-git-send-email-zhaoshenglong@huawei.com> <567032AD.8000206@arm.com> <567036DE.80605@linaro.org> <567038E3.3010102@arm.com> <20151215204739.GK4120@cbox> <56711348.6010406@huawei.com> <56711B99.1000608@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 748834852E for ; Wed, 16 Dec 2015 15:31:14 -0500 (EST) Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id dMTERYTWQoPw for ; Wed, 16 Dec 2015 15:31:13 -0500 (EST) Received: from mail-wm0-f45.google.com (mail-wm0-f45.google.com [74.125.82.45]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 5D83C41280 for ; Wed, 16 Dec 2015 15:31:13 -0500 (EST) Received: by mail-wm0-f45.google.com with SMTP id l126so55362509wml.1 for ; Wed, 16 Dec 2015 12:33:35 -0800 (PST) Content-Disposition: inline In-Reply-To: <56711B99.1000608@huawei.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu To: Shannon Zhao Cc: kvm@vger.kernel.org, Marc Zyngier , will.deacon@arm.com, Shannon Zhao , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org List-Id: kvmarm@lists.cs.columbia.edu On Wed, Dec 16, 2015 at 04:06:49PM +0800, Shannon Zhao wrote: > Hi, > > On 2015/12/16 15:31, Shannon Zhao wrote: > >>>> >> > But in this case, you're returning an error if it is *not* initialized. > >>>> >> > I understand that in that case you cannot return an interrupt number (-1 > >>>> >> > would be weird), but returning -EBUSY feels even more weird. > >>>> >> > > >>>> >> > I'd settle for -ENOXIO, or something similar. Anyone having a better idea? > >>>> >> > > >> > ENXIO or ENODEV would be my choice too, and add that to the > >> > Documentation clearly describing when this error code is used. > >> > > >> > By the way, why do you loop over all VCPUS to set the same value when > >> > you can't do anything per VCPU anyway? It seems to me it's either a > >> > per-VM property (that you can store on the VM data structure) or it's a > >> > true per-VCPU property? > > This is a per-VCPU property. PMU interrupt could be PPI or SPI. For PPI > > the interrupt numbers are same for each vcpu, while for SPI they are > > different, so it needs to set them separately. I planned to support both > > PPI and SPI. I think I should add support for SPI at this moment and let > > users (QEMU) to set these interrupts for each one. > > How about below vPMU Documentation? > > ARM Virtual Performance Monitor Unit (vPMU) > =========================================== > > Device types supported: > KVM_DEV_TYPE_ARM_PMU_V3 ARM Performance Monitor Unit v3 > > Instantiate one PMU instance for per VCPU through this API. > > Groups: > KVM_DEV_ARM_PMU_GRP_IRQ > Attributes: > The attr field of kvm_device_attr encodes two values: > bits: | 63 .... 32 | 31 .... 0 | > values: | vcpu_index | irq_num | > The irq_num describes the PMU overflow interrupt number for the > specified > vcpu_index vcpu. This interrupt could be a PPI or SPI, but for one > VM the > interrupt type must be same for each vcpu. some formatting snafus that I expect come from pasting the text in an e-mail client. > > Errors: > -ENXIO: Getting or setting this attribute is not yet supported 'not yet supported' as in something we'll implement later, or as in you need to call this other function before you can access this state? > -ENODEV: Getting the PMU overflow interrupt number while it's not set > -EBUSY: The PMU overflow interrupt is already set > -EINVAL: Invalid vcpu_index or irq_num supplied > > Otherwise looks good. Thanks, -Christoffer From mboxrd@z Thu Jan 1 00:00:00 1970 From: christoffer.dall@linaro.org (Christoffer Dall) Date: Wed, 16 Dec 2015 21:33:36 +0100 Subject: [PATCH v7 19/19] KVM: ARM64: Add a new kvm ARM PMU device In-Reply-To: <56711B99.1000608@huawei.com> References: <1450169379-12336-1-git-send-email-zhaoshenglong@huawei.com> <1450169379-12336-20-git-send-email-zhaoshenglong@huawei.com> <567032AD.8000206@arm.com> <567036DE.80605@linaro.org> <567038E3.3010102@arm.com> <20151215204739.GK4120@cbox> <56711348.6010406@huawei.com> <56711B99.1000608@huawei.com> Message-ID: <20151216203336.GE24889@cbox> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Dec 16, 2015 at 04:06:49PM +0800, Shannon Zhao wrote: > Hi, > > On 2015/12/16 15:31, Shannon Zhao wrote: > >>>> >> > But in this case, you're returning an error if it is *not* initialized. > >>>> >> > I understand that in that case you cannot return an interrupt number (-1 > >>>> >> > would be weird), but returning -EBUSY feels even more weird. > >>>> >> > > >>>> >> > I'd settle for -ENOXIO, or something similar. Anyone having a better idea? > >>>> >> > > >> > ENXIO or ENODEV would be my choice too, and add that to the > >> > Documentation clearly describing when this error code is used. > >> > > >> > By the way, why do you loop over all VCPUS to set the same value when > >> > you can't do anything per VCPU anyway? It seems to me it's either a > >> > per-VM property (that you can store on the VM data structure) or it's a > >> > true per-VCPU property? > > This is a per-VCPU property. PMU interrupt could be PPI or SPI. For PPI > > the interrupt numbers are same for each vcpu, while for SPI they are > > different, so it needs to set them separately. I planned to support both > > PPI and SPI. I think I should add support for SPI at this moment and let > > users (QEMU) to set these interrupts for each one. > > How about below vPMU Documentation? > > ARM Virtual Performance Monitor Unit (vPMU) > =========================================== > > Device types supported: > KVM_DEV_TYPE_ARM_PMU_V3 ARM Performance Monitor Unit v3 > > Instantiate one PMU instance for per VCPU through this API. > > Groups: > KVM_DEV_ARM_PMU_GRP_IRQ > Attributes: > The attr field of kvm_device_attr encodes two values: > bits: | 63 .... 32 | 31 .... 0 | > values: | vcpu_index | irq_num | > The irq_num describes the PMU overflow interrupt number for the > specified > vcpu_index vcpu. This interrupt could be a PPI or SPI, but for one > VM the > interrupt type must be same for each vcpu. some formatting snafus that I expect come from pasting the text in an e-mail client. > > Errors: > -ENXIO: Getting or setting this attribute is not yet supported 'not yet supported' as in something we'll implement later, or as in you need to call this other function before you can access this state? > -ENODEV: Getting the PMU overflow interrupt number while it's not set > -EBUSY: The PMU overflow interrupt is already set > -EINVAL: Invalid vcpu_index or irq_num supplied > > Otherwise looks good. Thanks, -Christoffer