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

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 2/24/23 22:12, Andrew Cooper wrote:
> 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.
I think this change merits its own patch.

> > +
> > +#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.
Feel free to drop it. There was not in the other header. I have a 
tendency to include headers for everything.
> ~Andrew
--
Xenia