1. Patchew
  2. Xen
  3. [Xen-devel] [PATCH 00/10] x86: Default vs Max policies
  4. View patch

[Xen-devel] [PATCH 05/10] x86/msr: Compile out unused logic/objects

Andrew Cooper posted 10 patches 5 years, 11 months ago
[Xen-devel] [PATCH 05/10] x86/msr: Compile out unused logic/objects
Posted by Andrew Cooper 5 years, 11 months ago 
Arrange to compile out the PV or HVM logic and objects as applicable.  This
involves a bit of complexity in init_domain_msr_policy() as is_pv_domain()
can't be evaulated at compile time.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/msr.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index e39bb6dce4..738d7123f9 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -31,9 +31,13 @@
 DEFINE_PER_CPU(uint32_t, tsc_aux);
 
 struct msr_policy __read_mostly     raw_msr_policy,
-                  __read_mostly    host_msr_policy,
-                  __read_mostly hvm_max_msr_policy,
-                  __read_mostly  pv_max_msr_policy;
+                  __read_mostly    host_msr_policy;
+#ifdef CONFIG_PV
+struct msr_policy __read_mostly  pv_max_msr_policy;
+#endif
+#ifdef CONFIG_HVM
+struct msr_policy __read_mostly hvm_max_msr_policy;
+#endif
 
 static void __init calculate_raw_policy(void)
 {
@@ -56,9 +60,6 @@ static void __init calculate_hvm_max_policy(void)
 {
     struct msr_policy *mp = &hvm_max_msr_policy;
 
-    if ( !hvm_enabled )
-        return;
-
     *mp = host_msr_policy;
 
     /* It's always possible to emulate CPUID faulting for HVM guests */
@@ -76,16 +77,27 @@ void __init init_guest_msr_policy(void)
 {
     calculate_raw_policy();
     calculate_host_policy();
-    calculate_hvm_max_policy();
-    calculate_pv_max_policy();
+
+    if ( IS_ENABLED(CONFIG_PV) )
+        calculate_pv_max_policy();
+
+    if ( hvm_enabled )
+        calculate_hvm_max_policy();
 }
 
 int init_domain_msr_policy(struct domain *d)
 {
-    struct msr_policy *mp =
-        xmemdup(is_pv_domain(d) ?  &pv_max_msr_policy
-                                : &hvm_max_msr_policy);
+    struct msr_policy *mp = is_pv_domain(d)
+        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_msr_policy : NULL)
+        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_msr_policy : NULL);
+
+    if ( !mp )
+    {
+        ASSERT_UNREACHABLE();
+        return -EOPNOTSUPP;
+    }
 
+    mp = xmemdup(mp);
     if ( !mp )
         return -ENOMEM;
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 05/10] x86/msr: Compile out unused logic/objects
Posted by Jan Beulich 5 years, 11 months ago 
On 26.02.2020 21:22, Andrew Cooper wrote:
> @@ -76,16 +77,27 @@ void __init init_guest_msr_policy(void)
>  {
>      calculate_raw_policy();
>      calculate_host_policy();
> -    calculate_hvm_max_policy();
> -    calculate_pv_max_policy();
> +
> +    if ( IS_ENABLED(CONFIG_PV) )
> +        calculate_pv_max_policy();
> +
> +    if ( hvm_enabled )


Any chance of talking you into doing things more symmetrically,
by either also using IS_ENABLED(CONFIG_HVM) here or ...

> +        calculate_hvm_max_policy();
>  }
>  
>  int init_domain_msr_policy(struct domain *d)
>  {
> -    struct msr_policy *mp =
> -        xmemdup(is_pv_domain(d) ?  &pv_max_msr_policy
> -                                : &hvm_max_msr_policy);
> +    struct msr_policy *mp = is_pv_domain(d)
> +        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_msr_policy : NULL)
> +        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_msr_policy : NULL);

