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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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