| Message ID | 9667345e-835e-7c55-8d6d-2774008b0017@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: is_pv*domain() adjustments | expand |
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.
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
--- 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
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.