Hi Sergiy,
On 03/06/2024 12:34, Sergiy Kibrik wrote:
For the most cases arch_vcpu_ioreq_completion() routine is just an empty stub,
except when handling VIO_realmode_completion, which only happens on HVM
domains running on VT-x machine. When VT-x is disabled in build configuration,
both x86 & arm version of routine become empty stubs.
To dispose of these useless stubs we can do optional call to arch-specific
ioreq completion handler, if it's present, and drop arm and generic x86
handlers.
Actual handling of VIO_realmore_completion can be done by VMX code then.
Signed-off-by: Sergiy Kibrik <sergiy_kib...@epam.com>
---
xen/arch/arm/ioreq.c | 6 ------
xen/arch/x86/hvm/ioreq.c | 23 -----------------------
xen/arch/x86/hvm/vmx/vmx.c | 16 ++++++++++++++++
xen/common/ioreq.c | 5 ++++-
xen/include/xen/ioreq.h | 2 +-
5 files changed, 21 insertions(+), 31 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..088650e007 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -29,29 +29,6 @@ bool arch_ioreq_complete_mmio(void)
return handle_mmio();
}
-bool arch_vcpu_ioreq_completion(enum vio_completion completion)
-{
- switch ( completion )
- {
- case VIO_realmode_completion:
- {
- struct hvm_emulate_ctxt ctxt;
-
- hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
- vmx_realmode_emulate_one(&ctxt);
- hvm_emulate_writeback(&ctxt);
-
- break;
- }
-
- default:
- ASSERT_UNREACHABLE();
- break;
- }
-
- return true;
-}
-
static gfn_t hvm_alloc_legacy_ioreq_gfn(struct ioreq_server *s)
{
struct domain *d = s->target;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f16faa6a61..7187d1819c 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -10,6 +10,7 @@
#include <xen/param.h>
#include <xen/trace.h>
#include <xen/sched.h>
+#include <xen/ioreq.h>
#include <xen/irq.h>
#include <xen/softirq.h>
#include <xen/domain_page.h>
@@ -2749,6 +2750,20 @@ static void cf_check vmx_set_reg(struct vcpu *v,
unsigned int reg, uint64_t val)
vmx_vmcs_exit(v);
}
+bool realmode_vcpu_ioreq_completion(enum vio_completion completion)
No one seems to call this function outside of vmx.c. So can it be 'static'?
+{
+ struct hvm_emulate_ctxt ctxt;
+
+ if ( completion != VIO_realmode_completion )
+ ASSERT_UNREACHABLE();
+
+ hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
+ vmx_realmode_emulate_one(&ctxt);
+ hvm_emulate_writeback(&ctxt);
+
+ return true;
+}
+
static struct hvm_function_table __initdata_cf_clobber vmx_function_table = {
.name = "VMX",
.cpu_up_prepare = vmx_cpu_up_prepare,
@@ -3070,6 +3085,7 @@ const struct hvm_function_table * __init start_vmx(void)
lbr_tsx_fixup_check();
ler_to_fixup_check();
+ arch_vcpu_ioreq_completion = realmode_vcpu_ioreq_completion;
return &vmx_function_table;
}
diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index 1257a3d972..94fde97ece 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -33,6 +33,8 @@
#include <public/hvm/ioreq.h>
#include <public/hvm/params.h>
+bool (*arch_vcpu_ioreq_completion)(enum vio_completion completion) = NULL;
I don't think this should be allowed to be modified after boot. So I
woudl add __ro_after_init.
+
void ioreq_request_mapcache_invalidate(const struct domain *d)
{
struct vcpu *v = current;
@@ -244,7 +246,8 @@ bool vcpu_ioreq_handle_completion(struct vcpu *v)
break;
default:
- res = arch_vcpu_ioreq_completion(completion);
+ if ( arch_vcpu_ioreq_completion )
+ res = arch_vcpu_ioreq_completion(completion);
I think this wants an:
else {
ASSERT_UNREACHABLE();
}
So this match the existing code. But I am not fully convinced that this
is the right approach. Arch_vcpu_ioreq_completion is not meant to change
across boot (or even at compile time for Arm).
Reading the previous thread, I think something like below would work:
static arch_vcpu_ioreq_completion(...)
{
#ifdef CONFIG_VMX
/* Existing code */
#else
ASSERT_UNREACHABLE();
return true;
#endif
}
If we want to avoid stub, then I think it would be better to use
#ifdef CONFIG_VMX
static arch_vcpu_ioreq...
{
}
#endif CONFIG_VMX
Then in the x86 header.
#ifdef CONFIG_VMX
static arch_vcpu_ioreq..();
#define arch_vcpu_ioreq...
#endif
And then in common/ioreq.c
#ifdef arch_vcpu_ioreq
res = arch_vcpu_ioreq(...)
#else
ASSERT_UNREACHABLE();
#endif
break;
}
diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h
index cd399adf17..880214ec41 100644
--- a/xen/include/xen/ioreq.h
+++ b/xen/include/xen/ioreq.h
@@ -111,7 +111,7 @@ 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);
+extern 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);
--
Julien Grall
