Patchwork [v7,21/27] KVM: arm/arm64: Add hook for arch-specific KVM initialisation

login
register
mail settings
Submitter Dave Martin
Date March 29, 2019, 1 p.m.
Message ID <1553864452-15080-22-git-send-email-Dave.Martin@arm.com>
Download mbox | patch
Permalink /patch/761183/
State New
Headers show

Comments

Dave Martin - March 29, 2019, 1 p.m.
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(+)
Andrew Jones - April 4, 2019, 2:25 p.m.
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
Dave Martin - April 4, 2019, 2:53 p.m.
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
Andrew Jones - April 4, 2019, 4:33 p.m.
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
Dave Martin - April 5, 2019, 9:36 a.m.
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
Andrew Jones - April 5, 2019, 10:40 a.m.
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
Dave Martin - April 5, 2019, 11:14 a.m.
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

Patch

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)