| Message ID | 1553864452-15080-22-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: SVE guest support | expand |
On Fri, Mar 29, 2019 at 01:00:46PM +0000, Dave Martin wrote: > This patch adds a kvm_arm_init_arch_resources() hook to perform > subarch-specific initialisation when starting up KVM. > > This will be used in a subsequent patch for global SVE-related > setup on arm64. > > No functional change. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Reviewed-by: Julien Thierry <julien.thierry@arm.com> > Tested-by: zhang.lei <zhang.lei@jp.fujitsu.com> > --- > arch/arm/include/asm/kvm_host.h | 2 ++ > arch/arm64/include/asm/kvm_host.h | 2 ++ > virt/kvm/arm/arm.c | 4 ++++ > 3 files changed, 8 insertions(+) > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index 770d732..a49ee01 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -53,6 +53,8 @@ > > DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use); > > +static inline int kvm_arm_init_arch_resources(void) { return 0; } > + > u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode); > int __attribute_const__ kvm_target_cpu(void); > int kvm_reset_vcpu(struct kvm_vcpu *vcpu); > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 205438a..3e89509 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -58,6 +58,8 @@ > > DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use); > > +static inline int kvm_arm_init_arch_resources(void) { return 0; } > + > int __attribute_const__ kvm_target_cpu(void); > int kvm_reset_vcpu(struct kvm_vcpu *vcpu); > int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext); > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index 99c3738..c69e137 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -1664,6 +1664,10 @@ int kvm_arch_init(void *opaque) > if (err) > return err; > > + err = kvm_arm_init_arch_resources(); > + if (err) > + return err; > + > if (!in_hyp_mode) { > err = init_hyp_mode(); > if (err) > -- > 2.1.4 > It's not clear to me from the commit message why init_common_resources() won't work for this. Maybe it'll be more clear as I continue the review. drew
On Thu, Apr 04, 2019 at 04:25:28PM +0200, Andrew Jones wrote: > On Fri, Mar 29, 2019 at 01:00:46PM +0000, Dave Martin wrote: > > This patch adds a kvm_arm_init_arch_resources() hook to perform > > subarch-specific initialisation when starting up KVM. > > > > This will be used in a subsequent patch for global SVE-related > > setup on arm64. > > > > No functional change. > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > Reviewed-by: Julien Thierry <julien.thierry@arm.com> > > Tested-by: zhang.lei <zhang.lei@jp.fujitsu.com> > > --- > > arch/arm/include/asm/kvm_host.h | 2 ++ > > arch/arm64/include/asm/kvm_host.h | 2 ++ > > virt/kvm/arm/arm.c | 4 ++++ > > 3 files changed, 8 insertions(+) > > > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > > index 770d732..a49ee01 100644 > > --- a/arch/arm/include/asm/kvm_host.h > > +++ b/arch/arm/include/asm/kvm_host.h > > @@ -53,6 +53,8 @@ > > > > DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use); > > > > +static inline int kvm_arm_init_arch_resources(void) { return 0; } > > + > > u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode); > > int __attribute_const__ kvm_target_cpu(void); > > int kvm_reset_vcpu(struct kvm_vcpu *vcpu); > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index 205438a..3e89509 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -58,6 +58,8 @@ > > > > DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use); > > > > +static inline int kvm_arm_init_arch_resources(void) { return 0; } > > + > > int __attribute_const__ kvm_target_cpu(void); > > int kvm_reset_vcpu(struct kvm_vcpu *vcpu); > > int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext); > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > > index 99c3738..c69e137 100644 > > --- a/virt/kvm/arm/arm.c > > +++ b/virt/kvm/arm/arm.c > > @@ -1664,6 +1664,10 @@ int kvm_arch_init(void *opaque) > > if (err) > > return err; > > > > + err = kvm_arm_init_arch_resources(); > > + if (err) > > + return err; > > + > > if (!in_hyp_mode) { > > err = init_hyp_mode(); > > if (err) > > -- > > 2.1.4 > > > > It's not clear to me from the commit message why init_common_resources() > won't work for this. Maybe it'll be more clear as I continue the review. init_common_resources() is for stuff common to arm and arm64. kvm_arm_init_arch_resources() is intended for stuff specific to the particular arch backend. Maybe there is a better name for this. Cheers ---Dave
On Thu, Apr 04, 2019 at 03:53:55PM +0100, Dave Martin wrote: > On Thu, Apr 04, 2019 at 04:25:28PM +0200, Andrew Jones wrote: > > On Fri, Mar 29, 2019 at 01:00:46PM +0000, Dave Martin wrote: > > > This patch adds a kvm_arm_init_arch_resources() hook to perform > > > subarch-specific initialisation when starting up KVM. > > > > > > This will be used in a subsequent patch for global SVE-related > > > setup on arm64. > > > > > > No functional change. > > > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > Reviewed-by: Julien Thierry <julien.thierry@arm.com> > > > Tested-by: zhang.lei <zhang.lei@jp.fujitsu.com> > > > --- > > > arch/arm/include/asm/kvm_host.h | 2 ++ > > > arch/arm64/include/asm/kvm_host.h | 2 ++ > > > virt/kvm/arm/arm.c | 4 ++++ > > > 3 files changed, 8 insertions(+) > > > > > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > > > index 770d732..a49ee01 100644 > > > --- a/arch/arm/include/asm/kvm_host.h > > > +++ b/arch/arm/include/asm/kvm_host.h > > > @@ -53,6 +53,8 @@ > > > > > > DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use); > > > > > > +static inline int kvm_arm_init_arch_resources(void) { return 0; } > > > + > > > u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode); > > > int __attribute_const__ kvm_target_cpu(void); > > > int kvm_reset_vcpu(struct kvm_vcpu *vcpu); > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > > index 205438a..3e89509 100644 > > > --- a/arch/arm64/include/asm/kvm_host.h > > > +++ b/arch/arm64/include/asm/kvm_host.h > > > @@ -58,6 +58,8 @@ > > > > > > DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use); > > > > > > +static inline int kvm_arm_init_arch_resources(void) { return 0; } > > > + > > > int __attribute_const__ kvm_target_cpu(void); > > > int kvm_reset_vcpu(struct kvm_vcpu *vcpu); > > > int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext); > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > > > index 99c3738..c69e137 100644 > > > --- a/virt/kvm/arm/arm.c > > > +++ b/virt/kvm/arm/arm.c > > > @@ -1664,6 +1664,10 @@ int kvm_arch_init(void *opaque) > > > if (err) > > > return err; > > > > > > + err = kvm_arm_init_arch_resources(); > > > + if (err) > > > + return err; > > > + > > > if (!in_hyp_mode) { > > > err = init_hyp_mode(); > > > if (err) > > > -- > > > 2.1.4 > > > > > > > It's not clear to me from the commit message why init_common_resources() > > won't work for this. Maybe it'll be more clear as I continue the review. > > init_common_resources() is for stuff common to arm and arm64. Well currently init_common_resources() only calls kvm_set_ipa_limit(), which isn't implemented for arm. So if there was a plan to only use that function for init that actually does something on both, it doesn't. > > kvm_arm_init_arch_resources() is intended for stuff specific to the > particular arch backend. I'm not sure we need that yet. We just need an arm setup sve stub like arm's kvm_set_ipa_limit() stub. OTOH, if we have to keep adding stubs to arm we should probably have something like kvm_arm_init_arch_resources() instead, and kvm_set_ipa_limit() should be moved inside the arm64 one and the arm ipa limit stub can go. Then since init_common_resources() would no longer be used, it could just be deleted. > > Maybe there is a better name for this. > The name is fine for me. Thanks, drew
On Thu, Apr 04, 2019 at 06:33:08PM +0200, Andrew Jones wrote: > On Thu, Apr 04, 2019 at 03:53:55PM +0100, Dave Martin wrote: > > On Thu, Apr 04, 2019 at 04:25:28PM +0200, Andrew Jones wrote: > > > On Fri, Mar 29, 2019 at 01:00:46PM +0000, Dave Martin wrote: [...] > > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > > > > index 99c3738..c69e137 100644 > > > > --- a/virt/kvm/arm/arm.c > > > > +++ b/virt/kvm/arm/arm.c > > > > @@ -1664,6 +1664,10 @@ int kvm_arch_init(void *opaque) > > > > if (err) > > > > return err; > > > > > > > > + err = kvm_arm_init_arch_resources(); > > > > + if (err) > > > > + return err; > > > > + > > > > if (!in_hyp_mode) { > > > > err = init_hyp_mode(); > > > > if (err) > > > > -- > > > > 2.1.4 > > > > > > > > > > It's not clear to me from the commit message why init_common_resources() > > > won't work for this. Maybe it'll be more clear as I continue the review. > > > > init_common_resources() is for stuff common to arm and arm64. > > Well currently init_common_resources() only calls kvm_set_ipa_limit(), > which isn't implemented for arm. So if there was a plan to only use > that function for init that actually does something on both, it doesn't. Hmmm, perhaps I was wishfully interpreting the word "common" to mean what I would like it to mean... > > kvm_arm_init_arch_resources() is intended for stuff specific to the > > particular arch backend. > > I'm not sure we need that yet. We just need an arm setup sve stub like > arm's kvm_set_ipa_limit() stub. OTOH, if we have to keep adding stubs > to arm we should probably have something like > kvm_arm_init_arch_resources() instead, and kvm_set_ipa_limit() should > be moved inside the arm64 one and the arm ipa limit stub can go. Then > since init_common_resources() would no longer be used, it could just > be deleted. OK, for simplicity I may call the sve setup directly as you suggest, and make an arm stub for it: that feels a bit weird, but there is precedent. If we end up accumulating a lot of these, we can revisit it and maybe invent something like kvm_arm_init_arch_resources() at that point. Does that sound reasonable? Cheers ---Dave
On Fri, Apr 05, 2019 at 10:36:24AM +0100, Dave Martin wrote: > On Thu, Apr 04, 2019 at 06:33:08PM +0200, Andrew Jones wrote: > > On Thu, Apr 04, 2019 at 03:53:55PM +0100, Dave Martin wrote: > > > On Thu, Apr 04, 2019 at 04:25:28PM +0200, Andrew Jones wrote: > > > > On Fri, Mar 29, 2019 at 01:00:46PM +0000, Dave Martin wrote: > > [...] > > > > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > > > > > index 99c3738..c69e137 100644 > > > > > --- a/virt/kvm/arm/arm.c > > > > > +++ b/virt/kvm/arm/arm.c > > > > > @@ -1664,6 +1664,10 @@ int kvm_arch_init(void *opaque) > > > > > if (err) > > > > > return err; > > > > > > > > > > + err = kvm_arm_init_arch_resources(); > > > > > + if (err) > > > > > + return err; > > > > > + > > > > > if (!in_hyp_mode) { > > > > > err = init_hyp_mode(); > > > > > if (err) > > > > > -- > > > > > 2.1.4 > > > > > > > > > > > > > It's not clear to me from the commit message why init_common_resources() > > > > won't work for this. Maybe it'll be more clear as I continue the review. > > > > > > init_common_resources() is for stuff common to arm and arm64. > > > > Well currently init_common_resources() only calls kvm_set_ipa_limit(), > > which isn't implemented for arm. So if there was a plan to only use > > that function for init that actually does something on both, it doesn't. > > Hmmm, perhaps I was wishfully interpreting the word "common" to mean > what I would like it to mean... > > > > kvm_arm_init_arch_resources() is intended for stuff specific to the > > > particular arch backend. > > > > I'm not sure we need that yet. We just need an arm setup sve stub like > > arm's kvm_set_ipa_limit() stub. OTOH, if we have to keep adding stubs > > to arm we should probably have something like > > kvm_arm_init_arch_resources() instead, and kvm_set_ipa_limit() should > > be moved inside the arm64 one and the arm ipa limit stub can go. Then > > since init_common_resources() would no longer be used, it could just > > be deleted. > > OK, for simplicity I may call the sve setup directly as you suggest, and > make an arm stub for it: that feels a bit weird, but there is precedent. > > If we end up accumulating a lot of these, we can revisit it and maybe > invent something like kvm_arm_init_arch_resources() at that point. > > Does that sound reasonable? Yup. Sounds good to me. Thanks, drew
On Fri, Apr 05, 2019 at 12:40:57PM +0200, Andrew Jones wrote: > On Fri, Apr 05, 2019 at 10:36:24AM +0100, Dave Martin wrote: > > On Thu, Apr 04, 2019 at 06:33:08PM +0200, Andrew Jones wrote: > > > On Thu, Apr 04, 2019 at 03:53:55PM +0100, Dave Martin wrote: > > > > On Thu, Apr 04, 2019 at 04:25:28PM +0200, Andrew Jones wrote: > > > > > On Fri, Mar 29, 2019 at 01:00:46PM +0000, Dave Martin wrote: > > > > [...] > > > > > > > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > > > > > > index 99c3738..c69e137 100644 > > > > > > --- a/virt/kvm/arm/arm.c > > > > > > +++ b/virt/kvm/arm/arm.c > > > > > > @@ -1664,6 +1664,10 @@ int kvm_arch_init(void *opaque) > > > > > > if (err) > > > > > > return err; > > > > > > > > > > > > + err = kvm_arm_init_arch_resources(); > > > > > > + if (err) > > > > > > + return err; > > > > > > + > > > > > > if (!in_hyp_mode) { > > > > > > err = init_hyp_mode(); > > > > > > if (err) > > > > > > -- > > > > > > 2.1.4 > > > > > > > > > > > > > > > > It's not clear to me from the commit message why init_common_resources() > > > > > won't work for this. Maybe it'll be more clear as I continue the review. > > > > > > > > init_common_resources() is for stuff common to arm and arm64. > > > > > > Well currently init_common_resources() only calls kvm_set_ipa_limit(), > > > which isn't implemented for arm. So if there was a plan to only use > > > that function for init that actually does something on both, it doesn't. > > > > Hmmm, perhaps I was wishfully interpreting the word "common" to mean > > what I would like it to mean... > > > > > > kvm_arm_init_arch_resources() is intended for stuff specific to the > > > > particular arch backend. > > > > > > I'm not sure we need that yet. We just need an arm setup sve stub like > > > arm's kvm_set_ipa_limit() stub. OTOH, if we have to keep adding stubs > > > to arm we should probably have something like > > > kvm_arm_init_arch_resources() instead, and kvm_set_ipa_limit() should > > > be moved inside the arm64 one and the arm ipa limit stub can go. Then > > > since init_common_resources() would no longer be used, it could just > > > be deleted. > > > > OK, for simplicity I may call the sve setup directly as you suggest, and > > make an arm stub for it: that feels a bit weird, but there is precedent. > > > > If we end up accumulating a lot of these, we can revisit it and maybe > > invent something like kvm_arm_init_arch_resources() at that point. > > > > Does that sound reasonable? > > Yup. Sounds good to me. OK, can do Cheers ---Dave
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index 770d732..a49ee01 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -53,6 +53,8 @@ DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use); +static inline int kvm_arm_init_arch_resources(void) { return 0; } + u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode); int __attribute_const__ kvm_target_cpu(void); int kvm_reset_vcpu(struct kvm_vcpu *vcpu); diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 205438a..3e89509 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -58,6 +58,8 @@ DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use); +static inline int kvm_arm_init_arch_resources(void) { return 0; } + int __attribute_const__ kvm_target_cpu(void); int kvm_reset_vcpu(struct kvm_vcpu *vcpu); int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext); diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c index 99c3738..c69e137 100644 --- a/virt/kvm/arm/arm.c +++ b/virt/kvm/arm/arm.c @@ -1664,6 +1664,10 @@ int kvm_arch_init(void *opaque) if (err) return err; + err = kvm_arm_init_arch_resources(); + if (err) + return err; + if (!in_hyp_mode) { err = init_hyp_mode(); if (err)