From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7C734C433E0 for ; Mon, 22 Feb 2021 10:26:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 367E164DFD for ; Mon, 22 Feb 2021 10:26:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230299AbhBVK0Y (ORCPT ); Mon, 22 Feb 2021 05:26:24 -0500 Received: from foss.arm.com ([217.140.110.172]:39458 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230189AbhBVKZe (ORCPT ); Mon, 22 Feb 2021 05:25:34 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 99B69D6E; Mon, 22 Feb 2021 02:24:38 -0800 (PST) Received: from slackpad.fritz.box (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6C95E3F73B; Mon, 22 Feb 2021 02:24:37 -0800 (PST) Date: Mon, 22 Feb 2021 10:23:33 +0000 From: Andre Przywara To: Alexandru Elisei Cc: Will Deacon , Julien Thierry , kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, Marc Zyngier Subject: Re: [PATCH kvmtool 01/21] ioport: Remove ioport__setup_arch() Message-ID: <20210222102333.2f1cb9e2@slackpad.fritz.box> In-Reply-To: <20210217155459.3a4bc991@slackpad.fritz.box> References: <20201210142908.169597-1-andre.przywara@arm.com> <20201210142908.169597-2-andre.przywara@arm.com> <814e0cd9-5e54-fade-f05c-80ea2b4a9039@arm.com> <20210211171648.36000cce@slackpad.fritz.box> <111b6cd6-ddf3-ec67-b782-67120be97943@arm.com> <20210217155459.3a4bc991@slackpad.fritz.box> Organization: Arm Ltd. X-Mailer: Claws Mail 3.17.1 (GTK+ 2.24.31; x86_64-slackware-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Wed, 17 Feb 2021 16:46:47 +0000 Andre Przywara wrote: > On Thu, 11 Feb 2021 17:32:01 +0000 > Alexandru Elisei wrote: > > Hi, > > > On 2/11/21 5:16 PM, Andre Przywara wrote: > > > On Wed, 10 Feb 2021 17:44:59 +0000 > > > Alexandru Elisei wrote: > > > > > > Hi Alex, > > > > > >> On 12/10/20 2:28 PM, Andre Przywara wrote: > > >>> Since x86 had a special need for registering tons of special I/O ports, > > >>> we had an ioport__setup_arch() callback, to allow each architecture > > >>> to do the same. As it turns out no one uses it beside x86, so we remove > > >>> that unnecessary abstraction. > > >>> > > >>> The generic function was registered via a device_base_init() call, so > > >>> we just do the same for the x86 specific function only, and can remove > > >>> the unneeded ioport__setup_arch(). > > >>> > > >>> Signed-off-by: Andre Przywara > > >>> --- > > >>> arm/ioport.c | 5 ----- > > >>> include/kvm/ioport.h | 1 - > > >>> ioport.c | 28 ---------------------------- > > >>> mips/kvm.c | 5 ----- > > >>> powerpc/ioport.c | 6 ------ > > >>> x86/ioport.c | 25 ++++++++++++++++++++++++- > > >>> 6 files changed, 24 insertions(+), 46 deletions(-) > > >>> > > >>> diff --git a/arm/ioport.c b/arm/ioport.c > > >>> index 2f0feb9a..24092c9d 100644 > > >>> --- a/arm/ioport.c > > >>> +++ b/arm/ioport.c > > >>> @@ -1,11 +1,6 @@ > > >>> #include "kvm/ioport.h" > > >>> #include "kvm/irq.h" > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm) > > >>> -{ > > >>> - return 0; > > >>> -} > > >>> - > > >>> void ioport__map_irq(u8 *irq) > > >>> { > > >>> *irq = irq__alloc_line(); > > >>> diff --git a/include/kvm/ioport.h b/include/kvm/ioport.h > > >>> index 039633f7..d0213541 100644 > > >>> --- a/include/kvm/ioport.h > > >>> +++ b/include/kvm/ioport.h > > >>> @@ -35,7 +35,6 @@ struct ioport_operations { > > >>> enum irq_type)); > > >>> }; > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm); > > >>> void ioport__map_irq(u8 *irq); > > >>> > > >>> int __must_check ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, > > >>> diff --git a/ioport.c b/ioport.c > > >>> index 844a832d..667e8386 100644 > > >>> --- a/ioport.c > > >>> +++ b/ioport.c > > >>> @@ -158,21 +158,6 @@ int ioport__unregister(struct kvm *kvm, u16 port) > > >>> return 0; > > >>> } > > >>> > > >>> -static void ioport__unregister_all(void) > > >>> -{ > > >>> - struct ioport *entry; > > >>> - struct rb_node *rb; > > >>> - struct rb_int_node *rb_node; > > >>> - > > >>> - rb = rb_first(&ioport_tree); > > >>> - while (rb) { > > >>> - rb_node = rb_int(rb); > > >>> - entry = ioport_node(rb_node); > > >>> - ioport_unregister(&ioport_tree, entry); > > >>> - rb = rb_first(&ioport_tree); > > >>> - } > > >>> -} > > >> I get the impression this is a rebasing artifact. The commit message doesn't > > >> mention anything about removing ioport__exit() -> ioport__unregister_all(), and as > > >> far as I can tell it's still needed because there are places other than > > >> ioport__setup_arch() from where ioport__register() is called. > > > I agree that the commit message is a bit thin on this fact, but the > > > functionality of ioport__unregister_all() is now in > > > x86/ioport.c:ioport__remove_arch(). I think removing ioport__init() > > > without removing ioport__exit() as well would look very weird, if not > > > hackish. > > > > Not necessarily. ioport__unregister_all() removes the ioports added by > > x86/ioport.c::ioport__setup_arch(), *plus* ioports added by different devices, > > like serial, rtc, virtio-pci and vfio-pci (which are used by arm/arm64). > > Right, indeed. Not that it really matters, since we are about to exit > anyway, but it looks indeed I need to move this to a generic teardown > method, or actually just keep that part here in this file. > > Will give this a try. Well, now having a closer look I needed to remove this from here, because this whole file will go away. To keep the current functionality, we would need to add it to mmio.c, and interestingly we don't do any kind of similar cleanup there for the MMIO regions (probably this is kvmtool exiting anyway, see above). I will see if I can introduce it there, for good measure. Cheers, Andre > > Thanks! > Andre > > > > > > > I can amend the commit message to mention this, or is there anything > > > else I missed? > > > > > > Cheers, > > > Andre > > > > > >>> - > > >>> static const char *to_direction(int direction) > > >>> { > > >>> if (direction == KVM_EXIT_IO_IN) > > >>> @@ -220,16 +205,3 @@ out: > > >>> > > >>> return !kvm->cfg.ioport_debug; > > >>> } > > >>> - > > >>> -int ioport__init(struct kvm *kvm) > > >>> -{ > > >>> - return ioport__setup_arch(kvm); > > >>> -} > > >>> -dev_base_init(ioport__init); > > >>> - > > >>> -int ioport__exit(struct kvm *kvm) > > >>> -{ > > >>> - ioport__unregister_all(); > > >>> - return 0; > > >>> -} > > >>> -dev_base_exit(ioport__exit); > > >>> diff --git a/mips/kvm.c b/mips/kvm.c > > >>> index 26355930..e110e5d5 100644 > > >>> --- a/mips/kvm.c > > >>> +++ b/mips/kvm.c > > >>> @@ -100,11 +100,6 @@ void kvm__irq_trigger(struct kvm *kvm, int irq) > > >>> die_perror("KVM_IRQ_LINE ioctl"); > > >>> } > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm) > > >>> -{ > > >>> - return 0; > > >>> -} > > >>> - > > >>> bool kvm__arch_cpu_supports_vm(void) > > >>> { > > >>> return true; > > >>> diff --git a/powerpc/ioport.c b/powerpc/ioport.c > > >>> index 0c188b61..a5cff4ee 100644 > > >>> --- a/powerpc/ioport.c > > >>> +++ b/powerpc/ioport.c > > >>> @@ -12,12 +12,6 @@ > > >>> > > >>> #include > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm) > > >>> -{ > > >>> - /* PPC has no legacy ioports to set up */ > > >>> - return 0; > > >>> -} > > >>> - > > >>> void ioport__map_irq(u8 *irq) > > >>> { > > >>> } > > >>> diff --git a/x86/ioport.c b/x86/ioport.c > > >>> index 7ad7b8f3..8c5c7699 100644 > > >>> --- a/x86/ioport.c > > >>> +++ b/x86/ioport.c > > >>> @@ -69,7 +69,7 @@ void ioport__map_irq(u8 *irq) > > >>> { > > >>> } > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm) > > >>> +static int ioport__setup_arch(struct kvm *kvm) > > >>> { > > >>> int r; > > >>> > > >>> @@ -150,3 +150,26 @@ int ioport__setup_arch(struct kvm *kvm) > > >>> > > >>> return 0; > > >>> } > > >>> +dev_base_init(ioport__setup_arch); > > >>> + > > >>> +static int ioport__remove_arch(struct kvm *kvm) > > >>> +{ > > >>> + ioport__unregister(kvm, 0x510); > > >>> + ioport__unregister(kvm, 0x402); > > >>> + ioport__unregister(kvm, 0x03D5); > > >>> + ioport__unregister(kvm, 0x03D4); > > >>> + ioport__unregister(kvm, 0x0378); > > >>> + ioport__unregister(kvm, 0x0278); > > >>> + ioport__unregister(kvm, 0x00F0); > > >>> + ioport__unregister(kvm, 0x00ED); > > >>> + ioport__unregister(kvm, IOPORT_DBG); > > >>> + ioport__unregister(kvm, 0x00C0); > > >>> + ioport__unregister(kvm, 0x00A0); > > >>> + ioport__unregister(kvm, 0x0092); > > >>> + ioport__unregister(kvm, 0x0040); > > >>> + ioport__unregister(kvm, 0x0020); > > >>> + ioport__unregister(kvm, 0x0000); > > >>> + > > >>> + return 0; > > >>> +} > > >>> +dev_base_exit(ioport__remove_arch); > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DB549C433E0 for ; Mon, 22 Feb 2021 10:24:44 +0000 (UTC) Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by mail.kernel.org (Postfix) with ESMTP id 3E9B964E31 for ; Mon, 22 Feb 2021 10:24:44 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3E9B964E31 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvmarm-bounces@lists.cs.columbia.edu Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 7E87C4B15D; Mon, 22 Feb 2021 05:24:43 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu 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 KH93s2pRIya9; Mon, 22 Feb 2021 05:24:42 -0500 (EST) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 22E8B4B16C; Mon, 22 Feb 2021 05:24:42 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 10D124B15D for ; Mon, 22 Feb 2021 05:24:41 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu 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 51SMMGt7pctw for ; Mon, 22 Feb 2021 05:24:39 -0500 (EST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 7C7EE4B168 for ; Mon, 22 Feb 2021 05:24:39 -0500 (EST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 99B69D6E; Mon, 22 Feb 2021 02:24:38 -0800 (PST) Received: from slackpad.fritz.box (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6C95E3F73B; Mon, 22 Feb 2021 02:24:37 -0800 (PST) Date: Mon, 22 Feb 2021 10:23:33 +0000 From: Andre Przywara To: Alexandru Elisei Subject: Re: [PATCH kvmtool 01/21] ioport: Remove ioport__setup_arch() Message-ID: <20210222102333.2f1cb9e2@slackpad.fritz.box> In-Reply-To: <20210217155459.3a4bc991@slackpad.fritz.box> References: <20201210142908.169597-1-andre.przywara@arm.com> <20201210142908.169597-2-andre.przywara@arm.com> <814e0cd9-5e54-fade-f05c-80ea2b4a9039@arm.com> <20210211171648.36000cce@slackpad.fritz.box> <111b6cd6-ddf3-ec67-b782-67120be97943@arm.com> <20210217155459.3a4bc991@slackpad.fritz.box> Organization: Arm Ltd. X-Mailer: Claws Mail 3.17.1 (GTK+ 2.24.31; x86_64-slackware-linux-gnu) MIME-Version: 1.0 Cc: kvm@vger.kernel.org, Marc Zyngier , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu On Wed, 17 Feb 2021 16:46:47 +0000 Andre Przywara wrote: > On Thu, 11 Feb 2021 17:32:01 +0000 > Alexandru Elisei wrote: > > Hi, > > > On 2/11/21 5:16 PM, Andre Przywara wrote: > > > On Wed, 10 Feb 2021 17:44:59 +0000 > > > Alexandru Elisei wrote: > > > > > > Hi Alex, > > > > > >> On 12/10/20 2:28 PM, Andre Przywara wrote: > > >>> Since x86 had a special need for registering tons of special I/O ports, > > >>> we had an ioport__setup_arch() callback, to allow each architecture > > >>> to do the same. As it turns out no one uses it beside x86, so we remove > > >>> that unnecessary abstraction. > > >>> > > >>> The generic function was registered via a device_base_init() call, so > > >>> we just do the same for the x86 specific function only, and can remove > > >>> the unneeded ioport__setup_arch(). > > >>> > > >>> Signed-off-by: Andre Przywara > > >>> --- > > >>> arm/ioport.c | 5 ----- > > >>> include/kvm/ioport.h | 1 - > > >>> ioport.c | 28 ---------------------------- > > >>> mips/kvm.c | 5 ----- > > >>> powerpc/ioport.c | 6 ------ > > >>> x86/ioport.c | 25 ++++++++++++++++++++++++- > > >>> 6 files changed, 24 insertions(+), 46 deletions(-) > > >>> > > >>> diff --git a/arm/ioport.c b/arm/ioport.c > > >>> index 2f0feb9a..24092c9d 100644 > > >>> --- a/arm/ioport.c > > >>> +++ b/arm/ioport.c > > >>> @@ -1,11 +1,6 @@ > > >>> #include "kvm/ioport.h" > > >>> #include "kvm/irq.h" > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm) > > >>> -{ > > >>> - return 0; > > >>> -} > > >>> - > > >>> void ioport__map_irq(u8 *irq) > > >>> { > > >>> *irq = irq__alloc_line(); > > >>> diff --git a/include/kvm/ioport.h b/include/kvm/ioport.h > > >>> index 039633f7..d0213541 100644 > > >>> --- a/include/kvm/ioport.h > > >>> +++ b/include/kvm/ioport.h > > >>> @@ -35,7 +35,6 @@ struct ioport_operations { > > >>> enum irq_type)); > > >>> }; > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm); > > >>> void ioport__map_irq(u8 *irq); > > >>> > > >>> int __must_check ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, > > >>> diff --git a/ioport.c b/ioport.c > > >>> index 844a832d..667e8386 100644 > > >>> --- a/ioport.c > > >>> +++ b/ioport.c > > >>> @@ -158,21 +158,6 @@ int ioport__unregister(struct kvm *kvm, u16 port) > > >>> return 0; > > >>> } > > >>> > > >>> -static void ioport__unregister_all(void) > > >>> -{ > > >>> - struct ioport *entry; > > >>> - struct rb_node *rb; > > >>> - struct rb_int_node *rb_node; > > >>> - > > >>> - rb = rb_first(&ioport_tree); > > >>> - while (rb) { > > >>> - rb_node = rb_int(rb); > > >>> - entry = ioport_node(rb_node); > > >>> - ioport_unregister(&ioport_tree, entry); > > >>> - rb = rb_first(&ioport_tree); > > >>> - } > > >>> -} > > >> I get the impression this is a rebasing artifact. The commit message doesn't > > >> mention anything about removing ioport__exit() -> ioport__unregister_all(), and as > > >> far as I can tell it's still needed because there are places other than > > >> ioport__setup_arch() from where ioport__register() is called. > > > I agree that the commit message is a bit thin on this fact, but the > > > functionality of ioport__unregister_all() is now in > > > x86/ioport.c:ioport__remove_arch(). I think removing ioport__init() > > > without removing ioport__exit() as well would look very weird, if not > > > hackish. > > > > Not necessarily. ioport__unregister_all() removes the ioports added by > > x86/ioport.c::ioport__setup_arch(), *plus* ioports added by different devices, > > like serial, rtc, virtio-pci and vfio-pci (which are used by arm/arm64). > > Right, indeed. Not that it really matters, since we are about to exit > anyway, but it looks indeed I need to move this to a generic teardown > method, or actually just keep that part here in this file. > > Will give this a try. Well, now having a closer look I needed to remove this from here, because this whole file will go away. To keep the current functionality, we would need to add it to mmio.c, and interestingly we don't do any kind of similar cleanup there for the MMIO regions (probably this is kvmtool exiting anyway, see above). I will see if I can introduce it there, for good measure. Cheers, Andre > > Thanks! > Andre > > > > > > > I can amend the commit message to mention this, or is there anything > > > else I missed? > > > > > > Cheers, > > > Andre > > > > > >>> - > > >>> static const char *to_direction(int direction) > > >>> { > > >>> if (direction == KVM_EXIT_IO_IN) > > >>> @@ -220,16 +205,3 @@ out: > > >>> > > >>> return !kvm->cfg.ioport_debug; > > >>> } > > >>> - > > >>> -int ioport__init(struct kvm *kvm) > > >>> -{ > > >>> - return ioport__setup_arch(kvm); > > >>> -} > > >>> -dev_base_init(ioport__init); > > >>> - > > >>> -int ioport__exit(struct kvm *kvm) > > >>> -{ > > >>> - ioport__unregister_all(); > > >>> - return 0; > > >>> -} > > >>> -dev_base_exit(ioport__exit); > > >>> diff --git a/mips/kvm.c b/mips/kvm.c > > >>> index 26355930..e110e5d5 100644 > > >>> --- a/mips/kvm.c > > >>> +++ b/mips/kvm.c > > >>> @@ -100,11 +100,6 @@ void kvm__irq_trigger(struct kvm *kvm, int irq) > > >>> die_perror("KVM_IRQ_LINE ioctl"); > > >>> } > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm) > > >>> -{ > > >>> - return 0; > > >>> -} > > >>> - > > >>> bool kvm__arch_cpu_supports_vm(void) > > >>> { > > >>> return true; > > >>> diff --git a/powerpc/ioport.c b/powerpc/ioport.c > > >>> index 0c188b61..a5cff4ee 100644 > > >>> --- a/powerpc/ioport.c > > >>> +++ b/powerpc/ioport.c > > >>> @@ -12,12 +12,6 @@ > > >>> > > >>> #include > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm) > > >>> -{ > > >>> - /* PPC has no legacy ioports to set up */ > > >>> - return 0; > > >>> -} > > >>> - > > >>> void ioport__map_irq(u8 *irq) > > >>> { > > >>> } > > >>> diff --git a/x86/ioport.c b/x86/ioport.c > > >>> index 7ad7b8f3..8c5c7699 100644 > > >>> --- a/x86/ioport.c > > >>> +++ b/x86/ioport.c > > >>> @@ -69,7 +69,7 @@ void ioport__map_irq(u8 *irq) > > >>> { > > >>> } > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm) > > >>> +static int ioport__setup_arch(struct kvm *kvm) > > >>> { > > >>> int r; > > >>> > > >>> @@ -150,3 +150,26 @@ int ioport__setup_arch(struct kvm *kvm) > > >>> > > >>> return 0; > > >>> } > > >>> +dev_base_init(ioport__setup_arch); > > >>> + > > >>> +static int ioport__remove_arch(struct kvm *kvm) > > >>> +{ > > >>> + ioport__unregister(kvm, 0x510); > > >>> + ioport__unregister(kvm, 0x402); > > >>> + ioport__unregister(kvm, 0x03D5); > > >>> + ioport__unregister(kvm, 0x03D4); > > >>> + ioport__unregister(kvm, 0x0378); > > >>> + ioport__unregister(kvm, 0x0278); > > >>> + ioport__unregister(kvm, 0x00F0); > > >>> + ioport__unregister(kvm, 0x00ED); > > >>> + ioport__unregister(kvm, IOPORT_DBG); > > >>> + ioport__unregister(kvm, 0x00C0); > > >>> + ioport__unregister(kvm, 0x00A0); > > >>> + ioport__unregister(kvm, 0x0092); > > >>> + ioport__unregister(kvm, 0x0040); > > >>> + ioport__unregister(kvm, 0x0020); > > >>> + ioport__unregister(kvm, 0x0000); > > >>> + > > >>> + return 0; > > >>> +} > > >>> +dev_base_exit(ioport__remove_arch); > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0BA92C433E0 for ; Mon, 22 Feb 2021 10:25:56 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9755764DFD for ; Mon, 22 Feb 2021 10:25:55 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9755764DFD Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: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=u5JsD8UFBzvOb0fkGKmzKrTlHBaqcVxCRVsZctBJZME=; b=RqQehmSWKwOgCvPLYuN0MtxXU WA5baTNWpn9gC51cxE7UN/KJb5WThKKbSPr0uBiqt64riUI0BmSWXqF8pAvHyFPVfnn3qOhtsVFr5 Uy8QrFEL/qucBVk16JJitzsTccMUQsg3+xIQQfN5B1oZEjx7+Lum6+nqtPfZ9DguCJh5WzW/KMOBQ o5OR6S4fWycTgQ9k6YJ35H9D5gvIlcnm1igJK0LrQpiwGUZbv77hWPf/KGGAGbL3lJjlSnE8wRb7M Z5kw2LXz9yCaSYXueqk+dX1u+gtnRNI4/ujjWDcjJHsnAHTNjK1GPSMBLUsAwbL4bSl2XPd6VaIIe JjKFsY/LA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lE8OU-0007Gx-3H; Mon, 22 Feb 2021 10:24:46 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lE8OQ-0007G2-WA for linux-arm-kernel@lists.infradead.org; Mon, 22 Feb 2021 10:24:44 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 99B69D6E; Mon, 22 Feb 2021 02:24:38 -0800 (PST) Received: from slackpad.fritz.box (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6C95E3F73B; Mon, 22 Feb 2021 02:24:37 -0800 (PST) Date: Mon, 22 Feb 2021 10:23:33 +0000 From: Andre Przywara To: Alexandru Elisei Subject: Re: [PATCH kvmtool 01/21] ioport: Remove ioport__setup_arch() Message-ID: <20210222102333.2f1cb9e2@slackpad.fritz.box> In-Reply-To: <20210217155459.3a4bc991@slackpad.fritz.box> References: <20201210142908.169597-1-andre.przywara@arm.com> <20201210142908.169597-2-andre.przywara@arm.com> <814e0cd9-5e54-fade-f05c-80ea2b4a9039@arm.com> <20210211171648.36000cce@slackpad.fritz.box> <111b6cd6-ddf3-ec67-b782-67120be97943@arm.com> <20210217155459.3a4bc991@slackpad.fritz.box> Organization: Arm Ltd. X-Mailer: Claws Mail 3.17.1 (GTK+ 2.24.31; x86_64-slackware-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210222_052443_185639_5F74C982 X-CRM114-Status: GOOD ( 34.14 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kvm@vger.kernel.org, Marc Zyngier , Julien Thierry , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, 17 Feb 2021 16:46:47 +0000 Andre Przywara wrote: > On Thu, 11 Feb 2021 17:32:01 +0000 > Alexandru Elisei wrote: > > Hi, > > > On 2/11/21 5:16 PM, Andre Przywara wrote: > > > On Wed, 10 Feb 2021 17:44:59 +0000 > > > Alexandru Elisei wrote: > > > > > > Hi Alex, > > > > > >> On 12/10/20 2:28 PM, Andre Przywara wrote: > > >>> Since x86 had a special need for registering tons of special I/O ports, > > >>> we had an ioport__setup_arch() callback, to allow each architecture > > >>> to do the same. As it turns out no one uses it beside x86, so we remove > > >>> that unnecessary abstraction. > > >>> > > >>> The generic function was registered via a device_base_init() call, so > > >>> we just do the same for the x86 specific function only, and can remove > > >>> the unneeded ioport__setup_arch(). > > >>> > > >>> Signed-off-by: Andre Przywara > > >>> --- > > >>> arm/ioport.c | 5 ----- > > >>> include/kvm/ioport.h | 1 - > > >>> ioport.c | 28 ---------------------------- > > >>> mips/kvm.c | 5 ----- > > >>> powerpc/ioport.c | 6 ------ > > >>> x86/ioport.c | 25 ++++++++++++++++++++++++- > > >>> 6 files changed, 24 insertions(+), 46 deletions(-) > > >>> > > >>> diff --git a/arm/ioport.c b/arm/ioport.c > > >>> index 2f0feb9a..24092c9d 100644 > > >>> --- a/arm/ioport.c > > >>> +++ b/arm/ioport.c > > >>> @@ -1,11 +1,6 @@ > > >>> #include "kvm/ioport.h" > > >>> #include "kvm/irq.h" > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm) > > >>> -{ > > >>> - return 0; > > >>> -} > > >>> - > > >>> void ioport__map_irq(u8 *irq) > > >>> { > > >>> *irq = irq__alloc_line(); > > >>> diff --git a/include/kvm/ioport.h b/include/kvm/ioport.h > > >>> index 039633f7..d0213541 100644 > > >>> --- a/include/kvm/ioport.h > > >>> +++ b/include/kvm/ioport.h > > >>> @@ -35,7 +35,6 @@ struct ioport_operations { > > >>> enum irq_type)); > > >>> }; > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm); > > >>> void ioport__map_irq(u8 *irq); > > >>> > > >>> int __must_check ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, > > >>> diff --git a/ioport.c b/ioport.c > > >>> index 844a832d..667e8386 100644 > > >>> --- a/ioport.c > > >>> +++ b/ioport.c > > >>> @@ -158,21 +158,6 @@ int ioport__unregister(struct kvm *kvm, u16 port) > > >>> return 0; > > >>> } > > >>> > > >>> -static void ioport__unregister_all(void) > > >>> -{ > > >>> - struct ioport *entry; > > >>> - struct rb_node *rb; > > >>> - struct rb_int_node *rb_node; > > >>> - > > >>> - rb = rb_first(&ioport_tree); > > >>> - while (rb) { > > >>> - rb_node = rb_int(rb); > > >>> - entry = ioport_node(rb_node); > > >>> - ioport_unregister(&ioport_tree, entry); > > >>> - rb = rb_first(&ioport_tree); > > >>> - } > > >>> -} > > >> I get the impression this is a rebasing artifact. The commit message doesn't > > >> mention anything about removing ioport__exit() -> ioport__unregister_all(), and as > > >> far as I can tell it's still needed because there are places other than > > >> ioport__setup_arch() from where ioport__register() is called. > > > I agree that the commit message is a bit thin on this fact, but the > > > functionality of ioport__unregister_all() is now in > > > x86/ioport.c:ioport__remove_arch(). I think removing ioport__init() > > > without removing ioport__exit() as well would look very weird, if not > > > hackish. > > > > Not necessarily. ioport__unregister_all() removes the ioports added by > > x86/ioport.c::ioport__setup_arch(), *plus* ioports added by different devices, > > like serial, rtc, virtio-pci and vfio-pci (which are used by arm/arm64). > > Right, indeed. Not that it really matters, since we are about to exit > anyway, but it looks indeed I need to move this to a generic teardown > method, or actually just keep that part here in this file. > > Will give this a try. Well, now having a closer look I needed to remove this from here, because this whole file will go away. To keep the current functionality, we would need to add it to mmio.c, and interestingly we don't do any kind of similar cleanup there for the MMIO regions (probably this is kvmtool exiting anyway, see above). I will see if I can introduce it there, for good measure. Cheers, Andre > > Thanks! > Andre > > > > > > > I can amend the commit message to mention this, or is there anything > > > else I missed? > > > > > > Cheers, > > > Andre > > > > > >>> - > > >>> static const char *to_direction(int direction) > > >>> { > > >>> if (direction == KVM_EXIT_IO_IN) > > >>> @@ -220,16 +205,3 @@ out: > > >>> > > >>> return !kvm->cfg.ioport_debug; > > >>> } > > >>> - > > >>> -int ioport__init(struct kvm *kvm) > > >>> -{ > > >>> - return ioport__setup_arch(kvm); > > >>> -} > > >>> -dev_base_init(ioport__init); > > >>> - > > >>> -int ioport__exit(struct kvm *kvm) > > >>> -{ > > >>> - ioport__unregister_all(); > > >>> - return 0; > > >>> -} > > >>> -dev_base_exit(ioport__exit); > > >>> diff --git a/mips/kvm.c b/mips/kvm.c > > >>> index 26355930..e110e5d5 100644 > > >>> --- a/mips/kvm.c > > >>> +++ b/mips/kvm.c > > >>> @@ -100,11 +100,6 @@ void kvm__irq_trigger(struct kvm *kvm, int irq) > > >>> die_perror("KVM_IRQ_LINE ioctl"); > > >>> } > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm) > > >>> -{ > > >>> - return 0; > > >>> -} > > >>> - > > >>> bool kvm__arch_cpu_supports_vm(void) > > >>> { > > >>> return true; > > >>> diff --git a/powerpc/ioport.c b/powerpc/ioport.c > > >>> index 0c188b61..a5cff4ee 100644 > > >>> --- a/powerpc/ioport.c > > >>> +++ b/powerpc/ioport.c > > >>> @@ -12,12 +12,6 @@ > > >>> > > >>> #include > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm) > > >>> -{ > > >>> - /* PPC has no legacy ioports to set up */ > > >>> - return 0; > > >>> -} > > >>> - > > >>> void ioport__map_irq(u8 *irq) > > >>> { > > >>> } > > >>> diff --git a/x86/ioport.c b/x86/ioport.c > > >>> index 7ad7b8f3..8c5c7699 100644 > > >>> --- a/x86/ioport.c > > >>> +++ b/x86/ioport.c > > >>> @@ -69,7 +69,7 @@ void ioport__map_irq(u8 *irq) > > >>> { > > >>> } > > >>> > > >>> -int ioport__setup_arch(struct kvm *kvm) > > >>> +static int ioport__setup_arch(struct kvm *kvm) > > >>> { > > >>> int r; > > >>> > > >>> @@ -150,3 +150,26 @@ int ioport__setup_arch(struct kvm *kvm) > > >>> > > >>> return 0; > > >>> } > > >>> +dev_base_init(ioport__setup_arch); > > >>> + > > >>> +static int ioport__remove_arch(struct kvm *kvm) > > >>> +{ > > >>> + ioport__unregister(kvm, 0x510); > > >>> + ioport__unregister(kvm, 0x402); > > >>> + ioport__unregister(kvm, 0x03D5); > > >>> + ioport__unregister(kvm, 0x03D4); > > >>> + ioport__unregister(kvm, 0x0378); > > >>> + ioport__unregister(kvm, 0x0278); > > >>> + ioport__unregister(kvm, 0x00F0); > > >>> + ioport__unregister(kvm, 0x00ED); > > >>> + ioport__unregister(kvm, IOPORT_DBG); > > >>> + ioport__unregister(kvm, 0x00C0); > > >>> + ioport__unregister(kvm, 0x00A0); > > >>> + ioport__unregister(kvm, 0x0092); > > >>> + ioport__unregister(kvm, 0x0040); > > >>> + ioport__unregister(kvm, 0x0020); > > >>> + ioport__unregister(kvm, 0x0000); > > >>> + > > >>> + return 0; > > >>> +} > > >>> +dev_base_exit(ioport__remove_arch); > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel