From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: [RFC PATCH v2 05/19] KVM: arm/arm64: Check that system supports split eoi/deactivate Date: Tue, 1 Aug 2017 13:37:14 +0100 Message-ID: References: <20170717142718.13853-1-cdall@linaro.org> <20170717142718.13853-6-cdall@linaro.org> <3ca8bc15-b5e7-abf7-78cc-9bbc5332c267@arm.com> <20170801122611.GE5176@cbox> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org To: Christoffer Dall Return-path: Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:39980 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751339AbdHAMhR (ORCPT ); Tue, 1 Aug 2017 08:37:17 -0400 In-Reply-To: <20170801122611.GE5176@cbox> Sender: kvm-owner@vger.kernel.org List-ID: On 01/08/17 13:26, Christoffer Dall wrote: > On Tue, Aug 01, 2017 at 12:35:23PM +0100, Marc Zyngier wrote: >> On 17/07/17 15:27, Christoffer Dall wrote: >>> Some systems without proper firmware and/or hardware description data >>> don't support the split EOI and deactivate operation. >>> >>> On such systems, we cannot leave the physical interrupt active after the >>> timer handler on the host has run, so we cannot support KVM with an >>> in-kernel GIC with the timer changes we about to introduce. >>> >>> This patch makes sure that trying to initialize the KVM GIC code will >>> fail on such systems. >>> >>> Cc: Marc Zyngier >>> Signed-off-by: Christoffer Dall >>> --- >>> drivers/irqchip/irq-gic.c | 12 +++++++++--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>> index 090991f..b7e4fed 100644 >>> --- a/drivers/irqchip/irq-gic.c >>> +++ b/drivers/irqchip/irq-gic.c >>> @@ -1391,7 +1391,8 @@ int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq) >>> return 0; >>> } >>> >>> -static void __init gic_of_setup_kvm_info(struct device_node *node) >>> +static void __init gic_of_setup_kvm_info(struct device_node *node, >>> + bool supports_deactivate) >> >> Ouch, nasty. This shadows the static key which is also called >> supports_deactivate... >> > > oh, yeah, that's a trap waiting to happen. > >>> { >>> int ret; >>> struct resource *vctrl_res = &gic_v2_kvm_info.vctrl; >>> @@ -1411,6 +1412,9 @@ static void __init gic_of_setup_kvm_info(struct device_node *node) >>> if (ret) >>> return; >>> >>> + if (!supports_deactivate) >>> + return; >>> + >>> gic_set_kvm_info(&gic_v2_kvm_info); >> >> Speaking of which, the static key should already be initialized, so this >> could actually read: >> >> if (static_key_true(&supports_deactivate)) >> gic_set_kvm_info(&gic_v2_kvm_info); >> >>> } >>> >>> @@ -1419,6 +1423,7 @@ gic_of_init(struct device_node *node, struct device_node *parent) >>> { >>> struct gic_chip_data *gic; >>> int irq, ret; >>> + bool has_eoimode; >>> >>> if (WARN_ON(!node)) >>> return -ENODEV; >>> @@ -1436,7 +1441,8 @@ gic_of_init(struct device_node *node, struct device_node *parent) >>> * Disable split EOI/Deactivate if either HYP is not available >>> * or the CPU interface is too small. >>> */ >>> - if (gic_cnt == 0 && !gic_check_eoimode(node, &gic->raw_cpu_base)) >>> + has_eoimode = gic_check_eoimode(node, &gic->raw_cpu_base); >>> + if (gic_cnt == 0 && !has_eoimode) >>> static_key_slow_dec(&supports_deactivate); >>> >>> ret = __gic_init_bases(gic, -1, &node->fwnode); >>> @@ -1447,7 +1453,7 @@ gic_of_init(struct device_node *node, struct device_node *parent) >>> >>> if (!gic_cnt) { >>> gic_init_physaddr(node); >>> - gic_of_setup_kvm_info(node); >>> + gic_of_setup_kvm_info(node, has_eoimode); >>> } >>> >>> if (parent) { >>> >> >> and we shouldn't need any of this. What do you think? >> > > I wasn't exactly sure if gic_cnt > 0 && !gic_check_eiomode() could then > end up registering the KVM info when we shouldn't. > > If that's not a concern, I'm happy to rework this. I think it should be fine. gic_cnt is incremented each time we find a GIC, and we'll only register the KVM info when we discover the first one (while gic_cnt is still zero). Also, nobody is mad enough to have multiple GICs these days (cough...). Thanks, M. -- Jazz is not dead. It just smells funny... From mboxrd@z Thu Jan 1 00:00:00 1970 From: marc.zyngier@arm.com (Marc Zyngier) Date: Tue, 1 Aug 2017 13:37:14 +0100 Subject: [RFC PATCH v2 05/19] KVM: arm/arm64: Check that system supports split eoi/deactivate In-Reply-To: <20170801122611.GE5176@cbox> References: <20170717142718.13853-1-cdall@linaro.org> <20170717142718.13853-6-cdall@linaro.org> <3ca8bc15-b5e7-abf7-78cc-9bbc5332c267@arm.com> <20170801122611.GE5176@cbox> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 01/08/17 13:26, Christoffer Dall wrote: > On Tue, Aug 01, 2017 at 12:35:23PM +0100, Marc Zyngier wrote: >> On 17/07/17 15:27, Christoffer Dall wrote: >>> Some systems without proper firmware and/or hardware description data >>> don't support the split EOI and deactivate operation. >>> >>> On such systems, we cannot leave the physical interrupt active after the >>> timer handler on the host has run, so we cannot support KVM with an >>> in-kernel GIC with the timer changes we about to introduce. >>> >>> This patch makes sure that trying to initialize the KVM GIC code will >>> fail on such systems. >>> >>> Cc: Marc Zyngier >>> Signed-off-by: Christoffer Dall >>> --- >>> drivers/irqchip/irq-gic.c | 12 +++++++++--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>> index 090991f..b7e4fed 100644 >>> --- a/drivers/irqchip/irq-gic.c >>> +++ b/drivers/irqchip/irq-gic.c >>> @@ -1391,7 +1391,8 @@ int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq) >>> return 0; >>> } >>> >>> -static void __init gic_of_setup_kvm_info(struct device_node *node) >>> +static void __init gic_of_setup_kvm_info(struct device_node *node, >>> + bool supports_deactivate) >> >> Ouch, nasty. This shadows the static key which is also called >> supports_deactivate... >> > > oh, yeah, that's a trap waiting to happen. > >>> { >>> int ret; >>> struct resource *vctrl_res = &gic_v2_kvm_info.vctrl; >>> @@ -1411,6 +1412,9 @@ static void __init gic_of_setup_kvm_info(struct device_node *node) >>> if (ret) >>> return; >>> >>> + if (!supports_deactivate) >>> + return; >>> + >>> gic_set_kvm_info(&gic_v2_kvm_info); >> >> Speaking of which, the static key should already be initialized, so this >> could actually read: >> >> if (static_key_true(&supports_deactivate)) >> gic_set_kvm_info(&gic_v2_kvm_info); >> >>> } >>> >>> @@ -1419,6 +1423,7 @@ gic_of_init(struct device_node *node, struct device_node *parent) >>> { >>> struct gic_chip_data *gic; >>> int irq, ret; >>> + bool has_eoimode; >>> >>> if (WARN_ON(!node)) >>> return -ENODEV; >>> @@ -1436,7 +1441,8 @@ gic_of_init(struct device_node *node, struct device_node *parent) >>> * Disable split EOI/Deactivate if either HYP is not available >>> * or the CPU interface is too small. >>> */ >>> - if (gic_cnt == 0 && !gic_check_eoimode(node, &gic->raw_cpu_base)) >>> + has_eoimode = gic_check_eoimode(node, &gic->raw_cpu_base); >>> + if (gic_cnt == 0 && !has_eoimode) >>> static_key_slow_dec(&supports_deactivate); >>> >>> ret = __gic_init_bases(gic, -1, &node->fwnode); >>> @@ -1447,7 +1453,7 @@ gic_of_init(struct device_node *node, struct device_node *parent) >>> >>> if (!gic_cnt) { >>> gic_init_physaddr(node); >>> - gic_of_setup_kvm_info(node); >>> + gic_of_setup_kvm_info(node, has_eoimode); >>> } >>> >>> if (parent) { >>> >> >> and we shouldn't need any of this. What do you think? >> > > I wasn't exactly sure if gic_cnt > 0 && !gic_check_eiomode() could then > end up registering the KVM info when we shouldn't. > > If that's not a concern, I'm happy to rework this. I think it should be fine. gic_cnt is incremented each time we find a GIC, and we'll only register the KVM info when we discover the first one (while gic_cnt is still zero). Also, nobody is mad enough to have multiple GICs these days (cough...). Thanks, M. -- Jazz is not dead. It just smells funny...