On 12.11.25 19:21, Alejandro Vallejo wrote:
On Wed Nov 12, 2025 at 4:56 PM CET, Grygorii Strashko wrote:
On 12.11.25 17:22, Alejandro Vallejo wrote:
They are unnecessary. The only two cases with link-time failures are
fallback else branches that can just as well be adjusted with explicit
IS_ENABLED(CONFIG_PV). HVM has no equivalent stubs and there's no reason
to keep the asymmetry.
No functional change.
Signed-off-by: Alejandro Vallejo <[email protected]>
---
I'd rather remove the "rc = -EOPNOTSUPP" branch altogether, or replace
it with ASSERT_UNREACHABLE(), but I kept it here to preserve prior behaviour.
Thoughts?
---
xen/arch/x86/domain.c | 10 ++++++----
xen/arch/x86/include/asm/pv/domain.h | 25 -------------------------
2 files changed, 6 insertions(+), 29 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 19fd86ce88..0977d9323d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -571,15 +571,17 @@ int arch_vcpu_create(struct vcpu *v)
if ( is_hvm_domain(d) )
rc = hvm_vcpu_initialise(v);
- else if ( !is_idle_domain(d) )
- rc = pv_vcpu_initialise(v);
- else
+ else if ( is_idle_domain(d) )
{
The is_idle_domain() wants to go first here, i think.
[1] https://patchwork.kernel.org/comment/26646246/
I'm not sure I follow. I inverted the condition in order for the PV case to
become the fallback "else" and be thus eliminable through DCE.
/* Idle domain */
v->arch.cr3 = __pa(idle_pg_table);
rc = 0;
v->arch.msrs = ZERO_BLOCK_PTR; /* Catch stray misuses */
}
+ else if ( IS_ENABLED(CONFIG_PV) )
+ rc = pv_vcpu_initialise(v);
+ else
+ rc = -EOPNOTSUPP;
if ( rc )
goto fail;
Actually, if you are here and have time and inspiration :)
I may find at least one of those two :)
- if ( is_idle_domain(d) ) staff can be consolidated at the
beginning of arch_vcpu_create() which will make it much more readable and
simple.
Good point
Though it's subtle because the idle domain has wacky matching semantics
and there's many exit paths. Let me see if I can make a clearer version
with that sort of consolidation that is not a functional change.
- mapcache_vcpu_init() is PV only (->pv_vcpu_initialise()?)
This I shouldn't do. It's PV-only only temporarily. The directmap removal series
(in-flight for a while now, but ought to make it upstream sooner or later) makes
it also usable for HVM when the directmap is sparsely populated. I'd rather not
generate more churn than I have to for that series.
It's all up to you.
--
Best regards,
-grygorii
