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 <alejandro.garciavallejo@amd.com>
---
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) )
{
/* 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;
@@ -614,7 +616,7 @@ void arch_vcpu_destroy(struct vcpu *v)
if ( is_hvm_vcpu(v) )
hvm_vcpu_destroy(v);
- else
+ else if ( IS_ENABLED(CONFIG_PV) )
pv_vcpu_destroy(v);
}
diff --git a/xen/arch/x86/include/asm/pv/domain.h b/xen/arch/x86/include/asm/pv/domain.h
index 75a6b9e5c7..582d004051 100644
--- a/xen/arch/x86/include/asm/pv/domain.h
+++ b/xen/arch/x86/include/asm/pv/domain.h
@@ -54,8 +54,6 @@ static inline unsigned long get_pcid_bits(const struct vcpu *v, bool is_xpti)
#endif
}
-#ifdef CONFIG_PV
-
void pv_vcpu_destroy(struct vcpu *v);
int pv_vcpu_initialise(struct vcpu *v);
void pv_domain_destroy(struct domain *d);
@@ -84,29 +82,6 @@ void pv_set_reg(struct vcpu *v, unsigned int reg, uint64_t val);
bool xpti_pcid_enabled(void);
-#else /* !CONFIG_PV */
-
-#include <xen/errno.h>
-
-static inline void pv_vcpu_destroy(struct vcpu *v) {}
-static inline int pv_vcpu_initialise(struct vcpu *v) { return -EOPNOTSUPP; }
-static inline void pv_domain_destroy(struct domain *d) {}
-static inline int pv_domain_initialise(struct domain *d) { return -EOPNOTSUPP; }
-
-static inline unsigned long pv_make_cr4(const struct vcpu *v) { return ~0UL; }
-
-static inline uint64_t pv_get_reg(struct vcpu *v, unsigned int reg)
-{
- ASSERT_UNREACHABLE();
- return 0;
-}
-static inline void pv_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
-{
- ASSERT_UNREACHABLE();
-}
-
-#endif /* CONFIG_PV */
-
void cf_check paravirt_ctxt_switch_from(struct vcpu *v);
void cf_check paravirt_ctxt_switch_to(struct vcpu *v);
--
2.43.0On 12.11.2025 16: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 <alejandro.garciavallejo@amd.com>
> ---
> 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?
I think using ASSERT_UNREACHABLE() there would be better, in particular bring
things in line with the {hvm,pv}_domain_initialise() call site. Preferably
that way (happy to adjust while committing):
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
On Thu Nov 13, 2025 at 12:50 PM CET, Jan Beulich wrote:
> On 12.11.2025 16: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 <alejandro.garciavallejo@amd.com>
>> ---
>> 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?
>
> I think using ASSERT_UNREACHABLE() there would be better, in particular bring
> things in line with the {hvm,pv}_domain_initialise() call site. Preferably
> that way (happy to adjust while committing):
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Jan
Sounds good to me. Though arguablt the commit message might need to change too
to remove the last line about not being a functional change. While not
externally visible, it is a real change.
Cheers,
Alejandro
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 <alejandro.garciavallejo@amd.com>
> ---
> 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/
> /* 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 :)
- 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.
- mapcache_vcpu_init() is PV only (->pv_vcpu_initialise()?)
> @@ -614,7 +616,7 @@ void arch_vcpu_destroy(struct vcpu *v)
>
> if ( is_hvm_vcpu(v) )
> hvm_vcpu_destroy(v);
> - else
> + else if ( IS_ENABLED(CONFIG_PV) )
> pv_vcpu_destroy(v);
> }
>
--
Best regards,
-grygorii
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 <alejandro.garciavallejo@amd.com>
>> ---
>> 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
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 <alejandro.garciavallejo@amd.com>
>>> ---
>>> 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
On 12.11.2025 16:56, Grygorii Strashko wrote:
> On 12.11.25 17:22, Alejandro Vallejo wrote:
>> --- 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/
It's a "positive" check (no !) here, so no, imo the order above is fine.
>> /* 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 :)
> - 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.
This may indeed be possible, but needs to be done with care. Iirc at least
once someone already screwed up there, while trying something along these
lines.
Jan
© 2016 - 2025 Red Hat, Inc.