... (imo preferably) hvm_enabled here? Either way
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 05/10] x86/msr: Compile out unused logic/objects
Posted by Andrew Cooper 5 years, 11 months ago 
On 27/02/2020 08:07, Jan Beulich wrote:
> On 26.02.2020 21:22, Andrew Cooper wrote:
>> @@ -76,16 +77,27 @@ void __init init_guest_msr_policy(void)
>>  {
>>      calculate_raw_policy();
>>      calculate_host_policy();
>> -    calculate_hvm_max_policy();
>> -    calculate_pv_max_policy();
>> +
>> +    if ( IS_ENABLED(CONFIG_PV) )
>> +        calculate_pv_max_policy();
>> +
>> +    if ( hvm_enabled )
>
> Any chance of talking you into doing things more symmetrically,
> by either also using IS_ENABLED(CONFIG_HVM) here or ...
>
>> +        calculate_hvm_max_policy();
>>  }
>>  
>>  int init_domain_msr_policy(struct domain *d)
>>  {
>> -    struct msr_policy *mp =
>> -        xmemdup(is_pv_domain(d) ?  &pv_max_msr_policy
>> -                                : &hvm_max_msr_policy);
>> +    struct msr_policy *mp = is_pv_domain(d)
>> +        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_msr_policy : NULL)
>> +        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_msr_policy : NULL);
> ... (imo preferably) hvm_enabled here? Either way
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

The asymmetry is deliberate.

In the former hunk, hvm_enabled is short-circuited to false for
!CONFIG_HVM, and if I don't use hvm_enabled, here, then I've got to
retain the logic at the top of calculate_hvm_max_policy().  That seems
silly.

In this later hunk, we are looking for the most efficient way to allow
the compiler to discard the reference to hvm_max_msr_policy.  Using
hvm_enabled would be logically equivalent, but compile to more code in
CONFIG_HVM case, as it is a real boolean needing checking.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 05/10] x86/msr: Compile out unused logic/objects
Posted by Jan Beulich 5 years, 11 months ago 
On 27.02.2020 11:37, Andrew Cooper wrote:
> On 27/02/2020 08:07, Jan Beulich wrote:
>> On 26.02.2020 21:22, Andrew Cooper wrote:
>>> @@ -76,16 +77,27 @@ void __init init_guest_msr_policy(void)
>>>  {
>>>      calculate_raw_policy();
>>>      calculate_host_policy();
>>> -    calculate_hvm_max_policy();
>>> -    calculate_pv_max_policy();
>>> +
>>> +    if ( IS_ENABLED(CONFIG_PV) )
>>> +        calculate_pv_max_policy();
>>> +
>>> +    if ( hvm_enabled )
>>
>> Any chance of talking you into doing things more symmetrically,
>> by either also using IS_ENABLED(CONFIG_HVM) here or ...
>>
>>> +        calculate_hvm_max_policy();
>>>  }
>>>  
>>>  int init_domain_msr_policy(struct domain *d)
>>>  {
>>> -    struct msr_policy *mp =
>>> -        xmemdup(is_pv_domain(d) ?  &pv_max_msr_policy
>>> -                                : &hvm_max_msr_policy);
>>> +    struct msr_policy *mp = is_pv_domain(d)
>>> +        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_msr_policy : NULL)
>>> +        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_msr_policy : NULL);
>> ... (imo preferably) hvm_enabled here? Either way
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> The asymmetry is deliberate.
> 
> In the former hunk, hvm_enabled is short-circuited to false for
> !CONFIG_HVM, and if I don't use hvm_enabled, here, then I've got to
> retain the logic at the top of calculate_hvm_max_policy().  That seems
> silly.
> 
> In this later hunk, we are looking for the most efficient way to allow
> the compiler to discard the reference to hvm_max_msr_policy.  Using
> hvm_enabled would be logically equivalent, but compile to more code in
> CONFIG_HVM case, as it is a real boolean needing checking.

Fair enough, albeit I don't think the added evaluation of hvm_enabled
would be the end of the world.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.