| Message ID | 20200417155004.16806-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/pv: Start to trim 32bit support | expand |
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
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)
... 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(-)