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. Cheers, Alejandro
