... and move arch.is_32bit_pv into the pv union while at it.
Bloat-o-meter reports the following net savings with some notable differences
highlighted:
add/remove: 4/6 grow/shrink: 5/76 up/down: 1955/-18792 (-16837)
Function old new delta
...
pv_vcpu_initialise 411 158 -253
guest_cpuid 1837 1584 -253
pv_hypercall 579 297 -282
check_descriptor 427 130 -297
_get_page_type 5915 5202 -713
arch_get_info_guest 2225 1195 -1030
context_switch 3831 2635 -1196
dom0_construct_pv 10284 8939 -1345
arch_set_info_guest 5564 3267 -2297
Total: Before=3079563, After=3062726, chg -0.55%
In principle, DOMAIN_is_32bit_pv should be based on CONFIG_PV32, but the
assembly code is going to need further untangling before that becomes easy to
do. For now, use CONFIG_PV as missed accidentally by c/s ec651bd2460 "x86:
make entry point code build when !CONFIG_PV".
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/domctl.c | 4 ++--
xen/arch/x86/pv/domain.c | 6 +++---
xen/arch/x86/pv/hypercall.c | 2 ++
xen/arch/x86/x86_64/asm-offsets.c | 4 +++-
xen/include/asm-x86/domain.h | 4 ++--
xen/include/xen/sched.h | 15 +++++++++++++--
6 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index add70126b9..3822dd7fd1 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -576,8 +576,8 @@ long arch_do_domctl(
ret = -EOPNOTSUPP;
else if ( is_pv_domain(d) )
{
- if ( ((domctl->u.address_size.size == 64) && !d->arch.is_32bit_pv) ||
- ((domctl->u.address_size.size == 32) && d->arch.is_32bit_pv) )
+ if ( ((domctl->u.address_size.size == 64) && !d->arch.pv.is_32bit) ||
+ ((domctl->u.address_size.size == 32) && d->arch.pv.is_32bit) )
ret = 0;
else if ( domctl->u.address_size.size == 32 )
ret = switch_compat(d);
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 47a0db082f..e0977bfbd7 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
return 0;
d->arch.has_32bit_shinfo = 1;
- d->arch.is_32bit_pv = 1;
+ d->arch.pv.is_32bit = 1;
for_each_vcpu( d, v )
{
@@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
return 0;
undo_and_fail:
- d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+ d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
for_each_vcpu( d, v )
{
free_compat_arg_xlat(v);
@@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
d->arch.ctxt_switch = &pv_csw;
/* 64-bit PV guest by default. */
- d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+ d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
d->arch.pv.xpti = is_hardware_domain(d) ? opt_xpti_hwdom : opt_xpti_domu;
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 17ddf9ea1f..32d90a543f 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -302,6 +302,7 @@ void pv_ring3_init_hypercall_page(void *p)
}
}
+#ifdef CONFIG_PV32
void pv_ring1_init_hypercall_page(void *p)
{
unsigned int i;
@@ -329,6 +330,7 @@ void pv_ring1_init_hypercall_page(void *p)
*(u8 *)(p+ 7) = 0xc3; /* ret */
}
}
+#endif
/*
* Local variables:
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index 500df7a3e7..9f66a69be7 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -98,8 +98,10 @@ void __dummy__(void)
OFFSET(VCPU_nsvm_hap_enabled, struct vcpu, arch.hvm.nvcpu.u.nsvm.ns_hap_enabled);
BLANK();
- OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.is_32bit_pv);
+#ifdef CONFIG_PV
+ OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
BLANK();
+#endif
OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4192c636b1..ae155d6522 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -254,6 +254,8 @@ struct pv_domain
atomic_t nr_l4_pages;
+ /* Is a 32-bit PV guest? */
+ bool is_32bit;
/* XPTI active? */
bool xpti;
/* Use PCID feature? */
@@ -333,8 +335,6 @@ struct arch_domain
/* NB. protected by d->event_lock and by irq_desc[irq].lock */
struct radix_tree_root irq_pirq;
- /* Is a 32-bit PV (non-HVM) guest? */
- bool_t is_32bit_pv;
/* Is shared-info page in 32-bit format? */
bool_t has_32bit_shinfo;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 195e7ee583..6101761d25 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -985,7 +985,11 @@ static always_inline bool is_pv_vcpu(const struct vcpu *v)
#ifdef CONFIG_COMPAT
static always_inline bool is_pv_32bit_domain(const struct domain *d)
{
- return is_pv_domain(d) && d->arch.is_32bit_pv;
+#ifdef CONFIG_PV32
+ return is_pv_domain(d) && d->arch.pv.is_32bit;
+#else
+ return false;
+#endif
}
static always_inline bool is_pv_32bit_vcpu(const struct vcpu *v)
@@ -995,7 +999,14 @@ static always_inline bool is_pv_32bit_vcpu(const struct vcpu *v)
static always_inline bool is_pv_64bit_domain(const struct domain *d)
{
- return is_pv_domain(d) && !d->arch.is_32bit_pv;
+ if ( !is_pv_domain(d) )
+ return false;
+
+#ifdef CONFIG_PV32
+ return !d->arch.pv.is_32bit;
+#else
+ return true;
+#endif
}
static always_inline bool is_pv_64bit_vcpu(const struct vcpu *v)
--
2.11.0
On 17.04.2020 17:50, Andrew Cooper wrote:
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
> return 0;
>
> d->arch.has_32bit_shinfo = 1;
> - d->arch.is_32bit_pv = 1;
> + d->arch.pv.is_32bit = 1;
>
> for_each_vcpu( d, v )
> {
> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
> return 0;
>
> undo_and_fail:
> - d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
> + d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
> for_each_vcpu( d, v )
> {
> free_compat_arg_xlat(v);
> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
> d->arch.ctxt_switch = &pv_csw;
>
> /* 64-bit PV guest by default. */
> - d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
> + d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
Switch to true/false while you're touching these?
Jan
On 20/04/2020 15:09, Jan Beulich wrote:
> On 17.04.2020 17:50, Andrew Cooper wrote:
>> --- a/xen/arch/x86/pv/domain.c
>> +++ b/xen/arch/x86/pv/domain.c
>> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>> return 0;
>>
>> d->arch.has_32bit_shinfo = 1;
>> - d->arch.is_32bit_pv = 1;
>> + d->arch.pv.is_32bit = 1;
>>
>> for_each_vcpu( d, v )
>> {
>> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>> return 0;
>>
>> undo_and_fail:
>> - d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>> + d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>> for_each_vcpu( d, v )
>> {
>> free_compat_arg_xlat(v);
>> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>> d->arch.ctxt_switch = &pv_csw;
>>
>> /* 64-bit PV guest by default. */
>> - d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>> + d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
> Switch to true/false while you're touching these?
Yes, but I'm tempted to delete these lines in the final hunk. Its
writing zeros into a zeroed structures.
~Andrew
On 29.04.2020 15:13, Andrew Cooper wrote:
> On 20/04/2020 15:09, Jan Beulich wrote:
>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/pv/domain.c
>>> +++ b/xen/arch/x86/pv/domain.c
>>> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>>> return 0;
>>>
>>> d->arch.has_32bit_shinfo = 1;
>>> - d->arch.is_32bit_pv = 1;
>>> + d->arch.pv.is_32bit = 1;
>>>
>>> for_each_vcpu( d, v )
>>> {
>>> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>>> return 0;
>>>
>>> undo_and_fail:
>>> - d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>> + d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>> for_each_vcpu( d, v )
>>> {
>>> free_compat_arg_xlat(v);
>>> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>>> d->arch.ctxt_switch = &pv_csw;
>>>
>>> /* 64-bit PV guest by default. */
>>> - d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>> + d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>> Switch to true/false while you're touching these?
>
> Yes, but I'm tempted to delete these lines in the final hunk. Its
> writing zeros into a zeroed structures.
Oh, yes, agreed.
Jan
On 29/04/2020 14:29, Jan Beulich wrote:
> On 29.04.2020 15:13, Andrew Cooper wrote:
>> On 20/04/2020 15:09, Jan Beulich wrote:
>>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/pv/domain.c
>>>> +++ b/xen/arch/x86/pv/domain.c
>>>> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>>>> return 0;
>>>>
>>>> d->arch.has_32bit_shinfo = 1;
>>>> - d->arch.is_32bit_pv = 1;
>>>> + d->arch.pv.is_32bit = 1;
>>>>
>>>> for_each_vcpu( d, v )
>>>> {
>>>> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>>>> return 0;
>>>>
>>>> undo_and_fail:
>>>> - d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>>> + d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>>> for_each_vcpu( d, v )
>>>> {
>>>> free_compat_arg_xlat(v);
>>>> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>>>> d->arch.ctxt_switch = &pv_csw;
>>>>
>>>> /* 64-bit PV guest by default. */
>>>> - d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>>> + d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>> Switch to true/false while you're touching these?
>> Yes, but I'm tempted to delete these lines in the final hunk. Its
>> writing zeros into a zeroed structures.
> Oh, yes, agreed.
Can I take this as an ack then?
~Andrew
On 29.04.2020 15:30, Andrew Cooper wrote:
> On 29/04/2020 14:29, Jan Beulich wrote:
>> On 29.04.2020 15:13, Andrew Cooper wrote:
>>> On 20/04/2020 15:09, Jan Beulich wrote:
>>>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/pv/domain.c
>>>>> +++ b/xen/arch/x86/pv/domain.c
>>>>> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>>>>> return 0;
>>>>>
>>>>> d->arch.has_32bit_shinfo = 1;
>>>>> - d->arch.is_32bit_pv = 1;
>>>>> + d->arch.pv.is_32bit = 1;
>>>>>
>>>>> for_each_vcpu( d, v )
>>>>> {
>>>>> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>>>>> return 0;
>>>>>
>>>>> undo_and_fail:
>>>>> - d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>>>> + d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>>>> for_each_vcpu( d, v )
>>>>> {
>>>>> free_compat_arg_xlat(v);
>>>>> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>>>>> d->arch.ctxt_switch = &pv_csw;
>>>>>
>>>>> /* 64-bit PV guest by default. */
>>>>> - d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>>>> + d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>>> Switch to true/false while you're touching these?
>>> Yes, but I'm tempted to delete these lines in the final hunk. Its
>>> writing zeros into a zeroed structures.
>> Oh, yes, agreed.
>
> Can I take this as an ack then?
Sorry, didn't realize I didn't give one yet with the adjustments
made:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
© 2016 - 2026 Red Hat, Inc.