* [PATCH v2 0/2] x86: is_pv*domain() adjustments @ 2021-04-15 9:33 Jan Beulich 2021-04-15 9:34 ` [PATCH v2 1/2] x86: correct is_pv_domain() when !CONFIG_PV Jan Beulich 2021-04-15 9:36 ` [PATCH v2 2/2] x86: use is_pv_64bit_domain() to avoid double evaluate_nospec() Jan Beulich 0 siblings, 2 replies; 6+ messages in thread From: Jan Beulich @ 2021-04-15 9:33 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné 1: correct is_pv_domain() when !CONFIG_PV 2: use is_pv_64bit_domain() to avoid double evaluate_nospec() Jan ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] x86: correct is_pv_domain() when !CONFIG_PV 2021-04-15 9:33 [PATCH v2 0/2] x86: is_pv*domain() adjustments Jan Beulich @ 2021-04-15 9:34 ` Jan Beulich 2021-04-15 10:53 ` Roger Pau Monné 2021-04-15 9:36 ` [PATCH v2 2/2] x86: use is_pv_64bit_domain() to avoid double evaluate_nospec() Jan Beulich 1 sibling, 1 reply; 6+ messages in thread From: Jan Beulich @ 2021-04-15 9:34 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné On x86, idle and other system domains are implicitly PV. While I couldn't spot any cases where this is actively a problem, some cases required quite close inspection to be certain there couldn't e.g. be some ASSERT_UNREACHABLE() that would trigger in this case. Let's be on the safe side and make sure these always have is_pv_domain() returning true. For the build to still work, this requires a few adjustments elsewhere. In particular is_pv_64bit_domain() now gains a CONFIG_PV dependency, which means that is_pv_32bit_domain() || is_pv_64bit_domain() is no longer guaranteed to be the same as is_pv_domain(). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Add comment. --- a/xen/arch/x86/dom0_build.c +++ b/xen/arch/x86/dom0_build.c @@ -571,7 +571,7 @@ int __init construct_dom0(struct domain if ( is_hvm_domain(d) ) rc = dom0_construct_pvh(d, image, image_headroom, initrd, cmdline); - else if ( is_pv_domain(d) ) + else if ( is_pv_64bit_domain(d) || is_pv_32bit_domain(d) ) rc = dom0_construct_pv(d, image, image_headroom, initrd, cmdline); else panic("Cannot construct Dom0. No guest interface available\n"); --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1564,6 +1564,7 @@ arch_do_vcpu_op( */ static void load_segments(struct vcpu *n) { +#ifdef CONFIG_PV struct cpu_user_regs *uregs = &n->arch.user_regs; unsigned long gsb = 0, gss = 0; bool compat = is_pv_32bit_vcpu(n); @@ -1729,6 +1730,7 @@ static void load_segments(struct vcpu *n regs->cs = FLAT_KERNEL_CS; regs->rip = pv->failsafe_callback_eip; } +#endif } /* @@ -1743,6 +1745,7 @@ static void load_segments(struct vcpu *n */ static void save_segments(struct vcpu *v) { +#ifdef CONFIG_PV struct cpu_user_regs *regs = &v->arch.user_regs; read_sregs(regs); @@ -1768,6 +1771,7 @@ static void save_segments(struct vcpu *v else v->arch.pv.gs_base_user = gs_base; } +#endif } void paravirt_ctxt_switch_from(struct vcpu *v) --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -457,13 +457,13 @@ long arch_do_domctl( case XEN_DOMCTL_set_address_size: if ( is_hvm_domain(d) ) ret = -EOPNOTSUPP; + else if ( is_pv_64bit_domain(d) && domctl->u.address_size.size == 32 ) + ret = switch_compat(d); else if ( is_pv_domain(d) ) { 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); else ret = -EINVAL; } --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -1036,9 +1036,14 @@ static always_inline bool is_control_dom #define VM_ASSIST(d, t) (test_bit(VMASST_TYPE_ ## t, &(d)->vm_assist)) +/* + * Note that is_pv_domain() can return true (for system domains) even when + * both is_pv_64bit_domain() and is_pv_32bit_domain() return false. IOW + * system domains can be considered PV without specific bitness. + */ static always_inline bool is_pv_domain(const struct domain *d) { - return IS_ENABLED(CONFIG_PV) && + return IS_ENABLED(CONFIG_X86) && evaluate_nospec(!(d->options & XEN_DOMCTL_CDF_hvm)); } @@ -1064,7 +1069,7 @@ static always_inline bool is_pv_32bit_vc static always_inline bool is_pv_64bit_domain(const struct domain *d) { - if ( !is_pv_domain(d) ) + if ( !IS_ENABLED(CONFIG_PV) || !is_pv_domain(d) ) return false; #ifdef CONFIG_PV32 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] x86: correct is_pv_domain() when !CONFIG_PV 2021-04-15 9:34 ` [PATCH v2 1/2] x86: correct is_pv_domain() when !CONFIG_PV Jan Beulich @ 2021-04-15 10:53 ` Roger Pau Monné 2021-04-15 11:20 ` Jan Beulich 0 siblings, 1 reply; 6+ messages in thread From: Roger Pau Monné @ 2021-04-15 10:53 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu On Thu, Apr 15, 2021 at 11:34:55AM +0200, Jan Beulich wrote: > On x86, idle and other system domains are implicitly PV. While I > couldn't spot any cases where this is actively a problem, some cases > required quite close inspection to be certain there couldn't e.g. be > some ASSERT_UNREACHABLE() that would trigger in this case. Let's be on > the safe side and make sure these always have is_pv_domain() returning > true. > > For the build to still work, this requires a few adjustments elsewhere. > In particular is_pv_64bit_domain() now gains a CONFIG_PV dependency, > which means that is_pv_32bit_domain() || is_pv_64bit_domain() is no > longer guaranteed to be the same as is_pv_domain(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Add comment. Sorry for not replying earlier, I've been thinking about this because I don't really like this approach as I think it makes code harder to follow for two reasons, first is_pv_32bit_domain() || is_pv_64bit_domain() != is_pv_domain(), which I could live with, and then also is_pv_64bit_domain() returning different values for system domains depending on whether CONFIG_PV is enabled. Given that AFAICT this patch is not fixing any actively identified issue I would rather prefer to introduce is_system_domain and use it when appropriate? I think that would be clearer long term, and avoid tying ourselves deeper into aliasing system domain with PV domains. Thanks, Roger. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] x86: correct is_pv_domain() when !CONFIG_PV 2021-04-15 10:53 ` Roger Pau Monné @ 2021-04-15 11:20 ` Jan Beulich 0 siblings, 0 replies; 6+ messages in thread From: Jan Beulich @ 2021-04-15 11:20 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu On 15.04.2021 12:53, Roger Pau Monné wrote: > On Thu, Apr 15, 2021 at 11:34:55AM +0200, Jan Beulich wrote: >> On x86, idle and other system domains are implicitly PV. While I >> couldn't spot any cases where this is actively a problem, some cases >> required quite close inspection to be certain there couldn't e.g. be >> some ASSERT_UNREACHABLE() that would trigger in this case. Let's be on >> the safe side and make sure these always have is_pv_domain() returning >> true. >> >> For the build to still work, this requires a few adjustments elsewhere. >> In particular is_pv_64bit_domain() now gains a CONFIG_PV dependency, >> which means that is_pv_32bit_domain() || is_pv_64bit_domain() is no >> longer guaranteed to be the same as is_pv_domain(). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v2: Add comment. > > Sorry for not replying earlier, I've been thinking about this because > I don't really like this approach as I think it makes code harder to > follow for two reasons, first is_pv_32bit_domain() || > is_pv_64bit_domain() != is_pv_domain(), which I could live with, and > then also is_pv_64bit_domain() returning different values for system > domains depending on whether CONFIG_PV is enabled. Well, okay, I'll consider the patch rejected then, despite thinking that it could save us from subtle issues down the road. > Given that AFAICT this patch is not fixing any actively identified > issue I would rather prefer to introduce is_system_domain and use it > when appropriate? > > I think that would be clearer long term, and avoid tying ourselves > deeper into aliasing system domain with PV domains. Of course, but it won't help until we've audited and (if needed) amended all code using is_pv_*() or e.g. implying PV when !is_hvm_*(). Patch 2, while grouped with this one, is technically independent. Therefore I'd still appreciate separate feedback there. Jan ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] x86: use is_pv_64bit_domain() to avoid double evaluate_nospec() 2021-04-15 9:33 [PATCH v2 0/2] x86: is_pv*domain() adjustments Jan Beulich 2021-04-15 9:34 ` [PATCH v2 1/2] x86: correct is_pv_domain() when !CONFIG_PV Jan Beulich @ 2021-04-15 9:36 ` Jan Beulich 2021-04-15 12:25 ` Roger Pau Monné 1 sibling, 1 reply; 6+ messages in thread From: Jan Beulich @ 2021-04-15 9:36 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1105,7 +1105,7 @@ int arch_set_info_guest( * update_cr3(), sh_update_cr3(), sh_walk_guest_tables(), and * shadow_one_bit_disable() for why that is. */ - !is_hvm_domain(d) && !is_pv_32bit_domain(d) ) + is_pv_64bit_domain(d) ) v->arch.flags &= ~TF_kernel_mode; vcpu_setup_fpu(v, v->arch.xsave_area, @@ -1251,7 +1251,7 @@ int arch_set_info_guest( * correct initial RO_MPT_VIRT_{START,END} L4 entry). */ if ( d != current->domain && !VM_ASSIST(d, m2p_strict) && - is_pv_domain(d) && !is_pv_32bit_domain(d) && + is_pv_64bit_domain(d) && test_bit(VMASST_TYPE_m2p_strict, &c.nat->vm_assist) && atomic_read(&d->arch.pv.nr_l4_pages) ) { @@ -1980,8 +1980,7 @@ static void __context_switch(void) #if defined(CONFIG_PV) && defined(CONFIG_HVM) /* Prefetch the VMCB if we expect to use it later in the context switch */ - if ( cpu_has_svm && is_pv_domain(nd) && !is_pv_32bit_domain(nd) && - !is_idle_domain(nd) ) + if ( cpu_has_svm && is_pv_64bit_domain(nd) && !is_idle_domain(nd) ) svm_load_segs_prefetch(); #endif ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] x86: use is_pv_64bit_domain() to avoid double evaluate_nospec() 2021-04-15 9:36 ` [PATCH v2 2/2] x86: use is_pv_64bit_domain() to avoid double evaluate_nospec() Jan Beulich @ 2021-04-15 12:25 ` Roger Pau Monné 0 siblings, 0 replies; 6+ messages in thread From: Roger Pau Monné @ 2021-04-15 12:25 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu On Thu, Apr 15, 2021 at 11:36:18AM +0200, Jan Beulich wrote: > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-15 12:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-15 9:33 [PATCH v2 0/2] x86: is_pv*domain() adjustments Jan Beulich 2021-04-15 9:34 ` [PATCH v2 1/2] x86: correct is_pv_domain() when !CONFIG_PV Jan Beulich 2021-04-15 10:53 ` Roger Pau Monné 2021-04-15 11:20 ` Jan Beulich 2021-04-15 9:36 ` [PATCH v2 2/2] x86: use is_pv_64bit_domain() to avoid double evaluate_nospec() Jan Beulich 2021-04-15 12:25 ` Roger Pau Monné
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).