Re: [PATCH v3 05/14] x86/svm: move nestedsvm declarations used only by svm code to private header

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/02/2023 6:50 pm, Xenia Ragiadakou wrote:
> diff --git a/xen/arch/x86/hvm/svm/nestedhvm.h 
> b/xen/arch/x86/hvm/svm/nestedhvm.h
> new file mode 100644
> index 0000000000..43245e13de
> --- /dev/null
> +++ b/xen/arch/x86/hvm/svm/nestedhvm.h
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * nestedsvm.h: Nested Virtualization
> + *
> + * Copyright (c) 2011, Advanced Micro Devices, Inc
> + */
> +
> +#ifndef __X86_HVM_SVM_NESTEDHVM_PRIV_H__
> +#define __X86_HVM_SVM_NESTEDHVM_PRIV_H__
> +
> +#include <xen/mm.h>
> +#include <xen/types.h>
> +
> +#include <asm/hvm/vcpu.h>
> +#include <asm/hvm/hvm.h>
> +#include <asm/hvm/nestedhvm.h>
> +#include <asm/msr-index.h>
> +
> +/* SVM specific intblk types, cannot be an enum because gcc 4.5 complains */
> +/* GIF cleared */
> +#define hvm_intblk_svm_gif      hvm_intblk_arch

I know you're just moving code, but I simply don't believe this comment.

This additional delta seems to compile fine:

diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index dbb0022190a8..111b10673cf4 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -154,7 +154,7 @@ void svm_intr_assist(void)
             return;
 
         intblk = hvm_interrupt_blocked(v, intack);
-        if ( intblk == hvm_intblk_svm_gif )
+        if ( intblk == hvm_intblk_arch ) /* GIF */
         {
             ASSERT(nestedhvm_enabled(v->domain));
             return;
diff --git a/xen/arch/x86/hvm/svm/nestedhvm.h
b/xen/arch/x86/hvm/svm/nestedhvm.h
index 43245e13deb7..c7747daae24a 100644
--- a/xen/arch/x86/hvm/svm/nestedhvm.h
+++ b/xen/arch/x86/hvm/svm/nestedhvm.h
@@ -16,10 +16,6 @@
 #include <asm/hvm/nestedhvm.h>
 #include <asm/msr-index.h>
 
-/* SVM specific intblk types, cannot be an enum because gcc 4.5
complains */
-/* GIF cleared */
-#define hvm_intblk_svm_gif      hvm_intblk_arch
-
 #define vcpu_nestedsvm(v) (vcpu_nestedhvm(v).u.nsvm)
 
 /* True when l1 guest enabled SVM in EFER */
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c
b/xen/arch/x86/hvm/svm/nestedsvm.c
index 92316c6624ce..1794eb2105be 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -1247,7 +1247,7 @@ enum hvm_intblk cf_check nsvm_intr_blocked(struct
vcpu *v)
     ASSERT(nestedhvm_enabled(v->domain));
 
     if ( !nestedsvm_gif_isset(v) )
-        return hvm_intblk_svm_gif;
+        return hvm_intblk_arch;
 
     if ( nestedhvm_vcpu_in_guestmode(v) )
     {


but the first hunk demonstrates an error in the original logic. 
Architecturally, GIF can become set for SKINIT, and in systems where SVM
isn't available.

I'm unsure whether its better to fix this up in this patch, or defer it
for later.

> +
> +#define vcpu_nestedsvm(v) (vcpu_nestedhvm(v).u.nsvm)
> +
> +/* True when l1 guest enabled SVM in EFER */
> +#define nsvm_efer_svm_enabled(v) \
> +    (!!((v)->arch.hvm.guest_efer & EFER_SVME))

This seems to be the only use of asm/msr-index.h, and it's a macro so
the header is actually unused.

I'd drop the include - its a very common header anyway.

~Andrew