| Message ID | 20230224185010.3692754-6-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/hvm: {svm,vmx} {c,h} cleanup | expand |
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
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
On 24/02/2023 8:28 pm, Xenia Ragiadakou wrote: > > 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. Yeah, it probably does. Seeing as you've reviewed my two alt patches, I'll commit some more as I've already resolved the conflicts around adjacent headers. ~Andrew
diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c index d21e930af0..dbb0022190 100644 --- a/xen/arch/x86/hvm/svm/intr.c +++ b/xen/arch/x86/hvm/svm/intr.c @@ -37,6 +37,8 @@ #include <xen/domain_page.h> #include <asm/hvm/trace.h> +#include "nestedhvm.h" + static void svm_inject_nmi(struct vcpu *v) { struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb; 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 + +#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)) + +int nestedsvm_vmcb_map(struct vcpu *v, uint64_t vmcbaddr); +void nestedsvm_vmexit_defer(struct vcpu *v, + uint64_t exitcode, uint64_t exitinfo1, uint64_t exitinfo2); +enum nestedhvm_vmexits +nestedsvm_vmexit_n2n1(struct vcpu *v, struct cpu_user_regs *regs); +enum nestedhvm_vmexits +nestedsvm_check_intercepts(struct vcpu *v, struct cpu_user_regs *regs, + uint64_t exitcode); +void svm_nested_features_on_efer_update(struct vcpu *v); + +/* Interface methods */ +void cf_check nsvm_vcpu_destroy(struct vcpu *v); +int cf_check nsvm_vcpu_initialise(struct vcpu *v); +int cf_check nsvm_vcpu_reset(struct vcpu *v); +int nsvm_vcpu_vmrun(struct vcpu *v, struct cpu_user_regs *regs); +int cf_check nsvm_vcpu_vmexit_event(struct vcpu *v, + const struct x86_event *event); +uint64_t cf_check nsvm_vcpu_hostcr3(struct vcpu *v); +bool cf_check nsvm_vmcb_guest_intercepts_event( + struct vcpu *v, unsigned int vector, int errcode); +bool cf_check nsvm_vmcb_hap_enabled(struct vcpu *v); +enum hvm_intblk cf_check nsvm_intr_blocked(struct vcpu *v); + +/* Interrupts, vGIF */ +void svm_vmexit_do_clgi(struct cpu_user_regs *regs, struct vcpu *v); +void svm_vmexit_do_stgi(struct cpu_user_regs *regs, struct vcpu *v); +bool nestedsvm_gif_isset(struct vcpu *v); +int cf_check nsvm_hap_walk_L1_p2m( + struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa, unsigned int *page_order, + uint8_t *p2m_acc, struct npfec npfec); + +#define NSVM_INTR_NOTHANDLED 3 +#define NSVM_INTR_NOTINTERCEPTED 2 +#define NSVM_INTR_FORCEVMEXIT 1 +#define NSVM_INTR_MASKED 0 + +int nestedsvm_vcpu_interrupt(struct vcpu *v, const struct hvm_intack intack); + +#endif /* __X86_HVM_SVM_NESTEDHVM_PRIV_H__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c index 5f5752ce21..80b72b5dee 100644 --- a/xen/arch/x86/hvm/svm/nestedsvm.c +++ b/xen/arch/x86/hvm/svm/nestedsvm.c @@ -20,13 +20,13 @@ #include <asm/hvm/svm/svm.h> #include <asm/hvm/svm/vmcb.h> #include <asm/hvm/nestedhvm.h> -#include <asm/hvm/svm/nestedsvm.h> #include <asm/hvm/svm/svmdebug.h> #include <asm/paging.h> /* paging_mode_hap */ #include <asm/event.h> /* for local_event_delivery_(en|dis)able */ #include <asm/p2m.h> /* p2m_get_pagetable, p2m_get_nestedp2m */ #include "emulate.h" +#include "nestedhvm.h" #include "svm.h" #define NSVM_ERROR_VVMCB 1 diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index c767a3eb76..4b74ee3d7c 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -37,7 +37,6 @@ #include <asm/hvm/monitor.h> #include <asm/hvm/nestedhvm.h> #include <asm/hvm/support.h> -#include <asm/hvm/svm/nestedsvm.h> #include <asm/hvm/svm/svm.h> #include <asm/hvm/svm/svmdebug.h> #include <asm/hvm/svm/vmcb.h> @@ -55,6 +54,7 @@ #include "asid.h" #include "emulate.h" +#include "nestedhvm.h" #include "svm.h" void noreturn svm_asm_do_resume(void); diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h index 9e65919757..f700f26f90 100644 --- a/xen/arch/x86/hvm/svm/svm.h +++ b/xen/arch/x86/hvm/svm/svm.h @@ -36,7 +36,7 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid) ".byte 0x0f,0x01,0xdf" : /* output */ : /* input */ - "a" (linear), "c" (asid)); + "a" (linear), "c" (asid) ); } /* TSC rate */ diff --git a/xen/arch/x86/include/asm/hvm/svm/nestedsvm.h b/xen/arch/x86/include/asm/hvm/svm/nestedsvm.h index 656d7d1a9a..94d45d2e8d 100644 --- a/xen/arch/x86/include/asm/hvm/svm/nestedsvm.h +++ b/xen/arch/x86/include/asm/hvm/svm/nestedsvm.h @@ -18,15 +18,12 @@ #ifndef __ASM_X86_HVM_SVM_NESTEDSVM_H__ #define __ASM_X86_HVM_SVM_NESTEDSVM_H__ -#include <asm/hvm/hvm.h> -#include <asm/hvm/svm/vmcb.h> +#include <xen/types.h> -/* SVM specific intblk types, cannot be an enum because gcc 4.5 complains */ -/* GIF cleared */ -#define hvm_intblk_svm_gif hvm_intblk_arch +#include <asm/hvm/svm/vmcb.h> struct nestedsvm { - bool_t ns_gif; + bool ns_gif; uint64_t ns_msr_hsavepa; /* MSR HSAVE_PA value */ /* l1 guest physical address of virtual vmcb used by prior VMRUN. @@ -72,7 +69,7 @@ struct nestedsvm { uint64_t ns_vmcb_guestcr3, ns_vmcb_hostcr3; uint32_t ns_guest_asid; - bool_t ns_hap_enabled; + bool ns_hap_enabled; /* Only meaningful when vmexit_pending flag is set */ struct { @@ -90,48 +87,6 @@ struct nestedsvm { } ns_hostflags; }; -#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)) - -int nestedsvm_vmcb_map(struct vcpu *v, uint64_t vmcbaddr); -void nestedsvm_vmexit_defer(struct vcpu *v, - uint64_t exitcode, uint64_t exitinfo1, uint64_t exitinfo2); -enum nestedhvm_vmexits -nestedsvm_vmexit_n2n1(struct vcpu *v, struct cpu_user_regs *regs); -enum nestedhvm_vmexits -nestedsvm_check_intercepts(struct vcpu *v, struct cpu_user_regs *regs, - uint64_t exitcode); -void svm_nested_features_on_efer_update(struct vcpu *v); - -/* Interface methods */ -void cf_check nsvm_vcpu_destroy(struct vcpu *v); -int cf_check nsvm_vcpu_initialise(struct vcpu *v); -int cf_check nsvm_vcpu_reset(struct vcpu *v); -int nsvm_vcpu_vmrun(struct vcpu *v, struct cpu_user_regs *regs); -int cf_check nsvm_vcpu_vmexit_event(struct vcpu *v, const struct x86_event *event); -uint64_t cf_check nsvm_vcpu_hostcr3(struct vcpu *v); -bool cf_check nsvm_vmcb_guest_intercepts_event( - struct vcpu *v, unsigned int vector, int errcode); -bool cf_check nsvm_vmcb_hap_enabled(struct vcpu *v); -enum hvm_intblk cf_check nsvm_intr_blocked(struct vcpu *v); - -/* Interrupts, vGIF */ -void svm_vmexit_do_clgi(struct cpu_user_regs *regs, struct vcpu *v); -void svm_vmexit_do_stgi(struct cpu_user_regs *regs, struct vcpu *v); -bool_t nestedsvm_gif_isset(struct vcpu *v); -int cf_check nsvm_hap_walk_L1_p2m( - struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa, unsigned int *page_order, - uint8_t *p2m_acc, struct npfec npfec); - -#define NSVM_INTR_NOTHANDLED 3 -#define NSVM_INTR_NOTINTERCEPTED 2 -#define NSVM_INTR_FORCEVMEXIT 1 -#define NSVM_INTR_MASKED 0 -int nestedsvm_vcpu_interrupt(struct vcpu *v, const struct hvm_intack intack); - #endif /* ASM_X86_HVM_SVM_NESTEDSVM_H__ */ /*
Create a new private header in arch/x86/hvm/svm called nestedsvm.h and move there all definitions and declarations that are used only by svm code and don't need to reside in an external header. No functional change intended. Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> --- Changes in v3: - new patch xen/arch/x86/hvm/svm/intr.c | 2 + xen/arch/x86/hvm/svm/nestedhvm.h | 77 ++++++++++++++++++++ xen/arch/x86/hvm/svm/nestedsvm.c | 2 +- xen/arch/x86/hvm/svm/svm.c | 2 +- xen/arch/x86/hvm/svm/svm.h | 2 +- xen/arch/x86/include/asm/hvm/svm/nestedsvm.h | 53 +------------- 6 files changed, 86 insertions(+), 52 deletions(-) create mode 100644 xen/arch/x86/hvm/svm/nestedhvm.h