| Message ID | 67f143c15bece937d7b5c0739b14cc53b0c8c13d.1722333634.git.Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: make CPU virtualisation support configurable | expand |
On 30.07.2024 12:37, Sergiy Kibrik wrote: > From: Xenia Ragiadakou <burzalodowa@gmail.com> > > VIO_realmode_completion is specific to vmx realmode and thus the function > arch_vcpu_ioreq_completion() has actual handling work only in VMX-enabled build, > as for the rest x86 and ARM build configurations it is basically a stub. > > Here a separate configuration option ARCH_IOREQ_COMPLETION introduced that tells > whether the platform we're building for requires any specific ioreq completion > handling. As of now only VMX has such requirement, so the option is selected > by INTEL_VMX, for other configurations a generic default stub is provided > (it is ARM's version of arch_vcpu_ioreq_completion() moved to common header). > > Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> > CC: Julien Grall <julien@xen.org> > CC: Jan Beulich <jbeulich@suse.com> > --- > changes in v5: > - introduce ARCH_IOREQ_COMPLETION option & put arch_vcpu_ioreq_completion() under it > - description changed I'm worried by this naming inconsistency: We also have arch_ioreq_complete_mmio(), and who know what else we'll gain. I think the Kconfig identifier wants to equally include VCPU. > --- a/xen/Kconfig > +++ b/xen/Kconfig > @@ -95,4 +95,7 @@ config LTO > config ARCH_SUPPORTS_INT128 > bool > > +config ARCH_IOREQ_COMPLETION > + bool Please maintain alphabetic sorting with the other ARCH_*. > --- a/xen/include/xen/ioreq.h > +++ b/xen/include/xen/ioreq.h > @@ -111,7 +111,17 @@ void ioreq_domain_init(struct domain *d); > int ioreq_server_dm_op(struct xen_dm_op *op, struct domain *d, bool *const_op); > > bool arch_ioreq_complete_mmio(void); > + > +#ifdef CONFIG_ARCH_IOREQ_COMPLETION > bool arch_vcpu_ioreq_completion(enum vio_completion completion); > +#else > +static inline bool arch_vcpu_ioreq_completion(enum vio_completion completion) > +{ > + ASSERT_UNREACHABLE(); > + return true; > +} I understand this is how the Arm stub had it, but is "true" really appropriate here? Looking at the sole vcpu_ioreq_handle_completion() call site in x86 code, I'm inclined to say "false" would be better: We shouldn't resume a vCPU when such an (internal) error has been encountered. Jan
31.07.24 15:39, Jan Beulich: [..] >> --- >> changes in v5: >> - introduce ARCH_IOREQ_COMPLETION option & put arch_vcpu_ioreq_completion() under it >> - description changed > > I'm worried by this naming inconsistency: We also have arch_ioreq_complete_mmio(), > and who know what else we'll gain. I think the Kconfig identifier wants to equally > include VCPU. > sure, I'll make it ARCH_VCPU_IOREQ_COMPLETION [..] >> --- a/xen/include/xen/ioreq.h >> +++ b/xen/include/xen/ioreq.h >> @@ -111,7 +111,17 @@ void ioreq_domain_init(struct domain *d); >> int ioreq_server_dm_op(struct xen_dm_op *op, struct domain *d, bool *const_op); >> >> bool arch_ioreq_complete_mmio(void); >> + >> +#ifdef CONFIG_ARCH_IOREQ_COMPLETION >> bool arch_vcpu_ioreq_completion(enum vio_completion completion); >> +#else >> +static inline bool arch_vcpu_ioreq_completion(enum vio_completion completion) >> +{ >> + ASSERT_UNREACHABLE(); >> + return true; >> +} > > I understand this is how the Arm stub had it, but is "true" really appropriate > here? Looking at the sole vcpu_ioreq_handle_completion() call site in x86 code, > I'm inclined to say "false" would be better: We shouldn't resume a vCPU when > such an (internal) error has been encountered. > not just Arm stub, both x86 & Arm variants of arch_vcpu_ioreq_completion() return true unconditionally, so there must be a reason for this. -Sergiy
diff --git a/xen/Kconfig b/xen/Kconfig index e459cdac0c..4f477fa39b 100644 --- a/xen/Kconfig +++ b/xen/Kconfig @@ -95,4 +95,7 @@ config LTO config ARCH_SUPPORTS_INT128 bool +config ARCH_IOREQ_COMPLETION + bool + source "Kconfig.debug" diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c index 5df755b48b..2e829d2e7f 100644 --- a/xen/arch/arm/ioreq.c +++ b/xen/arch/arm/ioreq.c @@ -135,12 +135,6 @@ bool arch_ioreq_complete_mmio(void) return false; } -bool arch_vcpu_ioreq_completion(enum vio_completion completion) -{ - ASSERT_UNREACHABLE(); - return true; -} - /* * The "legacy" mechanism of mapping magic pages for the IOREQ servers * is x86 specific, so the following hooks don't need to be implemented on Arm: diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index cd81fd1675..eff9eedc19 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -127,6 +127,7 @@ config AMD_SVM config INTEL_VMX def_bool HVM + select ARCH_IOREQ_COMPLETION config XEN_SHSTK bool "Supervisor Shadow Stacks" diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c index 4eb7a70182..0153ac4195 100644 --- a/xen/arch/x86/hvm/ioreq.c +++ b/xen/arch/x86/hvm/ioreq.c @@ -29,6 +29,7 @@ bool arch_ioreq_complete_mmio(void) return handle_mmio(); } +#ifdef CONFIG_ARCH_IOREQ_COMPLETION bool arch_vcpu_ioreq_completion(enum vio_completion completion) { switch ( completion ) @@ -51,6 +52,7 @@ bool arch_vcpu_ioreq_completion(enum vio_completion completion) return true; } +#endif static gfn_t hvm_alloc_legacy_ioreq_gfn(struct ioreq_server *s) { diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h index cd399adf17..31d88eb2fe 100644 --- a/xen/include/xen/ioreq.h +++ b/xen/include/xen/ioreq.h @@ -111,7 +111,17 @@ void ioreq_domain_init(struct domain *d); int ioreq_server_dm_op(struct xen_dm_op *op, struct domain *d, bool *const_op); bool arch_ioreq_complete_mmio(void); + +#ifdef CONFIG_ARCH_IOREQ_COMPLETION bool arch_vcpu_ioreq_completion(enum vio_completion completion); +#else +static inline bool arch_vcpu_ioreq_completion(enum vio_completion completion) +{ + ASSERT_UNREACHABLE(); + return true; +} +#endif + int arch_ioreq_server_map_pages(struct ioreq_server *s); void arch_ioreq_server_unmap_pages(struct ioreq_server *s); void arch_ioreq_server_enable(struct ioreq_server *s);