* [XEN PATCH v1 13/15] x86: wire cpu_has_{svm/vmx}_* to false when svm/vmx not enabled
@ 2024-04-16 6:46 Sergiy Kibrik
2024-04-16 13:26 ` Andrew Cooper
2024-04-18 11:42 ` Jan Beulich
0 siblings, 2 replies; 5+ messages in thread
From: Sergiy Kibrik @ 2024-04-16 6:46 UTC (permalink / raw)
To: xen-devel
Cc: Xenia Ragiadakou, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Xenia Ragiadakou, Stefano Stabellini, Sergiy Kibrik
From: Xenia Ragiadakou <burzalodowa@gmail.com>
To be able to use cpu_has_{svm/vmx}_* macros in common code without enclosing
them inside #ifdef guards when the respective virtualization technology is
not enabled, define corresponding helper routines as false when not applicable.
No functional change intended.
Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
xen/arch/x86/include/asm/hvm/svm/svm.h | 8 ++++++++
xen/arch/x86/include/asm/hvm/vmx/vmcs.h | 7 +++++++
2 files changed, 15 insertions(+)
diff --git a/xen/arch/x86/include/asm/hvm/svm/svm.h b/xen/arch/x86/include/asm/hvm/svm/svm.h
index 4eeeb25da9..7e8cdb4a27 100644
--- a/xen/arch/x86/include/asm/hvm/svm/svm.h
+++ b/xen/arch/x86/include/asm/hvm/svm/svm.h
@@ -38,10 +38,18 @@ extern u32 svm_feature_flags;
#define SVM_FEATURE_SSS 19 /* NPT Supervisor Shadow Stacks */
#define SVM_FEATURE_SPEC_CTRL 20 /* MSR_SPEC_CTRL virtualisation */
+#ifdef CONFIG_SVM
static inline bool cpu_has_svm_feature(unsigned int feat)
{
return svm_feature_flags & (1u << feat);
}
+#else
+static inline bool cpu_has_svm_feature(unsigned int feat)
+{
+ return false;
+}
+#endif
+
#define cpu_has_svm_npt cpu_has_svm_feature(SVM_FEATURE_NPT)
#define cpu_has_svm_lbrv cpu_has_svm_feature(SVM_FEATURE_LBRV)
#define cpu_has_svm_svml cpu_has_svm_feature(SVM_FEATURE_SVML)
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
index fd197e2603..2d927d3100 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -287,10 +287,17 @@ extern uint64_t vmx_tertiary_exec_control;
#define VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL 0x80000000000ULL
extern u64 vmx_ept_vpid_cap;
+#ifdef CONFIG_VMX
static inline bool vmx_ctrl_has_feature(uint64_t control, unsigned long feature)
{
return control & feature;
}
+#else
+static inline bool vmx_ctrl_has_feature(uint64_t control, unsigned long feature)
+{
+ return false;
+}
+#endif
#define VMX_MISC_ACTIVITY_MASK 0x000001c0
#define VMX_MISC_PROC_TRACE 0x00004000
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [XEN PATCH v1 13/15] x86: wire cpu_has_{svm/vmx}_* to false when svm/vmx not enabled
2024-04-16 6:46 [XEN PATCH v1 13/15] x86: wire cpu_has_{svm/vmx}_* to false when svm/vmx not enabled Sergiy Kibrik
@ 2024-04-16 13:26 ` Andrew Cooper
2024-04-18 13:31 ` Sergiy Kibrik
2024-04-18 11:42 ` Jan Beulich
1 sibling, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2024-04-16 13:26 UTC (permalink / raw)
To: Sergiy Kibrik, xen-devel
Cc: Xenia Ragiadakou, Jan Beulich, Roger Pau Monné,
Xenia Ragiadakou, Stefano Stabellini
On 16/04/2024 7:46 am, Sergiy Kibrik wrote:
> From: Xenia Ragiadakou <burzalodowa@gmail.com>
>
> To be able to use cpu_has_{svm/vmx}_* macros in common code without enclosing
> them inside #ifdef guards when the respective virtualization technology is
> not enabled, define corresponding helper routines as false when not applicable.
>
> No functional change intended.
>
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> ---
> xen/arch/x86/include/asm/hvm/svm/svm.h | 8 ++++++++
> xen/arch/x86/include/asm/hvm/vmx/vmcs.h | 7 +++++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/xen/arch/x86/include/asm/hvm/svm/svm.h b/xen/arch/x86/include/asm/hvm/svm/svm.h
> index 4eeeb25da9..7e8cdb4a27 100644
> --- a/xen/arch/x86/include/asm/hvm/svm/svm.h
> +++ b/xen/arch/x86/include/asm/hvm/svm/svm.h
> @@ -38,10 +38,18 @@ extern u32 svm_feature_flags;
> #define SVM_FEATURE_SSS 19 /* NPT Supervisor Shadow Stacks */
> #define SVM_FEATURE_SPEC_CTRL 20 /* MSR_SPEC_CTRL virtualisation */
>
> +#ifdef CONFIG_SVM
> static inline bool cpu_has_svm_feature(unsigned int feat)
> {
> return svm_feature_flags & (1u << feat);
> }
> +#else
> +static inline bool cpu_has_svm_feature(unsigned int feat)
> +{
> + return false;
> +}
> +#endif
> +
> #define cpu_has_svm_npt cpu_has_svm_feature(SVM_FEATURE_NPT)
> #define cpu_has_svm_lbrv cpu_has_svm_feature(SVM_FEATURE_LBRV)
> #define cpu_has_svm_svml cpu_has_svm_feature(SVM_FEATURE_SVML)
> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> index fd197e2603..2d927d3100 100644
> --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
> @@ -287,10 +287,17 @@ extern uint64_t vmx_tertiary_exec_control;
> #define VMX_VPID_INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL 0x80000000000ULL
> extern u64 vmx_ept_vpid_cap;
>
> +#ifdef CONFIG_VMX
> static inline bool vmx_ctrl_has_feature(uint64_t control, unsigned long feature)
> {
> return control & feature;
> }
> +#else
> +static inline bool vmx_ctrl_has_feature(uint64_t control, unsigned long feature)
> +{
> + return false;
> +}
> +#endif
>
> #define VMX_MISC_ACTIVITY_MASK 0x000001c0
> #define VMX_MISC_PROC_TRACE 0x00004000
I'm afraid this is going in an unhelpful direction. We want to move
both of these files to be local to arch/x86/hvm/{vmx,svm}/.
cpu_has_svm_* isn't actually used outside of svm/; only the plain
SVM_FEATURE_* constants are, and that's only because they're not
expressed as plain cpu features yet.
cpu_has_vmx_* has a few more users, but most are unlikely to remain in
this form. One critical set of changes to fix vulnerabilities in
nested-virt is to make almost of of these decisions based on per-domain
state, not host state. The aspects which are host state should be in
regular cpu features.
I already volunteered to sort out the SEV feature leaf properly, and I
was going to do the SVM leaf while I was at it. If you can wait a few
days, I might be able to make half of this problem disappear.
~Andrew
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [XEN PATCH v1 13/15] x86: wire cpu_has_{svm/vmx}_* to false when svm/vmx not enabled
2024-04-16 6:46 [XEN PATCH v1 13/15] x86: wire cpu_has_{svm/vmx}_* to false when svm/vmx not enabled Sergiy Kibrik
2024-04-16 13:26 ` Andrew Cooper
@ 2024-04-18 11:42 ` Jan Beulich
1 sibling, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2024-04-18 11:42 UTC (permalink / raw)
To: Sergiy Kibrik
Cc: Xenia Ragiadakou, Andrew Cooper, Roger Pau Monné,
Xenia Ragiadakou, Stefano Stabellini, xen-devel
On 16.04.2024 08:46, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/include/asm/hvm/svm/svm.h
> +++ b/xen/arch/x86/include/asm/hvm/svm/svm.h
> @@ -38,10 +38,18 @@ extern u32 svm_feature_flags;
> #define SVM_FEATURE_SSS 19 /* NPT Supervisor Shadow Stacks */
> #define SVM_FEATURE_SPEC_CTRL 20 /* MSR_SPEC_CTRL virtualisation */
>
> +#ifdef CONFIG_SVM
> static inline bool cpu_has_svm_feature(unsigned int feat)
> {
> return svm_feature_flags & (1u << feat);
> }
> +#else
> +static inline bool cpu_has_svm_feature(unsigned int feat)
> +{
> + return false;
> +}
> +#endif
Already
static inline bool cpu_has_svm_feature(unsigned int feat)
{
#ifdef CONFIG_SVM
return svm_feature_flags & (1u << feat);
#else
return false;
#endif
}
would be less redundancy. But why not simply
static inline bool cpu_has_svm_feature(unsigned int feat)
{
return is_ENABLED(CONFIG_SVM) && (svm_feature_flags & (1u << feat));
}
?
Jan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [XEN PATCH v1 13/15] x86: wire cpu_has_{svm/vmx}_* to false when svm/vmx not enabled
2024-04-16 13:26 ` Andrew Cooper
@ 2024-04-18 13:31 ` Sergiy Kibrik
2024-04-25 22:21 ` Stefano Stabellini
0 siblings, 1 reply; 5+ messages in thread
From: Sergiy Kibrik @ 2024-04-18 13:31 UTC (permalink / raw)
To: Andrew Cooper, Stefano Stabellini
Cc: Jan Beulich, Roger Pau Monné, Xenia Ragiadakou, xen-devel
16.04.24 16:26, Andrew Cooper:
> I'm afraid this is going in an unhelpful direction. We want to move
> both of these files to be local to arch/x86/hvm/{vmx,svm}/.
>
> cpu_has_svm_* isn't actually used outside of svm/; only the plain
> SVM_FEATURE_* constants are, and that's only because they're not
> expressed as plain cpu features yet.
>
> cpu_has_vmx_* has a few more users, but most are unlikely to remain in
> this form. One critical set of changes to fix vulnerabilities in
> nested-virt is to make almost of of these decisions based on per-domain
> state, not host state. The aspects which are host state should be in
> regular cpu features.
>
> I already volunteered to sort out the SEV feature leaf properly, and I
> was going to do the SVM leaf while I was at it. If you can wait a few
> days, I might be able to make half of this problem disappear.
I guess it can wait, surely if a better solution is to be crafted at the
end.
Stefano, what's your opinion on that?
-Sergiy
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [XEN PATCH v1 13/15] x86: wire cpu_has_{svm/vmx}_* to false when svm/vmx not enabled
2024-04-18 13:31 ` Sergiy Kibrik
@ 2024-04-25 22:21 ` Stefano Stabellini
0 siblings, 0 replies; 5+ messages in thread
From: Stefano Stabellini @ 2024-04-25 22:21 UTC (permalink / raw)
To: Sergiy Kibrik
Cc: Andrew Cooper, Stefano Stabellini, Jan Beulich,
Roger Pau Monné,
Xenia Ragiadakou, xen-devel
[-- Attachment #1: Type: text/plain, Size: 1269 bytes --]
On Thu, 18 Apr 2024, Sergiy Kibrik wrote:
> 16.04.24 16:26, Andrew Cooper:
> > I'm afraid this is going in an unhelpful direction. We want to move
> > both of these files to be local to arch/x86/hvm/{vmx,svm}/.
> >
> > cpu_has_svm_* isn't actually used outside of svm/; only the plain
> > SVM_FEATURE_* constants are, and that's only because they're not
> > expressed as plain cpu features yet.
> >
> > cpu_has_vmx_* has a few more users, but most are unlikely to remain in
> > this form. One critical set of changes to fix vulnerabilities in
> > nested-virt is to make almost of of these decisions based on per-domain
> > state, not host state. The aspects which are host state should be in
> > regular cpu features.
> >
> > I already volunteered to sort out the SEV feature leaf properly, and I
> > was going to do the SVM leaf while I was at it. If you can wait a few
> > days, I might be able to make half of this problem disappear.
>
> I guess it can wait, surely if a better solution is to be crafted at the end.
>
> Stefano, what's your opinion on that?
I think Andrew's suggested direction is cleaner. We can certainly wait a
few days for Andrew to make progress. We can also follow Andrew's
suggestion in the next version of the series ourselves.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-04-25 22:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-16 6:46 [XEN PATCH v1 13/15] x86: wire cpu_has_{svm/vmx}_* to false when svm/vmx not enabled Sergiy Kibrik
2024-04-16 13:26 ` Andrew Cooper
2024-04-18 13:31 ` Sergiy Kibrik
2024-04-25 22:21 ` Stefano Stabellini
2024-04-18 11:42 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.