| Message ID | 20230801202006.20322-6-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Hyperlaunch domain roles and capabilities | expand |
On 01.08.2023 22:20, Daniel P. Smith wrote: > --- a/xen/arch/x86/cpu/common.c > +++ b/xen/arch/x86/cpu/common.c > @@ -164,48 +164,46 @@ static void set_cpuid_faulting(bool enable) > > void ctxt_switch_levelling(const struct vcpu *next) > { > - const struct domain *nextd = next ? next->domain : NULL; > - bool enable_cpuid_faulting; > - > - if (cpu_has_cpuid_faulting || > - boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)) { > - /* > - * No need to alter the faulting setting if we are switching > - * to idle; it won't affect any code running in idle context. > - */ > - if (nextd && is_idle_domain(nextd)) > - return; > - /* > - * We *should* be enabling faulting for PV control domains. > - * > - * The domain builder has now been updated to not depend on > - * seeing host CPUID values. This makes it compatible with > - * PVH toolstack domains, and lets us enable faulting by > - * default for all PV domains. > - * > - * However, as PV control domains have never had faulting > - * enforced on them before, there might plausibly be other > - * dependenices on host CPUID data. Therefore, we have left > - * an interim escape hatch in the form of > - * `dom0=no-cpuid-faulting` to restore the older behaviour. > - */ > - enable_cpuid_faulting = nextd && (opt_dom0_cpuid_faulting || > - !is_control_domain(nextd) || > - !is_pv_domain(nextd)) && > - (is_pv_domain(nextd) || > - next->arch.msrs-> > - misc_features_enables.cpuid_faulting); > - > - if (cpu_has_cpuid_faulting) > - set_cpuid_faulting(enable_cpuid_faulting); > - else > - amd_set_cpuid_user_dis(enable_cpuid_faulting); > - > - return; > - } > - > - if (ctxt_switch_masking) > - alternative_vcall(ctxt_switch_masking, next); > + const struct domain *nextd = next ? next->domain : NULL; > + bool enable_cpuid_faulting; > + > + if ( cpu_has_cpuid_faulting || > + boot_cpu_has(X86_FEATURE_CPUID_USER_DIS) ) { > + /* > + * No need to alter the faulting setting if we are switching > + * to idle; it won't affect any code running in idle context. > + */ > + if (nextd && is_idle_domain(nextd)) > + return; > + /* > + * We *should* be enabling faulting for PV control domains. > + * > + * The domain builder has now been updated to not depend on > + * seeing host CPUID values. This makes it compatible with > + * PVH toolstack domains, and lets us enable faulting by > + * default for all PV domains. > + * > + * However, as PV control domains have never had faulting > + * enforced on them before, there might plausibly be other > + * dependenices on host CPUID data. Therefore, we have left > + * an interim escape hatch in the form of > + * `dom0=no-cpuid-faulting` to restore the older behaviour. > + */ > + enable_cpuid_faulting = nextd && > + domain_has_cap(nextd, CAP_DISABLE_CPU_FAULT) && > + (is_pv_domain(nextd) || > + next->arch.msrs->misc_features_enables.cpuid_faulting); > + > + if (cpu_has_cpuid_faulting) > + set_cpuid_faulting(enable_cpuid_faulting); > + else > + amd_set_cpuid_user_dis(enable_cpuid_faulting); > + > + return; > + } > + > + if (ctxt_switch_masking) > + alternative_vcall(ctxt_switch_masking, next); > } I don't think I can spot what exactly changes here. Please avoid re- indenting the entire function. > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -907,6 +907,10 @@ static struct domain *__init create_dom0(const module_t *image, > > d->role |= ROLE_UNBOUNDED_DOMAIN; > > + if ( !opt_dom0_cpuid_faulting && > + !domain_set_cap(d, CAP_DISABLE_CPU_FAULT) ) > + printk(XENLOG_WARNING "failed to set CPU faulting on Dom %pd\n", d); No "Dom" please when you use %pd. Also I don't think you mean "set". Plus we commonly use "%pd: xyz failed\n". > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -472,7 +472,8 @@ struct domain > #define ROLE_HARDWARE_DOMAIN (1U<<2) > #define ROLE_XENSTORE_DOMAIN (1U<<3) > uint8_t role; > -#define CAP_CONSOLE_IO (1U<<0) > +#define CAP_CONSOLE_IO (1U<<0) > +#define CAP_DISABLE_CPU_FAULT (1U<<1) > uint8_t capabilities; > /* Is this guest being debugged by dom0? */ > bool debugger_attached; > @@ -1160,6 +1161,11 @@ static always_inline bool domain_set_cap( > case CAP_CONSOLE_IO: > d->capabilities |= cap; > break; > + case CAP_DISABLE_CPU_FAULT: > + /* Disabling cpu faulting is only allowed for a PV control domain. */ > + if ( is_pv_domain(d) && is_control_domain(d) ) > + d->capabilities |= cap; > + break; How do you express the x86-ness of this? Plus shouldn't this fail when either of the two conditions isn't met? Jan
On 8/8/23 11:30, Jan Beulich wrote: > On 01.08.2023 22:20, Daniel P. Smith wrote: >> --- a/xen/arch/x86/cpu/common.c >> +++ b/xen/arch/x86/cpu/common.c >> @@ -164,48 +164,46 @@ static void set_cpuid_faulting(bool enable) >> >> void ctxt_switch_levelling(const struct vcpu *next) >> { >> - const struct domain *nextd = next ? next->domain : NULL; >> - bool enable_cpuid_faulting; >> - >> - if (cpu_has_cpuid_faulting || >> - boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)) { >> - /* >> - * No need to alter the faulting setting if we are switching >> - * to idle; it won't affect any code running in idle context. >> - */ >> - if (nextd && is_idle_domain(nextd)) >> - return; >> - /* >> - * We *should* be enabling faulting for PV control domains. >> - * >> - * The domain builder has now been updated to not depend on >> - * seeing host CPUID values. This makes it compatible with >> - * PVH toolstack domains, and lets us enable faulting by >> - * default for all PV domains. >> - * >> - * However, as PV control domains have never had faulting >> - * enforced on them before, there might plausibly be other >> - * dependenices on host CPUID data. Therefore, we have left >> - * an interim escape hatch in the form of >> - * `dom0=no-cpuid-faulting` to restore the older behaviour. >> - */ >> - enable_cpuid_faulting = nextd && (opt_dom0_cpuid_faulting || >> - !is_control_domain(nextd) || >> - !is_pv_domain(nextd)) && >> - (is_pv_domain(nextd) || >> - next->arch.msrs-> >> - misc_features_enables.cpuid_faulting); >> - >> - if (cpu_has_cpuid_faulting) >> - set_cpuid_faulting(enable_cpuid_faulting); >> - else >> - amd_set_cpuid_user_dis(enable_cpuid_faulting); >> - >> - return; >> - } >> - >> - if (ctxt_switch_masking) >> - alternative_vcall(ctxt_switch_masking, next); >> + const struct domain *nextd = next ? next->domain : NULL; >> + bool enable_cpuid_faulting; >> + >> + if ( cpu_has_cpuid_faulting || >> + boot_cpu_has(X86_FEATURE_CPUID_USER_DIS) ) { >> + /* >> + * No need to alter the faulting setting if we are switching >> + * to idle; it won't affect any code running in idle context. >> + */ >> + if (nextd && is_idle_domain(nextd)) >> + return; >> + /* >> + * We *should* be enabling faulting for PV control domains. >> + * >> + * The domain builder has now been updated to not depend on >> + * seeing host CPUID values. This makes it compatible with >> + * PVH toolstack domains, and lets us enable faulting by >> + * default for all PV domains. >> + * >> + * However, as PV control domains have never had faulting >> + * enforced on them before, there might plausibly be other >> + * dependenices on host CPUID data. Therefore, we have left >> + * an interim escape hatch in the form of >> + * `dom0=no-cpuid-faulting` to restore the older behaviour. >> + */ >> + enable_cpuid_faulting = nextd && >> + domain_has_cap(nextd, CAP_DISABLE_CPU_FAULT) && >> + (is_pv_domain(nextd) || >> + next->arch.msrs->misc_features_enables.cpuid_faulting); >> + >> + if (cpu_has_cpuid_faulting) >> + set_cpuid_faulting(enable_cpuid_faulting); >> + else >> + amd_set_cpuid_user_dis(enable_cpuid_faulting); >> + >> + return; >> + } >> + >> + if (ctxt_switch_masking) >> + alternative_vcall(ctxt_switch_masking, next); >> } > > I don't think I can spot what exactly changes here. Please avoid re- > indenting the entire function. Oh, that was not intentional. I must have done a retab as that entire function is indented using hardtabs. >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -907,6 +907,10 @@ static struct domain *__init create_dom0(const module_t *image, >> >> d->role |= ROLE_UNBOUNDED_DOMAIN; >> >> + if ( !opt_dom0_cpuid_faulting && >> + !domain_set_cap(d, CAP_DISABLE_CPU_FAULT) ) >> + printk(XENLOG_WARNING "failed to set CPU faulting on Dom %pd\n", d); > > No "Dom" please when you use %pd. Also I don't think you mean "set". Plus > we commonly use "%pd: xyz failed\n". Ack on the "Dom" removal and "%pd:". As for set, it failed to set the capability on the domain. You could say enable but that is nothing more than synonyms, not changing the meaning of the statement. >> --- a/xen/include/xen/sched.h >> +++ b/xen/include/xen/sched.h >> @@ -472,7 +472,8 @@ struct domain >> #define ROLE_HARDWARE_DOMAIN (1U<<2) >> #define ROLE_XENSTORE_DOMAIN (1U<<3) >> uint8_t role; >> -#define CAP_CONSOLE_IO (1U<<0) >> +#define CAP_CONSOLE_IO (1U<<0) >> +#define CAP_DISABLE_CPU_FAULT (1U<<1) >> uint8_t capabilities; >> /* Is this guest being debugged by dom0? */ >> bool debugger_attached; >> @@ -1160,6 +1161,11 @@ static always_inline bool domain_set_cap( >> case CAP_CONSOLE_IO: >> d->capabilities |= cap; >> break; >> + case CAP_DISABLE_CPU_FAULT: >> + /* Disabling cpu faulting is only allowed for a PV control domain. */ >> + if ( is_pv_domain(d) && is_control_domain(d) ) >> + d->capabilities |= cap; >> + break; > > How do you express the x86-ness of this? Plus shouldn't this fail when > either of the two conditions isn't met? You are correct, since this moves an x86 capability out into common, it should be ifdef'ed for x86. Correct me if I am wrong here, but in the original check it would evaluate that all three conditions to be true. All this change did is effectively move the last two conditions into domain_set_cap(). Thus storing the evaluation of the first two conditions during dom0 capability setup for the result to later be evaluated during dom0 cpuid policy setup as it was done before. v/r, dps
On 09.08.2023 01:59, Daniel P. Smith wrote: > On 8/8/23 11:30, Jan Beulich wrote: >> On 01.08.2023 22:20, Daniel P. Smith wrote: >>> --- a/xen/arch/x86/setup.c >>> +++ b/xen/arch/x86/setup.c >>> @@ -907,6 +907,10 @@ static struct domain *__init create_dom0(const module_t *image, >>> >>> d->role |= ROLE_UNBOUNDED_DOMAIN; >>> >>> + if ( !opt_dom0_cpuid_faulting && >>> + !domain_set_cap(d, CAP_DISABLE_CPU_FAULT) ) >>> + printk(XENLOG_WARNING "failed to set CPU faulting on Dom %pd\n", d); >> >> No "Dom" please when you use %pd. Also I don't think you mean "set". Plus >> we commonly use "%pd: xyz failed\n". > > Ack on the "Dom" removal and "%pd:". > > As for set, it failed to set the capability on the domain. You could say > enable but that is nothing more than synonyms, not changing the meaning > of the statement. But you don't say "capability" in the message. That's what is being set. But what you do instead is disable CPUID faulting. In fact I wonder whether expressing that as a capability actually makes sense. To me a capability is something a domain may make use of, but doesn't have to. That's not the case here: CPUID faulting is either active for a domain, or it is not. Jan
diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c index 1f954d4e59..42c3193938 100644 --- a/xen/arch/x86/cpu-policy.c +++ b/xen/arch/x86/cpu-policy.c @@ -912,7 +912,7 @@ void __init init_dom0_cpuid_policy(struct domain *d) * If the domain is getting unfiltered CPUID, don't let the guest kernel * play with CPUID faulting either, as Xen's CPUID path won't cope. */ - if ( !opt_dom0_cpuid_faulting && is_control_domain(d) && is_pv_domain(d) ) + if ( domain_has_cap(d, CAP_DISABLE_CPU_FAULT) ) p->platform_info.cpuid_faulting = false; recalculate_cpuid_policy(d); diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c index cfcdaace12..937581e353 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -164,48 +164,46 @@ static void set_cpuid_faulting(bool enable) void ctxt_switch_levelling(const struct vcpu *next) { - const struct domain *nextd = next ? next->domain : NULL; - bool enable_cpuid_faulting; - - if (cpu_has_cpuid_faulting || - boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)) { - /* - * No need to alter the faulting setting if we are switching - * to idle; it won't affect any code running in idle context. - */ - if (nextd && is_idle_domain(nextd)) - return; - /* - * We *should* be enabling faulting for PV control domains. - * - * The domain builder has now been updated to not depend on - * seeing host CPUID values. This makes it compatible with - * PVH toolstack domains, and lets us enable faulting by - * default for all PV domains. - * - * However, as PV control domains have never had faulting - * enforced on them before, there might plausibly be other - * dependenices on host CPUID data. Therefore, we have left - * an interim escape hatch in the form of - * `dom0=no-cpuid-faulting` to restore the older behaviour. - */ - enable_cpuid_faulting = nextd && (opt_dom0_cpuid_faulting || - !is_control_domain(nextd) || - !is_pv_domain(nextd)) && - (is_pv_domain(nextd) || - next->arch.msrs-> - misc_features_enables.cpuid_faulting); - - if (cpu_has_cpuid_faulting) - set_cpuid_faulting(enable_cpuid_faulting); - else - amd_set_cpuid_user_dis(enable_cpuid_faulting); - - return; - } - - if (ctxt_switch_masking) - alternative_vcall(ctxt_switch_masking, next); + const struct domain *nextd = next ? next->domain : NULL; + bool enable_cpuid_faulting; + + if ( cpu_has_cpuid_faulting || + boot_cpu_has(X86_FEATURE_CPUID_USER_DIS) ) { + /* + * No need to alter the faulting setting if we are switching + * to idle; it won't affect any code running in idle context. + */ + if (nextd && is_idle_domain(nextd)) + return; + /* + * We *should* be enabling faulting for PV control domains. + * + * The domain builder has now been updated to not depend on + * seeing host CPUID values. This makes it compatible with + * PVH toolstack domains, and lets us enable faulting by + * default for all PV domains. + * + * However, as PV control domains have never had faulting + * enforced on them before, there might plausibly be other + * dependenices on host CPUID data. Therefore, we have left + * an interim escape hatch in the form of + * `dom0=no-cpuid-faulting` to restore the older behaviour. + */ + enable_cpuid_faulting = nextd && + domain_has_cap(nextd, CAP_DISABLE_CPU_FAULT) && + (is_pv_domain(nextd) || + next->arch.msrs->misc_features_enables.cpuid_faulting); + + if (cpu_has_cpuid_faulting) + set_cpuid_faulting(enable_cpuid_faulting); + else + amd_set_cpuid_user_dis(enable_cpuid_faulting); + + return; + } + + if (ctxt_switch_masking) + alternative_vcall(ctxt_switch_masking, next); } bool_t opt_cpu_info; diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 4e20edc3bf..d65144da01 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -907,6 +907,10 @@ static struct domain *__init create_dom0(const module_t *image, d->role |= ROLE_UNBOUNDED_DOMAIN; + if ( !opt_dom0_cpuid_faulting && + !domain_set_cap(d, CAP_DISABLE_CPU_FAULT) ) + printk(XENLOG_WARNING "failed to set CPU faulting on Dom %pd\n", d); + init_dom0_cpuid_policy(d); if ( alloc_dom0_vcpu0(d) == NULL ) diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index b04fbe0565..ebfe65cd73 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -472,7 +472,8 @@ struct domain #define ROLE_HARDWARE_DOMAIN (1U<<2) #define ROLE_XENSTORE_DOMAIN (1U<<3) uint8_t role; -#define CAP_CONSOLE_IO (1U<<0) +#define CAP_CONSOLE_IO (1U<<0) +#define CAP_DISABLE_CPU_FAULT (1U<<1) uint8_t capabilities; /* Is this guest being debugged by dom0? */ bool debugger_attached; @@ -1160,6 +1161,11 @@ static always_inline bool domain_set_cap( case CAP_CONSOLE_IO: d->capabilities |= cap; break; + case CAP_DISABLE_CPU_FAULT: + /* Disabling cpu faulting is only allowed for a PV control domain. */ + if ( is_pv_domain(d) && is_control_domain(d) ) + d->capabilities |= cap; + break; default: return false; }
This encapsulates disableing cpu faulting for PV dom0 as a capability. Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- xen/arch/x86/cpu-policy.c | 2 +- xen/arch/x86/cpu/common.c | 82 +++++++++++++++++++-------------------- xen/arch/x86/setup.c | 4 ++ xen/include/xen/sched.h | 8 +++- 4 files changed, 52 insertions(+), 44 deletions(-)