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 code only in VMX-enabled build,
as for the rest x86 and ARM build configurations it is basically a stub.
So these stubs can be avoided by moving VIO_realmode_completion handler's code
under CONFIG_VMX and then ARM stub can be removed completely.
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 v4:
- move whole arch_vcpu_ioreq_completion() under CONFIG_VMX and remove
ARM's variant of this handler, as Julien suggested
changes in v1:
- put VIO_realmode_completion enum under #ifdef CONFIG_VMX
---
xen/arch/arm/ioreq.c | 6 ------
xen/arch/x86/hvm/ioreq.c | 2 ++
xen/arch/x86/include/asm/hvm/ioreq.h | 5 +++++
xen/common/ioreq.c | 5 ++++-
xen/include/xen/ioreq.h | 1 -
5 files changed, 11 insertions(+), 8 deletions(-)
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/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 4eb7a70182..0406630dc8 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_VMX
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/arch/x86/include/asm/hvm/ioreq.h b/xen/arch/x86/include/asm/hvm/ioreq.h
index 84be14fd08..c5f16a1e4a 100644
--- a/xen/arch/x86/include/asm/hvm/ioreq.h
+++ b/xen/arch/x86/include/asm/hvm/ioreq.h
@@ -13,6 +13,11 @@
#define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
#define IOREQ_STATUS_RETRY X86EMUL_RETRY
+#ifdef CONFIG_VMX
+bool arch_vcpu_ioreq_completion(enum vio_completion completion);
+#define arch_vcpu_ioreq_completion
+#endif
+
#endif /* __ASM_X86_HVM_IOREQ_H__ */
/*
diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index 1257a3d972..10fe932a7e 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -242,9 +242,12 @@ bool vcpu_ioreq_handle_completion(struct vcpu *v)
res = handle_pio(vio->req.addr, vio->req.size,
vio->req.dir);
break;
-
default:
+#ifdef arch_vcpu_ioreq_completion
res = arch_vcpu_ioreq_completion(completion);
+#else
+ ASSERT_UNREACHABLE();
+#endif
break;
}
diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h
index cd399adf17..22fb9ba7b0 100644
--- a/xen/include/xen/ioreq.h
+++ b/xen/include/xen/ioreq.h
@@ -111,7 +111,6 @@ 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);
-bool arch_vcpu_ioreq_completion(enum vio_completion completion);
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);
--
2.25.1On 09.07.2024 08:09, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/include/asm/hvm/ioreq.h
> +++ b/xen/arch/x86/include/asm/hvm/ioreq.h
> @@ -13,6 +13,11 @@
> #define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
> #define IOREQ_STATUS_RETRY X86EMUL_RETRY
>
> +#ifdef CONFIG_VMX
> +bool arch_vcpu_ioreq_completion(enum vio_completion completion);
> +#define arch_vcpu_ioreq_completion
> +#endif
Putting the (or some kind of) #define here is certainly fine, but moving ...
> --- a/xen/include/xen/ioreq.h
> +++ b/xen/include/xen/ioreq.h
> @@ -111,7 +111,6 @@ 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);
> -bool arch_vcpu_ioreq_completion(enum vio_completion completion);
> 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);
... the declaration from here requires that all architectures wanting to
implement the function need to have identical copies. That's unnecessary
risk of going out of sync.
As to the #define itself: It expanding to nothing means the call site
de-generates to
#ifdef arch_vcpu_ioreq_completion
res = (completion);
#else
which hardly is what is meant (despite compiling fine, and it likely
only being Eclair which would then tell us about the issue). Further
there you're also removing a blank line, I don't see why you're doing
that.
Jan
09.07.24 10:28, Jan Beulich:
> On 09.07.2024 08:09, Sergiy Kibrik wrote:
>> --- a/xen/arch/x86/include/asm/hvm/ioreq.h
>> +++ b/xen/arch/x86/include/asm/hvm/ioreq.h
>> @@ -13,6 +13,11 @@
>> #define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
>> #define IOREQ_STATUS_RETRY X86EMUL_RETRY
>>
>> +#ifdef CONFIG_VMX
>> +bool arch_vcpu_ioreq_completion(enum vio_completion completion);
>> +#define arch_vcpu_ioreq_completion
>> +#endif
>
> Putting the (or some kind of) #define here is certainly fine, but moving ...
>
>> --- a/xen/include/xen/ioreq.h
>> +++ b/xen/include/xen/ioreq.h
>> @@ -111,7 +111,6 @@ 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);
>> -bool arch_vcpu_ioreq_completion(enum vio_completion completion);
>> 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);
>
> ... the declaration from here requires that all architectures wanting to
> implement the function need to have identical copies. That's unnecessary
> risk of going out of sync.
>
> As to the #define itself: It expanding to nothing means the call site
> de-generates to
>
> #ifdef arch_vcpu_ioreq_completion
> res = (completion);
> #else
>
> which hardly is what is meant (despite compiling fine, and it likely
> only being Eclair which would then tell us about the issue). Further
> there you're also removing a blank line, I don't see why you're doing
> that.
>
looking through these changes once again I wonder why can't we just move
stub to the header like this:
in xen/include/xen/ioreq.h:
#ifdef arch_vcpu_ioreq_completion
#ifdef CONFIG_VMX
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
and avoid additional pre-processor variables & conditionals, because it
looks like we do need some kind of stub that does ASSERT_UNREACHABLE()
anyway.
-Sergiy
On 10.07.2024 12:10, Sergiy Kibrik wrote:
> 09.07.24 10:28, Jan Beulich:
>> On 09.07.2024 08:09, Sergiy Kibrik wrote:
>>> --- a/xen/arch/x86/include/asm/hvm/ioreq.h
>>> +++ b/xen/arch/x86/include/asm/hvm/ioreq.h
>>> @@ -13,6 +13,11 @@
>>> #define IOREQ_STATUS_UNHANDLED X86EMUL_UNHANDLEABLE
>>> #define IOREQ_STATUS_RETRY X86EMUL_RETRY
>>>
>>> +#ifdef CONFIG_VMX
>>> +bool arch_vcpu_ioreq_completion(enum vio_completion completion);
>>> +#define arch_vcpu_ioreq_completion
>>> +#endif
>>
>> Putting the (or some kind of) #define here is certainly fine, but moving ...
>>
>>> --- a/xen/include/xen/ioreq.h
>>> +++ b/xen/include/xen/ioreq.h
>>> @@ -111,7 +111,6 @@ 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);
>>> -bool arch_vcpu_ioreq_completion(enum vio_completion completion);
>>> 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);
>>
>> ... the declaration from here requires that all architectures wanting to
>> implement the function need to have identical copies. That's unnecessary
>> risk of going out of sync.
>>
>> As to the #define itself: It expanding to nothing means the call site
>> de-generates to
>>
>> #ifdef arch_vcpu_ioreq_completion
>> res = (completion);
>> #else
>>
>> which hardly is what is meant (despite compiling fine, and it likely
>> only being Eclair which would then tell us about the issue). Further
>> there you're also removing a blank line, I don't see why you're doing
>> that.
>>
>
> looking through these changes once again I wonder why can't we just move
> stub to the header like this:
>
> in xen/include/xen/ioreq.h:
>
> #ifdef arch_vcpu_ioreq_completion
>
> #ifdef CONFIG_VMX
> 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
>
>
> and avoid additional pre-processor variables & conditionals, because it
> looks like we do need some kind of stub that does ASSERT_UNREACHABLE()
> anyway.
That's possible to do, yes, but not as long as you key it off of CONFIG_VMX.
This arch-specific setting would better not be used in a common code header.
You could introduce a helper CONFIG_* which VMX selects, at which point
doing what you suggest is an option.
However, in what you have above I can't figure why "#ifdef
arch_vcpu_ioreq_completion" is still there.
Jan
10.07.24 13:19, Jan Beulich:
>> looking through these changes once again I wonder why can't we just move
>> stub to the header like this:
>>
>> in xen/include/xen/ioreq.h:
>>
>> #ifdef arch_vcpu_ioreq_completion
>>
>> #ifdef CONFIG_VMX
>> 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
>>
>>
>> and avoid additional pre-processor variables & conditionals, because it
>> looks like we do need some kind of stub that does ASSERT_UNREACHABLE()
>> anyway.
> That's possible to do, yes, but not as long as you key it off of CONFIG_VMX.
> This arch-specific setting would better not be used in a common code header.
> You could introduce a helper CONFIG_* which VMX selects, at which point
> doing what you suggest is an option.
>
ok, I'll try this option in next series, lets see how it will look
> However, in what you have above I can't figure why "#ifdef
> arch_vcpu_ioreq_completion" is still there.
>
disregard it please, a copy-paste error
-Sergiy
© 2016 - 2026 Red Hat, Inc.