From: Dave Martin <Dave.Martin@arm.com> To: "Alex Bennée" <alex.bennee@linaro.org> Cc: Okamoto Takayuki <tokamoto@jp.fujitsu.com>, Christoffer Dall <cdall@kernel.org>, Ard Biesheuvel <ard.biesheuvel@linaro.org>, Marc Zyngier <marc.zyngier@arm.com>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com>, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC PATCH 05/16] KVM: arm: Add arch init/uninit hooks Date: Mon, 9 Jul 2018 16:15:49 +0100 [thread overview] Message-ID: <20180709151548.GF9486@e103592.cambridge.arm.com> (raw) In-Reply-To: <877em88yxf.fsf@linaro.org> On Fri, Jul 06, 2018 at 11:02:20AM +0100, Alex Bennée wrote: > > Dave Martin <Dave.Martin@arm.com> writes: > > > In preparation for adding support for SVE in guests on arm64, hooks > > for allocating and freeing additional per-vcpu memory are needed. > > > > kvm_arch_vcpu_setup() could be used for allocation, but this > > function is not clearly balanced by un "unsetup" function, making > > Isn't that a double negative there? Surely it would be balanced by a > kvm_arch_vcpu_unsetup() or possibly better named function. Yes, but there is no such function, and it wasn't clear what the semantics of the existing hooks is supposed to be... so I didn't feel comfortable adding an _unsetup(). I was trying to be minimally invasive while I got things working... > > it unclear where memory allocated in this function should be freed. > > > > To keep things simple, this patch defines backend hooks > > kvm_arm_arch_vcpu_{,un}unint(), and plumbs them in appropriately. > > Is {,un} a notation for dropping un? This might be why I'm confused. I > would have written it as kvm_arm_arch_vcpu_[un]init() or even > kvm_arm_arch_vcpu_[init|uninit]. That should be kvm_arm_arch_vcpu_{,un}init(). Whether this is readily understood by people is another question. This is the bash brace-expansion syntax which I'm in the habit of using, partly because it looks nothing like C syntax, thus "reducing" confusion. Personally I make heavy use of this in the shell, like mv .config{,.old} etc. But that's me. Maybe other people don't. Too obscure? I have a number of patches that follow this convention upstream. > > The exusting kvm_arch_vcpu_init() function now calls > > /existing/ > > > kvm_arm_arch_vcpu_init(), while an explicit kvm_arch_vcpu_uninit() > > is added which current does nothing except to call > > kvm_arm_arch_vcpu_uninit(). > > OK I'm a little confused by this. It seems to me that KVM already has > the provision for an init/uninit. What does the extra level on > indirection buy you that keeping the static inline > kvm_arm_arch_vcpu_uninit in arm/kvm_host.h and a concrete implementation > in arm64/kvm/guest.c doesn't? There isn't an intentional extra level of indirection, but the existing code feels strangely factored due to the somewhat random use of prefixes in function names (i.e., kvm_arch_*() is sometimes a hook from KVM core into virt/kvm/arm/, some a hook from virt/kvm/arm/ into the arm or arm64 backend, and sometimes a hook from KVM core directly into the arm or arm64 backend). So, I wasn't really sure where to put things initially. This patch was always a bit of a bodge, and the series as posted doesn't fully make use of it anyway... so I'll need to revisit. Cheers ---Dave
WARNING: multiple messages have this Message-ID (diff)
From: Dave.Martin@arm.com (Dave Martin) To: linux-arm-kernel@lists.infradead.org Subject: [RFC PATCH 05/16] KVM: arm: Add arch init/uninit hooks Date: Mon, 9 Jul 2018 16:15:49 +0100 [thread overview] Message-ID: <20180709151548.GF9486@e103592.cambridge.arm.com> (raw) In-Reply-To: <877em88yxf.fsf@linaro.org> On Fri, Jul 06, 2018 at 11:02:20AM +0100, Alex Benn?e wrote: > > Dave Martin <Dave.Martin@arm.com> writes: > > > In preparation for adding support for SVE in guests on arm64, hooks > > for allocating and freeing additional per-vcpu memory are needed. > > > > kvm_arch_vcpu_setup() could be used for allocation, but this > > function is not clearly balanced by un "unsetup" function, making > > Isn't that a double negative there? Surely it would be balanced by a > kvm_arch_vcpu_unsetup() or possibly better named function. Yes, but there is no such function, and it wasn't clear what the semantics of the existing hooks is supposed to be... so I didn't feel comfortable adding an _unsetup(). I was trying to be minimally invasive while I got things working... > > it unclear where memory allocated in this function should be freed. > > > > To keep things simple, this patch defines backend hooks > > kvm_arm_arch_vcpu_{,un}unint(), and plumbs them in appropriately. > > Is {,un} a notation for dropping un? This might be why I'm confused. I > would have written it as kvm_arm_arch_vcpu_[un]init() or even > kvm_arm_arch_vcpu_[init|uninit]. That should be kvm_arm_arch_vcpu_{,un}init(). Whether this is readily understood by people is another question. This is the bash brace-expansion syntax which I'm in the habit of using, partly because it looks nothing like C syntax, thus "reducing" confusion. Personally I make heavy use of this in the shell, like mv .config{,.old} etc. But that's me. Maybe other people don't. Too obscure? I have a number of patches that follow this convention upstream. > > The exusting kvm_arch_vcpu_init() function now calls > > /existing/ > > > kvm_arm_arch_vcpu_init(), while an explicit kvm_arch_vcpu_uninit() > > is added which current does nothing except to call > > kvm_arm_arch_vcpu_uninit(). > > OK I'm a little confused by this. It seems to me that KVM already has > the provision for an init/uninit. What does the extra level on > indirection buy you that keeping the static inline > kvm_arm_arch_vcpu_uninit in arm/kvm_host.h and a concrete implementation > in arm64/kvm/guest.c doesn't? There isn't an intentional extra level of indirection, but the existing code feels strangely factored due to the somewhat random use of prefixes in function names (i.e., kvm_arch_*() is sometimes a hook from KVM core into virt/kvm/arm/, some a hook from virt/kvm/arm/ into the arm or arm64 backend, and sometimes a hook from KVM core directly into the arm or arm64 backend). So, I wasn't really sure where to put things initially. This patch was always a bit of a bodge, and the series as posted doesn't fully make use of it anyway... so I'll need to revisit. Cheers ---Dave
next prev parent reply other threads:[~2018-07-09 15:03 UTC|newest] Thread overview: 178+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-06-21 14:57 [RFC PATCH 00/16] KVM: arm64: Initial support for SVE guests Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-06-21 14:57 ` [RFC PATCH 01/16] arm64: fpsimd: Always set TIF_FOREIGN_FPSTATE on task state flush Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-06 9:07 ` Alex Bennée 2018-07-06 9:07 ` Alex Bennée 2018-06-21 14:57 ` [RFC PATCH 02/16] KVM: arm64: Delete orphaned declaration for __fpsimd_enabled() Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-06 9:08 ` Alex Bennée 2018-07-06 9:08 ` Alex Bennée 2018-06-21 14:57 ` [RFC PATCH 03/16] KVM: arm64: Refactor kvm_arm_num_regs() for easier maintenance Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-06 9:20 ` Alex Bennée 2018-07-06 9:20 ` Alex Bennée 2018-06-21 14:57 ` [RFC PATCH 04/16] KVM: arm64: Add missing #include of <linux/bitmap.h> to kvm_host.h Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-06 9:21 ` Alex Bennée 2018-07-06 9:21 ` Alex Bennée 2018-06-21 14:57 ` [RFC PATCH 05/16] KVM: arm: Add arch init/uninit hooks Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-06 10:02 ` Alex Bennée 2018-07-06 10:02 ` Alex Bennée 2018-07-09 15:15 ` Dave Martin [this message] 2018-07-09 15:15 ` Dave Martin 2018-06-21 14:57 ` [RFC PATCH 06/16] arm64/sve: Determine virtualisation-friendly vector lengths Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-06 13:20 ` Marc Zyngier 2018-07-06 13:20 ` Marc Zyngier 2018-06-21 14:57 ` [RFC PATCH 07/16] arm64/sve: Enable SVE state tracking for non-task contexts Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-25 13:58 ` Alex Bennée 2018-07-25 13:58 ` Alex Bennée 2018-07-25 14:39 ` Dave Martin 2018-07-25 14:39 ` Dave Martin 2018-06-21 14:57 ` [RFC PATCH 08/16] KVM: arm64: Support dynamically hideable system registers Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-25 14:12 ` Alex Bennée 2018-07-25 14:12 ` Alex Bennée 2018-07-25 14:36 ` Dave Martin 2018-07-25 14:36 ` Dave Martin 2018-07-25 15:41 ` Alex Bennée 2018-07-25 15:41 ` Alex Bennée 2018-07-26 12:53 ` Dave Martin 2018-07-26 12:53 ` Dave Martin 2018-08-07 19:20 ` Christoffer Dall 2018-08-07 19:20 ` Christoffer Dall 2018-08-08 8:33 ` Dave Martin 2018-08-08 8:33 ` Dave Martin 2018-06-21 14:57 ` [RFC PATCH 09/16] KVM: arm64: Allow ID registers to by dynamically read-as-zero Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-25 15:46 ` Alex Bennée 2018-07-25 15:46 ` Alex Bennée 2018-08-06 13:03 ` Christoffer Dall 2018-08-06 13:03 ` Christoffer Dall 2018-08-07 11:09 ` Dave Martin 2018-08-07 11:09 ` Dave Martin 2018-08-07 19:35 ` Christoffer Dall 2018-08-07 19:35 ` Christoffer Dall 2018-08-08 9:11 ` Dave Martin 2018-08-08 9:11 ` Dave Martin 2018-08-08 9:58 ` Christoffer Dall 2018-08-08 9:58 ` Christoffer Dall 2018-08-08 14:03 ` Peter Maydell 2018-08-08 14:03 ` Peter Maydell 2018-08-09 10:19 ` Dave Martin 2018-08-09 10:19 ` Dave Martin 2018-06-21 14:57 ` [RFC PATCH 10/16] KVM: arm64: Add a vcpu flag to control SVE visibility for the guest Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-19 11:08 ` Andrew Jones 2018-07-19 11:08 ` Andrew Jones 2018-07-25 11:41 ` Dave Martin 2018-07-25 11:41 ` Dave Martin 2018-07-25 13:43 ` Andrew Jones 2018-07-25 13:43 ` Andrew Jones 2018-07-25 14:41 ` Dave Martin 2018-07-25 14:41 ` Dave Martin 2018-07-19 15:02 ` Andrew Jones 2018-07-19 15:02 ` Andrew Jones 2018-07-25 11:48 ` Dave Martin 2018-07-25 11:48 ` Dave Martin 2018-06-21 14:57 ` [RFC PATCH 11/16] KVM: arm64/sve: System register context switch and access support Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-19 11:11 ` Andrew Jones 2018-07-19 11:11 ` Andrew Jones 2018-07-25 11:45 ` Dave Martin 2018-07-25 11:45 ` Dave Martin 2018-06-21 14:57 ` [RFC PATCH 12/16] KVM: arm64/sve: Context switch the SVE registers Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-19 13:13 ` Andrew Jones 2018-07-19 13:13 ` Andrew Jones 2018-07-25 11:50 ` Dave Martin 2018-07-25 11:50 ` Dave Martin 2018-07-25 13:57 ` Andrew Jones 2018-07-25 13:57 ` Andrew Jones 2018-07-25 14:12 ` Dave Martin 2018-07-25 14:12 ` Dave Martin 2018-08-06 13:19 ` Christoffer Dall 2018-08-06 13:19 ` Christoffer Dall 2018-08-07 11:15 ` Dave Martin 2018-08-07 11:15 ` Dave Martin 2018-08-07 19:43 ` Christoffer Dall 2018-08-07 19:43 ` Christoffer Dall 2018-08-08 8:23 ` Dave Martin 2018-08-08 8:23 ` Dave Martin 2018-06-21 14:57 ` [RFC PATCH 13/16] KVM: Allow 2048-bit register access via KVM_{GET, SET}_ONE_REG Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-25 15:58 ` Alex Bennée 2018-07-25 15:58 ` Alex Bennée 2018-07-26 12:58 ` Dave Martin 2018-07-26 12:58 ` Dave Martin 2018-07-26 13:55 ` Alex Bennée 2018-07-26 13:55 ` Alex Bennée 2018-07-27 9:26 ` Dave Martin 2018-07-27 9:26 ` Dave Martin 2018-06-21 14:57 ` [RFC PATCH 14/16] KVM: arm64/sve: Add SVE support to register access ioctl interface Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-19 13:04 ` Andrew Jones 2018-07-19 13:04 ` Andrew Jones 2018-07-25 14:06 ` Dave Martin 2018-07-25 14:06 ` Dave Martin 2018-07-25 17:20 ` Andrew Jones 2018-07-25 17:20 ` Andrew Jones 2018-07-26 13:10 ` Dave Martin 2018-07-26 13:10 ` Dave Martin 2018-08-03 14:57 ` Dave Martin 2018-08-03 14:57 ` Dave Martin 2018-08-03 15:11 ` Andrew Jones 2018-08-03 15:11 ` Andrew Jones 2018-08-03 15:38 ` Dave Martin 2018-08-03 15:38 ` Dave Martin 2018-08-06 13:25 ` Christoffer Dall 2018-08-06 13:25 ` Christoffer Dall 2018-08-07 11:17 ` Dave Martin 2018-08-07 11:17 ` Dave Martin 2018-06-21 14:57 ` [RFC PATCH 15/16] KVM: arm64: Enumerate SVE register indices for KVM_GET_REG_LIST Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-19 14:12 ` Andrew Jones 2018-07-19 14:12 ` Andrew Jones 2018-07-25 14:50 ` Dave Martin 2018-07-25 14:50 ` Dave Martin 2018-06-21 14:57 ` [RFC PATCH 16/16] KVM: arm64/sve: Report and enable SVE API extensions for userspace Dave Martin 2018-06-21 14:57 ` Dave Martin 2018-07-19 14:59 ` Andrew Jones 2018-07-19 14:59 ` Andrew Jones 2018-07-25 15:27 ` Dave Martin 2018-07-25 15:27 ` Dave Martin 2018-07-25 16:52 ` Andrew Jones 2018-07-25 16:52 ` Andrew Jones 2018-07-26 13:18 ` Dave Martin 2018-07-26 13:18 ` Dave Martin 2018-08-06 13:41 ` Christoffer Dall 2018-08-06 13:41 ` Christoffer Dall 2018-08-07 11:23 ` Dave Martin 2018-08-07 11:23 ` Dave Martin 2018-08-07 20:08 ` Christoffer Dall 2018-08-07 20:08 ` Christoffer Dall 2018-08-08 8:30 ` Dave Martin 2018-08-08 8:30 ` Dave Martin 2018-07-19 15:24 ` Andrew Jones 2018-07-19 15:24 ` Andrew Jones 2018-07-26 13:23 ` Dave Martin 2018-07-26 13:23 ` Dave Martin 2018-07-06 8:22 ` [RFC PATCH 00/16] KVM: arm64: Initial support for SVE guests Alex Bennée 2018-07-06 8:22 ` Alex Bennée 2018-07-06 9:05 ` Dave Martin 2018-07-06 9:05 ` Dave Martin 2018-07-06 9:20 ` Alex Bennée 2018-07-06 9:20 ` Alex Bennée 2018-07-06 9:23 ` Peter Maydell 2018-07-06 9:23 ` Peter Maydell 2018-07-06 10:11 ` Alex Bennée 2018-07-06 10:11 ` Alex Bennée 2018-07-06 10:14 ` Peter Maydell 2018-07-06 10:14 ` Peter Maydell 2018-08-06 13:05 ` Christoffer Dall 2018-08-06 13:05 ` Christoffer Dall 2018-08-07 11:18 ` Dave Martin 2018-08-07 11:18 ` Dave Martin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180709151548.GF9486@e103592.cambridge.arm.com \ --to=dave.martin@arm.com \ --cc=alex.bennee@linaro.org \ --cc=ard.biesheuvel@linaro.org \ --cc=catalin.marinas@arm.com \ --cc=cdall@kernel.org \ --cc=kvmarm@lists.cs.columbia.edu \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=marc.zyngier@arm.com \ --cc=tokamoto@jp.fujitsu.com \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.