| Message ID | 20190621093843.220980-5-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: ARMv8.3 Nested Virtualization support | expand |
On 21/06/2019 10:37, Marc Zyngier wrote: > From: Christoffer Dall <christoffer.dall@arm.com> > > Introduce the feature bit and a primitive that checks if the feature is > set behind a static key check based on the cpus_have_const_cap check. > > Checking nested_virt_in_use() on systems without nested virt enabled > should have neglgible overhead. > > We don't yet allow userspace to actually set this feature. > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm/include/asm/kvm_nested.h | 9 +++++++++ > arch/arm64/include/asm/kvm_nested.h | 13 +++++++++++++ > arch/arm64/include/uapi/asm/kvm.h | 1 + > 3 files changed, 23 insertions(+) > create mode 100644 arch/arm/include/asm/kvm_nested.h > create mode 100644 arch/arm64/include/asm/kvm_nested.h > > diff --git a/arch/arm/include/asm/kvm_nested.h b/arch/arm/include/asm/kvm_nested.h > new file mode 100644 > index 000000000000..124ff6445f8f > --- /dev/null > +++ b/arch/arm/include/asm/kvm_nested.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ARM_KVM_NESTED_H > +#define __ARM_KVM_NESTED_H > + > +#include <linux/kvm_host.h> > + > +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) { return false; } > + > +#endif /* __ARM_KVM_NESTED_H */ > diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h > new file mode 100644 > index 000000000000..8a3d121a0b42 > --- /dev/null > +++ b/arch/arm64/include/asm/kvm_nested.h > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ARM64_KVM_NESTED_H > +#define __ARM64_KVM_NESTED_H > + > +#include <linux/kvm_host.h> > + > +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) > +{ > + return cpus_have_const_cap(ARM64_HAS_NESTED_VIRT) && > + test_bit(KVM_ARM_VCPU_NESTED_VIRT, vcpu->arch.features); Nit: You could make it even cheaper for some systems by adding IS_DEFINED(CONFIG_ARM64_VHE). It would also make the dependency between NV and VHE more explicit. Otherwise: Reviewed-by: Julien Thierry <julien.thierry@arm.com> Cheers,
On Fri, Jun 21, 2019 at 10:37:48AM +0100, Marc Zyngier wrote: > From: Christoffer Dall <christoffer.dall@arm.com> > > Introduce the feature bit and a primitive that checks if the feature is > set behind a static key check based on the cpus_have_const_cap check. > > Checking nested_virt_in_use() on systems without nested virt enabled > should have neglgible overhead. > > We don't yet allow userspace to actually set this feature. > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm/include/asm/kvm_nested.h | 9 +++++++++ > arch/arm64/include/asm/kvm_nested.h | 13 +++++++++++++ > arch/arm64/include/uapi/asm/kvm.h | 1 + > 3 files changed, 23 insertions(+) > create mode 100644 arch/arm/include/asm/kvm_nested.h > create mode 100644 arch/arm64/include/asm/kvm_nested.h > > diff --git a/arch/arm/include/asm/kvm_nested.h b/arch/arm/include/asm/kvm_nested.h > new file mode 100644 > index 000000000000..124ff6445f8f > --- /dev/null > +++ b/arch/arm/include/asm/kvm_nested.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ARM_KVM_NESTED_H > +#define __ARM_KVM_NESTED_H > + > +#include <linux/kvm_host.h> > + > +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) { return false; } > + > +#endif /* __ARM_KVM_NESTED_H */ > diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h > new file mode 100644 > index 000000000000..8a3d121a0b42 > --- /dev/null > +++ b/arch/arm64/include/asm/kvm_nested.h > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ARM64_KVM_NESTED_H > +#define __ARM64_KVM_NESTED_H > + > +#include <linux/kvm_host.h> > + > +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) > +{ > + return cpus_have_const_cap(ARM64_HAS_NESTED_VIRT) && > + test_bit(KVM_ARM_VCPU_NESTED_VIRT, vcpu->arch.features); > +} > + > +#endif /* __ARM64_KVM_NESTED_H */ > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > index d819a3e8b552..563e2a8bae93 100644 > --- a/arch/arm64/include/uapi/asm/kvm.h > +++ b/arch/arm64/include/uapi/asm/kvm.h > @@ -106,6 +106,7 @@ struct kvm_regs { > #define KVM_ARM_VCPU_SVE 4 /* enable SVE for this CPU */ > #define KVM_ARM_VCPU_PTRAUTH_ADDRESS 5 /* VCPU uses address authentication */ > #define KVM_ARM_VCPU_PTRAUTH_GENERIC 6 /* VCPU uses generic authentication */ > +#define KVM_ARM_VCPU_NESTED_VIRT 7 /* Support nested virtualization */ This seems weirdly named: Isn't the feature we're exposing here really EL2? In that case, the feature the guest gets with this flag enabled is plain virtualisation, possibly with the option to nest further. Does the guest also get nested virt (i.e., recursively nested virt from the host's PoV) as a side effect, or would require an explicit extra flag? Cheers ---Dave
On Fri, Jun 21, 2019 at 10:37:48AM +0100, Marc Zyngier wrote: > From: Christoffer Dall <christoffer.dall@arm.com> > > Introduce the feature bit and a primitive that checks if the feature is > set behind a static key check based on the cpus_have_const_cap check. > > Checking nested_virt_in_use() on systems without nested virt enabled > should have neglgible overhead. > > We don't yet allow userspace to actually set this feature. > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- [...] > diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h > new file mode 100644 > index 000000000000..8a3d121a0b42 > --- /dev/null > +++ b/arch/arm64/include/asm/kvm_nested.h > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ARM64_KVM_NESTED_H > +#define __ARM64_KVM_NESTED_H > + > +#include <linux/kvm_host.h> > + > +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) > +{ > + return cpus_have_const_cap(ARM64_HAS_NESTED_VIRT) && > + test_bit(KVM_ARM_VCPU_NESTED_VIRT, vcpu->arch.features); > +} Also, is it worth having a vcpu->arch.flags flag for this, similarly to SVE and ptrauth? [...] Cheers ---Dave
On 24/06/2019 12:28, Dave Martin wrote: > On Fri, Jun 21, 2019 at 10:37:48AM +0100, Marc Zyngier wrote: >> From: Christoffer Dall <christoffer.dall@arm.com> >> >> Introduce the feature bit and a primitive that checks if the feature is >> set behind a static key check based on the cpus_have_const_cap check. >> >> Checking nested_virt_in_use() on systems without nested virt enabled >> should have neglgible overhead. >> >> We don't yet allow userspace to actually set this feature. >> >> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> arch/arm/include/asm/kvm_nested.h | 9 +++++++++ >> arch/arm64/include/asm/kvm_nested.h | 13 +++++++++++++ >> arch/arm64/include/uapi/asm/kvm.h | 1 + >> 3 files changed, 23 insertions(+) >> create mode 100644 arch/arm/include/asm/kvm_nested.h >> create mode 100644 arch/arm64/include/asm/kvm_nested.h >> >> diff --git a/arch/arm/include/asm/kvm_nested.h b/arch/arm/include/asm/kvm_nested.h >> new file mode 100644 >> index 000000000000..124ff6445f8f >> --- /dev/null >> +++ b/arch/arm/include/asm/kvm_nested.h >> @@ -0,0 +1,9 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +#ifndef __ARM_KVM_NESTED_H >> +#define __ARM_KVM_NESTED_H >> + >> +#include <linux/kvm_host.h> >> + >> +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) { return false; } >> + >> +#endif /* __ARM_KVM_NESTED_H */ >> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h >> new file mode 100644 >> index 000000000000..8a3d121a0b42 >> --- /dev/null >> +++ b/arch/arm64/include/asm/kvm_nested.h >> @@ -0,0 +1,13 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +#ifndef __ARM64_KVM_NESTED_H >> +#define __ARM64_KVM_NESTED_H >> + >> +#include <linux/kvm_host.h> >> + >> +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) >> +{ >> + return cpus_have_const_cap(ARM64_HAS_NESTED_VIRT) && >> + test_bit(KVM_ARM_VCPU_NESTED_VIRT, vcpu->arch.features); >> +} >> + >> +#endif /* __ARM64_KVM_NESTED_H */ >> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h >> index d819a3e8b552..563e2a8bae93 100644 >> --- a/arch/arm64/include/uapi/asm/kvm.h >> +++ b/arch/arm64/include/uapi/asm/kvm.h >> @@ -106,6 +106,7 @@ struct kvm_regs { >> #define KVM_ARM_VCPU_SVE 4 /* enable SVE for this CPU */ >> #define KVM_ARM_VCPU_PTRAUTH_ADDRESS 5 /* VCPU uses address authentication */ >> #define KVM_ARM_VCPU_PTRAUTH_GENERIC 6 /* VCPU uses generic authentication */ >> +#define KVM_ARM_VCPU_NESTED_VIRT 7 /* Support nested virtualization */ > > This seems weirdly named: > > Isn't the feature we're exposing here really EL2? In that case, the > feature the guest gets with this flag enabled is plain virtualisation, > possibly with the option to nest further. > > Does the guest also get nested virt (i.e., recursively nested virt from > the host's PoV) as a side effect, or would require an explicit extra > flag? So far, there is no extra flag to describe further nesting, and it directly comes from EL2 being emulated. I don't mind renaming this to KVM_ARM_VCPU_HAS_EL2, or something similar... Whether we want userspace to control the exposure of the nesting capability (i.e. EL2 with ARMv8.3-NV) is another question. Thanks, M.
On 24/06/2019 12:43, Dave Martin wrote: > On Fri, Jun 21, 2019 at 10:37:48AM +0100, Marc Zyngier wrote: >> From: Christoffer Dall <christoffer.dall@arm.com> >> >> Introduce the feature bit and a primitive that checks if the feature is >> set behind a static key check based on the cpus_have_const_cap check. >> >> Checking nested_virt_in_use() on systems without nested virt enabled >> should have neglgible overhead. >> >> We don't yet allow userspace to actually set this feature. >> >> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- > > [...] > >> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h >> new file mode 100644 >> index 000000000000..8a3d121a0b42 >> --- /dev/null >> +++ b/arch/arm64/include/asm/kvm_nested.h >> @@ -0,0 +1,13 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +#ifndef __ARM64_KVM_NESTED_H >> +#define __ARM64_KVM_NESTED_H >> + >> +#include <linux/kvm_host.h> >> + >> +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) >> +{ >> + return cpus_have_const_cap(ARM64_HAS_NESTED_VIRT) && >> + test_bit(KVM_ARM_VCPU_NESTED_VIRT, vcpu->arch.features); >> +} > > Also, is it worth having a vcpu->arch.flags flag for this, similarly to > SVE and ptrauth? What would we expose through this flag? Thanks, M.
On Wed, Jul 03, 2019 at 12:56:51PM +0100, Marc Zyngier wrote: > On 24/06/2019 12:43, Dave Martin wrote: > > On Fri, Jun 21, 2019 at 10:37:48AM +0100, Marc Zyngier wrote: > >> From: Christoffer Dall <christoffer.dall@arm.com> > >> > >> Introduce the feature bit and a primitive that checks if the feature is > >> set behind a static key check based on the cpus_have_const_cap check. > >> > >> Checking nested_virt_in_use() on systems without nested virt enabled > >> should have neglgible overhead. > >> > >> We don't yet allow userspace to actually set this feature. > >> > >> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com> > >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > >> --- > > > > [...] > > > >> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h > >> new file mode 100644 > >> index 000000000000..8a3d121a0b42 > >> --- /dev/null > >> +++ b/arch/arm64/include/asm/kvm_nested.h > >> @@ -0,0 +1,13 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 */ > >> +#ifndef __ARM64_KVM_NESTED_H > >> +#define __ARM64_KVM_NESTED_H > >> + > >> +#include <linux/kvm_host.h> > >> + > >> +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) > >> +{ > >> + return cpus_have_const_cap(ARM64_HAS_NESTED_VIRT) && > >> + test_bit(KVM_ARM_VCPU_NESTED_VIRT, vcpu->arch.features); > >> +} > > > > Also, is it worth having a vcpu->arch.flags flag for this, similarly to > > SVE and ptrauth? > > What would we expose through this flag? Nothing new, put possibly more efficient to access. AFAIK, test_bit() always results in an explicit load, whereas vcpu->arch.flags is just a variable, which we already access on some hot paths. So the compiler can read it once and cache it, with a bit of luck. For flags that are fixed after vcpu init, or flags that are only read/ written by the vcpu thread itself, this should work fine. Cheers ---Dave
On Wed, Jul 03, 2019 at 12:53:58PM +0100, Marc Zyngier wrote: > On 24/06/2019 12:28, Dave Martin wrote: > > On Fri, Jun 21, 2019 at 10:37:48AM +0100, Marc Zyngier wrote: > >> From: Christoffer Dall <christoffer.dall@arm.com> > >> > >> Introduce the feature bit and a primitive that checks if the feature is > >> set behind a static key check based on the cpus_have_const_cap check. > >> > >> Checking nested_virt_in_use() on systems without nested virt enabled > >> should have neglgible overhead. > >> > >> We don't yet allow userspace to actually set this feature. > >> > >> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com> > >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > >> --- > >> arch/arm/include/asm/kvm_nested.h | 9 +++++++++ > >> arch/arm64/include/asm/kvm_nested.h | 13 +++++++++++++ > >> arch/arm64/include/uapi/asm/kvm.h | 1 + > >> 3 files changed, 23 insertions(+) > >> create mode 100644 arch/arm/include/asm/kvm_nested.h > >> create mode 100644 arch/arm64/include/asm/kvm_nested.h > >> > >> diff --git a/arch/arm/include/asm/kvm_nested.h b/arch/arm/include/asm/kvm_nested.h > >> new file mode 100644 > >> index 000000000000..124ff6445f8f > >> --- /dev/null > >> +++ b/arch/arm/include/asm/kvm_nested.h > >> @@ -0,0 +1,9 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 */ > >> +#ifndef __ARM_KVM_NESTED_H > >> +#define __ARM_KVM_NESTED_H > >> + > >> +#include <linux/kvm_host.h> > >> + > >> +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) { return false; } > >> + > >> +#endif /* __ARM_KVM_NESTED_H */ > >> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h > >> new file mode 100644 > >> index 000000000000..8a3d121a0b42 > >> --- /dev/null > >> +++ b/arch/arm64/include/asm/kvm_nested.h > >> @@ -0,0 +1,13 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 */ > >> +#ifndef __ARM64_KVM_NESTED_H > >> +#define __ARM64_KVM_NESTED_H > >> + > >> +#include <linux/kvm_host.h> > >> + > >> +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) > >> +{ > >> + return cpus_have_const_cap(ARM64_HAS_NESTED_VIRT) && > >> + test_bit(KVM_ARM_VCPU_NESTED_VIRT, vcpu->arch.features); > >> +} > >> + > >> +#endif /* __ARM64_KVM_NESTED_H */ > >> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > >> index d819a3e8b552..563e2a8bae93 100644 > >> --- a/arch/arm64/include/uapi/asm/kvm.h > >> +++ b/arch/arm64/include/uapi/asm/kvm.h > >> @@ -106,6 +106,7 @@ struct kvm_regs { > >> #define KVM_ARM_VCPU_SVE 4 /* enable SVE for this CPU */ > >> #define KVM_ARM_VCPU_PTRAUTH_ADDRESS 5 /* VCPU uses address authentication */ > >> #define KVM_ARM_VCPU_PTRAUTH_GENERIC 6 /* VCPU uses generic authentication */ > >> +#define KVM_ARM_VCPU_NESTED_VIRT 7 /* Support nested virtualization */ > > > > This seems weirdly named: > > > > Isn't the feature we're exposing here really EL2? In that case, the > > feature the guest gets with this flag enabled is plain virtualisation, > > possibly with the option to nest further. > > > > Does the guest also get nested virt (i.e., recursively nested virt from > > the host's PoV) as a side effect, or would require an explicit extra > > flag? > > So far, there is no extra flag to describe further nesting, and it > directly comes from EL2 being emulated. I don't mind renaming this to > KVM_ARM_VCPU_HAS_EL2, or something similar... Whether we want userspace > to control the exposure of the nesting capability (i.e. EL2 with > ARMv8.3-NV) is another question. Agreed. KVM_ARM_VCPU_HAS_EL2 seems a reasonable name to me. If we have have an internal flag in vcpu_arch.flags we could call that something different (i.e., keep the NESTED_VIRT naming) if it's natural to do so. Cheers ---Dave
diff --git a/arch/arm/include/asm/kvm_nested.h b/arch/arm/include/asm/kvm_nested.h new file mode 100644 index 000000000000..124ff6445f8f --- /dev/null +++ b/arch/arm/include/asm/kvm_nested.h @@ -0,0 +1,9 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __ARM_KVM_NESTED_H +#define __ARM_KVM_NESTED_H + +#include <linux/kvm_host.h> + +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) { return false; } + +#endif /* __ARM_KVM_NESTED_H */ diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h new file mode 100644 index 000000000000..8a3d121a0b42 --- /dev/null +++ b/arch/arm64/include/asm/kvm_nested.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __ARM64_KVM_NESTED_H +#define __ARM64_KVM_NESTED_H + +#include <linux/kvm_host.h> + +static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu) +{ + return cpus_have_const_cap(ARM64_HAS_NESTED_VIRT) && + test_bit(KVM_ARM_VCPU_NESTED_VIRT, vcpu->arch.features); +} + +#endif /* __ARM64_KVM_NESTED_H */ diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h index d819a3e8b552..563e2a8bae93 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -106,6 +106,7 @@ struct kvm_regs { #define KVM_ARM_VCPU_SVE 4 /* enable SVE for this CPU */ #define KVM_ARM_VCPU_PTRAUTH_ADDRESS 5 /* VCPU uses address authentication */ #define KVM_ARM_VCPU_PTRAUTH_GENERIC 6 /* VCPU uses generic authentication */ +#define KVM_ARM_VCPU_NESTED_VIRT 7 /* Support nested virtualization */ struct kvm_vcpu_init { __u32 target;