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(-)

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;
     }
-- 
2.20.1

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