| Message ID | 1538141967-15375-6-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Initial support for SVE guests | expand |
On Fri, Sep 28, 2018 at 02:39:09PM +0100, Dave Martin wrote: > In preparation for adding support for SVE in guests on arm64, a > hook is needed for freeing additional per-vcpu memory when a vcpu > is freed. Can this commit motivate why we can't do the work in kvm_arch_vcpu_free, which we use for freeing other data structures? (Presumably, uninit is needed when you need to do something at the very last step after releasing the struct pid. Thanks, Christoffer > > x86 already uses the kvm_arch_vcpu_uninit() hook for a similar > purpose, so this patch populates the same hook for arm. Since SVE > is specific to arm64, a subsidiary hook kvm_arm_arch_vcpu_uninit() > is added (with trivial implementations for now) to enable separate > specialisation for arm and arm64. > > No functional change. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > --- > > Changes since RFCv1: > > * The vcpu _init_ hook that was added by the former version of this > patch was never used for anything, so it is gone from this version. > --- > arch/arm/include/asm/kvm_host.h | 3 ++- > arch/arm64/include/asm/kvm_host.h | 3 ++- > virt/kvm/arm/arm.c | 5 +++++ > 3 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index 3ad482d..c36760b 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -288,10 +288,11 @@ struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr); > static inline bool kvm_arch_check_sve_has_vhe(void) { return true; } > static inline void kvm_arch_hardware_unsetup(void) {} > static inline void kvm_arch_sync_events(struct kvm *kvm) {} > -static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {} > > +static inline void kvm_arm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} > + > static inline void kvm_arm_init_debug(void) {} > static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} > static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 6316a57..d4b65414 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -425,10 +425,11 @@ static inline bool kvm_arch_check_sve_has_vhe(void) > > static inline void kvm_arch_hardware_unsetup(void) {} > static inline void kvm_arch_sync_events(struct kvm *kvm) {} > -static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {} > > +static inline void kvm_arm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} > + > void kvm_arm_init_debug(void); > void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); > void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index c92053b..1418af9 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -358,6 +358,11 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) > return kvm_vgic_vcpu_init(vcpu); > } > > +void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) > +{ > + kvm_arm_arch_vcpu_uninit(vcpu); > +} > + > void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) > { > int *last_ran; > -- > 2.1.4 > > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
On Fri, Nov 02, 2018 at 09:05:36AM +0100, Christoffer Dall wrote: > On Fri, Sep 28, 2018 at 02:39:09PM +0100, Dave Martin wrote: > > In preparation for adding support for SVE in guests on arm64, a > > hook is needed for freeing additional per-vcpu memory when a vcpu > > is freed. > > Can this commit motivate why we can't do the work in kvm_arch_vcpu_free, > which we use for freeing other data structures? > > (Presumably, uninit is needed when you need to do something at the very > last step after releasing the struct pid. It wasn't to do with that. Rather, the division of responsibility between the vcpu_uninit and vcpu_free paths is not very clear. In the earlier version of the series, I think SVE state may have been allocated rather early and we may have needed to free it in the failure path of kvm_arch_vcpu_create() (which just calls kvm_vcpu_uninit()). (Alternatively, I may just have been wrong.) Now, the vcpu must be fully created before the KVM_ARM_SVE_CONFIG ioctl on it (which is what allocates sve_state) can succeed anyway. So the distinction between these two teardown phases is probably no longer important. I'll see whether I can get rid of this hook and free the SVE state in kvm_arch_vcpu_free() instead. Does that make sense? Cheers ---Dave
On Thu, Nov 15, 2018 at 04:40:31PM +0000, Dave Martin wrote: > On Fri, Nov 02, 2018 at 09:05:36AM +0100, Christoffer Dall wrote: > > On Fri, Sep 28, 2018 at 02:39:09PM +0100, Dave Martin wrote: > > > In preparation for adding support for SVE in guests on arm64, a > > > hook is needed for freeing additional per-vcpu memory when a vcpu > > > is freed. > > > > Can this commit motivate why we can't do the work in kvm_arch_vcpu_free, > > which we use for freeing other data structures? > > > > (Presumably, uninit is needed when you need to do something at the very > > last step after releasing the struct pid. > > It wasn't to do with that. > > Rather, the division of responsibility between the vcpu_uninit and > vcpu_free paths is not very clear. > > In the earlier version of the series, I think SVE state may have been > allocated rather early and we may have needed to free it in the failure > path of kvm_arch_vcpu_create() (which just calls kvm_vcpu_uninit()). > (Alternatively, I may just have been wrong.) > > Now, the vcpu must be fully created before the KVM_ARM_SVE_CONFIG ioctl > on it (which is what allocates sve_state) can succeed anyway. > > So the distinction between these two teardown phases is probably no > longer important. > > I'll see whether I can get rid of this hook and free the SVE state in > kvm_arch_vcpu_free() instead. > > Does that make sense? > Yes, thanks. Christoffer
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index 3ad482d..c36760b 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -288,10 +288,11 @@ struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr); static inline bool kvm_arch_check_sve_has_vhe(void) { return true; } static inline void kvm_arch_hardware_unsetup(void) {} static inline void kvm_arch_sync_events(struct kvm *kvm) {} -static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {} +static inline void kvm_arm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} + static inline void kvm_arm_init_debug(void) {} static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 6316a57..d4b65414 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -425,10 +425,11 @@ static inline bool kvm_arch_check_sve_has_vhe(void) static inline void kvm_arch_hardware_unsetup(void) {} static inline void kvm_arch_sync_events(struct kvm *kvm) {} -static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {} +static inline void kvm_arm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} + void kvm_arm_init_debug(void); void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c index c92053b..1418af9 100644 --- a/virt/kvm/arm/arm.c +++ b/virt/kvm/arm/arm.c @@ -358,6 +358,11 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) return kvm_vgic_vcpu_init(vcpu); } +void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) +{ + kvm_arm_arch_vcpu_uninit(vcpu); +} + void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) { int *last_ran;
In preparation for adding support for SVE in guests on arm64, a hook is needed for freeing additional per-vcpu memory when a vcpu is freed. x86 already uses the kvm_arch_vcpu_uninit() hook for a similar purpose, so this patch populates the same hook for arm. Since SVE is specific to arm64, a subsidiary hook kvm_arm_arch_vcpu_uninit() is added (with trivial implementations for now) to enable separate specialisation for arm and arm64. No functional change. Signed-off-by: Dave Martin <Dave.Martin@arm.com> --- Changes since RFCv1: * The vcpu _init_ hook that was added by the former version of this patch was never used for anything, so it is gone from this version. --- arch/arm/include/asm/kvm_host.h | 3 ++- arch/arm64/include/asm/kvm_host.h | 3 ++- virt/kvm/arm/arm.c | 5 +++++ 3 files changed, 9 insertions(+), 2 deletions(-)