| Message ID | 20181105143617.120602-3-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Workaround for Cortex-A76 erratum 1165522 | expand |
On Mon, Nov 05, 2018 at 02:36:14PM +0000, Marc Zyngier wrote: > Some implementations may be forced to use VHE to work around HW > errata, for example. Let's introduce a helper that checks for > these cases. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm/include/asm/kvm_host.h | 1 + > arch/arm64/include/asm/kvm_host.h | 6 ++++++ > virt/kvm/arm/arm.c | 5 +++++ > 3 files changed, 12 insertions(+) > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index c3469729f40c..0f2f782548cb 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -286,6 +286,7 @@ void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot); > struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr); > > static inline bool kvm_arch_sve_requires_vhe(void) { return false; } > +static inline bool kvm_arch_impl_requires_vhe(void) { return false; } > 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) {} > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index ca1714148400..7d6e974d024a 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -432,6 +432,12 @@ static inline bool kvm_arch_sve_requires_vhe(void) > return system_supports_sve(); > } > > +static inline bool kvm_arch_impl_requires_vhe(void) > +{ > + /* Some implementations have defects that confine them to VHE */ > + return false; > +} > + > 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) {} > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index 66de2efddfca..bc90a1cdd27f 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -1647,6 +1647,11 @@ int kvm_arch_init(void *opaque) > kvm_pr_unimpl("SVE system without VHE unsupported. Broken cpu?"); > return -ENODEV; > } > + > + if (kvm_arch_impl_requires_vhe()) { > + kvm_pr_unimpl("CPU requiring VHE in non-VHE mode"); > + return -ENODEV; > + } So as per my previous comment, I'd prefer this function to only issue a single call, and have the function in kvm_host.h check for all the conditions, do the printing, etc. Thoughts? Thanks, Christoffer > } > > for_each_online_cpu(cpu) { > -- > 2.19.1 >
On 06/11/18 07:53, Christoffer Dall wrote: > On Mon, Nov 05, 2018 at 02:36:14PM +0000, Marc Zyngier wrote: >> Some implementations may be forced to use VHE to work around HW >> errata, for example. Let's introduce a helper that checks for >> these cases. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> arch/arm/include/asm/kvm_host.h | 1 + >> arch/arm64/include/asm/kvm_host.h | 6 ++++++ >> virt/kvm/arm/arm.c | 5 +++++ >> 3 files changed, 12 insertions(+) >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> index c3469729f40c..0f2f782548cb 100644 >> --- a/arch/arm/include/asm/kvm_host.h >> +++ b/arch/arm/include/asm/kvm_host.h >> @@ -286,6 +286,7 @@ void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot); >> struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr); >> >> static inline bool kvm_arch_sve_requires_vhe(void) { return false; } >> +static inline bool kvm_arch_impl_requires_vhe(void) { return false; } >> 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) {} >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index ca1714148400..7d6e974d024a 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -432,6 +432,12 @@ static inline bool kvm_arch_sve_requires_vhe(void) >> return system_supports_sve(); >> } >> >> +static inline bool kvm_arch_impl_requires_vhe(void) >> +{ >> + /* Some implementations have defects that confine them to VHE */ >> + return false; >> +} >> + >> 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) {} >> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >> index 66de2efddfca..bc90a1cdd27f 100644 >> --- a/virt/kvm/arm/arm.c >> +++ b/virt/kvm/arm/arm.c >> @@ -1647,6 +1647,11 @@ int kvm_arch_init(void *opaque) >> kvm_pr_unimpl("SVE system without VHE unsupported. Broken cpu?"); >> return -ENODEV; >> } >> + >> + if (kvm_arch_impl_requires_vhe()) { >> + kvm_pr_unimpl("CPU requiring VHE in non-VHE mode"); >> + return -ENODEV; >> + } > > So as per my previous comment, I'd prefer this function to only issue a > single call, and have the function in kvm_host.h check for all the > conditions, do the printing, etc. > > Thoughts? Indeed, this seems a sensible alternative. Thanks, M.
On Mon, Nov 05, 2018 at 02:36:14PM +0000, Marc Zyngier wrote: > Some implementations may be forced to use VHE to work around HW > errata, for example. Let's introduce a helper that checks for > these cases. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm/include/asm/kvm_host.h | 1 + > arch/arm64/include/asm/kvm_host.h | 6 ++++++ > virt/kvm/arm/arm.c | 5 +++++ > 3 files changed, 12 insertions(+) > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index c3469729f40c..0f2f782548cb 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -286,6 +286,7 @@ void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot); > struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr); > > static inline bool kvm_arch_sve_requires_vhe(void) { return false; } > +static inline bool kvm_arch_impl_requires_vhe(void) { return false; } > 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) {} > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index ca1714148400..7d6e974d024a 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -432,6 +432,12 @@ static inline bool kvm_arch_sve_requires_vhe(void) > return system_supports_sve(); > } > > +static inline bool kvm_arch_impl_requires_vhe(void) > +{ > + /* Some implementations have defects that confine them to VHE */ > + return false; > +} > + > 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) {} > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index 66de2efddfca..bc90a1cdd27f 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -1647,6 +1647,11 @@ int kvm_arch_init(void *opaque) > kvm_pr_unimpl("SVE system without VHE unsupported. Broken cpu?"); > return -ENODEV; > } > + > + if (kvm_arch_impl_requires_vhe()) { > + kvm_pr_unimpl("CPU requiring VHE in non-VHE mode"); ( Scratch my previous comment if unless you restructure the code so there's only one nested if() here. ) Also, minor nit: it sounds like "VHE in non-VHE mode" is the thing this CPU requires, which is somewhat confusing. Does it work to word this as "CPU requiring VHE was booted in non-VHE mode" ? Cheers ---Dave
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index c3469729f40c..0f2f782548cb 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -286,6 +286,7 @@ void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot); struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr); static inline bool kvm_arch_sve_requires_vhe(void) { return false; } +static inline bool kvm_arch_impl_requires_vhe(void) { return false; } 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) {} diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index ca1714148400..7d6e974d024a 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -432,6 +432,12 @@ static inline bool kvm_arch_sve_requires_vhe(void) return system_supports_sve(); } +static inline bool kvm_arch_impl_requires_vhe(void) +{ + /* Some implementations have defects that confine them to VHE */ + return false; +} + 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) {} diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c index 66de2efddfca..bc90a1cdd27f 100644 --- a/virt/kvm/arm/arm.c +++ b/virt/kvm/arm/arm.c @@ -1647,6 +1647,11 @@ int kvm_arch_init(void *opaque) kvm_pr_unimpl("SVE system without VHE unsupported. Broken cpu?"); return -ENODEV; } + + if (kvm_arch_impl_requires_vhe()) { + kvm_pr_unimpl("CPU requiring VHE in non-VHE mode"); + return -ENODEV; + } } for_each_online_cpu(cpu) {
Some implementations may be forced to use VHE to work around HW errata, for example. Let's introduce a helper that checks for these cases. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm/include/asm/kvm_host.h | 1 + arch/arm64/include/asm/kvm_host.h | 6 ++++++ virt/kvm/arm/arm.c | 5 +++++ 3 files changed, 12 insertions(+)