Re: [RFC 5/6] capabilities: add dom0 cpu faulting disable

[Jan Beulich <jbeulich@xxxxxxxx>]

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