[2/3] x86/pv: Short-circuit is_pv_{32, 64}bit_domain() in !CONFIG_PV32 builds
diff mbox series

Message ID 20200417155004.16806-3-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86/pv: Start to trim 32bit support
Related show

Commit Message

Andrew Cooper April 17, 2020, 3:50 p.m. UTC
... and move arch.is_32bit_pv into the pv union while at it.

Bloat-o-meter reports the following net savings with some notable differences
highlighted:

  add/remove: 4/6 grow/shrink: 5/76 up/down: 1955/-18792 (-16837)
  Function                                     old     new   delta
  ...
  pv_vcpu_initialise                           411     158    -253
  guest_cpuid                                 1837    1584    -253
  pv_hypercall                                 579     297    -282
  check_descriptor                             427     130    -297
  _get_page_type                              5915    5202    -713
  arch_get_info_guest                         2225    1195   -1030
  context_switch                              3831    2635   -1196
  dom0_construct_pv                          10284    8939   -1345
  arch_set_info_guest                         5564    3267   -2297
  Total: Before=3079563, After=3062726, chg -0.55%

In principle, DOMAIN_is_32bit_pv should be based on CONFIG_PV32, but the
assembly code is going to need further untangling before that becomes easy to
do.  For now, use CONFIG_PV as missed accidentally by c/s ec651bd2460 "x86:
make entry point code build when !CONFIG_PV".

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/domctl.c             |  4 ++--
 xen/arch/x86/pv/domain.c          |  6 +++---
 xen/arch/x86/pv/hypercall.c       |  2 ++
 xen/arch/x86/x86_64/asm-offsets.c |  4 +++-
 xen/include/asm-x86/domain.h      |  4 ++--
 xen/include/xen/sched.h           | 15 +++++++++++++--
 6 files changed, 25 insertions(+), 10 deletions(-)

Comments

Jan Beulich April 20, 2020, 2:09 p.m. UTC | #1
On 17.04.2020 17:50, Andrew Cooper wrote:
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>          return 0;
>  
>      d->arch.has_32bit_shinfo = 1;
> -    d->arch.is_32bit_pv = 1;
> +    d->arch.pv.is_32bit = 1;
>  
>      for_each_vcpu( d, v )
>      {
> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>      return 0;
>  
>   undo_and_fail:
> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>      for_each_vcpu( d, v )
>      {
>          free_compat_arg_xlat(v);
> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>      d->arch.ctxt_switch = &pv_csw;
>  
>      /* 64-bit PV guest by default. */
> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;

Switch to true/false while you're touching these?

Jan
Andrew Cooper April 29, 2020, 1:13 p.m. UTC | #2
On 20/04/2020 15:09, Jan Beulich wrote:
> On 17.04.2020 17:50, Andrew Cooper wrote:
>> --- a/xen/arch/x86/pv/domain.c
>> +++ b/xen/arch/x86/pv/domain.c
>> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>>          return 0;
>>  
>>      d->arch.has_32bit_shinfo = 1;
>> -    d->arch.is_32bit_pv = 1;
>> +    d->arch.pv.is_32bit = 1;
>>  
>>      for_each_vcpu( d, v )
>>      {
>> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>>      return 0;
>>  
>>   undo_and_fail:
>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>      for_each_vcpu( d, v )
>>      {
>>          free_compat_arg_xlat(v);
>> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>>      d->arch.ctxt_switch = &pv_csw;
>>  
>>      /* 64-bit PV guest by default. */
>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
> Switch to true/false while you're touching these?

Yes, but I'm tempted to delete these lines in the final hunk.  Its
writing zeros into a zeroed structures.

~Andrew
Jan Beulich April 29, 2020, 1:29 p.m. UTC | #3
On 29.04.2020 15:13, Andrew Cooper wrote:
> On 20/04/2020 15:09, Jan Beulich wrote:
>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/pv/domain.c
>>> +++ b/xen/arch/x86/pv/domain.c
>>> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>>>          return 0;
>>>  
>>>      d->arch.has_32bit_shinfo = 1;
>>> -    d->arch.is_32bit_pv = 1;
>>> +    d->arch.pv.is_32bit = 1;
>>>  
>>>      for_each_vcpu( d, v )
>>>      {
>>> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>>>      return 0;
>>>  
>>>   undo_and_fail:
>>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>>      for_each_vcpu( d, v )
>>>      {
>>>          free_compat_arg_xlat(v);
>>> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>>>      d->arch.ctxt_switch = &pv_csw;
>>>  
>>>      /* 64-bit PV guest by default. */
>>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>> Switch to true/false while you're touching these?
> 
> Yes, but I'm tempted to delete these lines in the final hunk.  Its
> writing zeros into a zeroed structures.

Oh, yes, agreed.

Jan
Andrew Cooper April 29, 2020, 1:30 p.m. UTC | #4
On 29/04/2020 14:29, Jan Beulich wrote:
> On 29.04.2020 15:13, Andrew Cooper wrote:
>> On 20/04/2020 15:09, Jan Beulich wrote:
>>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/pv/domain.c
>>>> +++ b/xen/arch/x86/pv/domain.c
>>>> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>>>>          return 0;
>>>>  
>>>>      d->arch.has_32bit_shinfo = 1;
>>>> -    d->arch.is_32bit_pv = 1;
>>>> +    d->arch.pv.is_32bit = 1;
>>>>  
>>>>      for_each_vcpu( d, v )
>>>>      {
>>>> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>>>>      return 0;
>>>>  
>>>>   undo_and_fail:
>>>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>>>      for_each_vcpu( d, v )
>>>>      {
>>>>          free_compat_arg_xlat(v);
>>>> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>>>>      d->arch.ctxt_switch = &pv_csw;
>>>>  
>>>>      /* 64-bit PV guest by default. */
>>>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>> Switch to true/false while you're touching these?
>> Yes, but I'm tempted to delete these lines in the final hunk.  Its
>> writing zeros into a zeroed structures.
> Oh, yes, agreed.

Can I take this as an ack then?

~Andrew
Jan Beulich April 29, 2020, 1:37 p.m. UTC | #5
On 29.04.2020 15:30, Andrew Cooper wrote:
> On 29/04/2020 14:29, Jan Beulich wrote:
>> On 29.04.2020 15:13, Andrew Cooper wrote:
>>> On 20/04/2020 15:09, Jan Beulich wrote:
>>>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/pv/domain.c
>>>>> +++ b/xen/arch/x86/pv/domain.c
>>>>> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>>>>>          return 0;
>>>>>  
>>>>>      d->arch.has_32bit_shinfo = 1;
>>>>> -    d->arch.is_32bit_pv = 1;
>>>>> +    d->arch.pv.is_32bit = 1;
>>>>>  
>>>>>      for_each_vcpu( d, v )
>>>>>      {
>>>>> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>>>>>      return 0;
>>>>>  
>>>>>   undo_and_fail:
>>>>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>>>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>>>>      for_each_vcpu( d, v )
>>>>>      {
>>>>>          free_compat_arg_xlat(v);
>>>>> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>>>>>      d->arch.ctxt_switch = &pv_csw;
>>>>>  
>>>>>      /* 64-bit PV guest by default. */
>>>>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>>>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>>> Switch to true/false while you're touching these?
>>> Yes, but I'm tempted to delete these lines in the final hunk.  Its
>>> writing zeros into a zeroed structures.
>> Oh, yes, agreed.
> 
> Can I take this as an ack then?

Sorry, didn't realize I didn't give one yet with the adjustments
made:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Patch
diff mbox series

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index add70126b9..3822dd7fd1 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -576,8 +576,8 @@  long arch_do_domctl(
             ret = -EOPNOTSUPP;
         else if ( is_pv_domain(d) )
         {
-            if ( ((domctl->u.address_size.size == 64) && !d->arch.is_32bit_pv) ||
-                 ((domctl->u.address_size.size == 32) && d->arch.is_32bit_pv) )
+            if ( ((domctl->u.address_size.size == 64) && !d->arch.pv.is_32bit) ||
+                 ((domctl->u.address_size.size == 32) && d->arch.pv.is_32bit) )
                 ret = 0;
             else if ( domctl->u.address_size.size == 32 )
                 ret = switch_compat(d);
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 47a0db082f..e0977bfbd7 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -215,7 +215,7 @@  int switch_compat(struct domain *d)
         return 0;
 
     d->arch.has_32bit_shinfo = 1;
-    d->arch.is_32bit_pv = 1;
+    d->arch.pv.is_32bit = 1;
 
     for_each_vcpu( d, v )
     {
@@ -235,7 +235,7 @@  int switch_compat(struct domain *d)
     return 0;
 
  undo_and_fail:
-    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
     for_each_vcpu( d, v )
     {
         free_compat_arg_xlat(v);
@@ -358,7 +358,7 @@  int pv_domain_initialise(struct domain *d)
     d->arch.ctxt_switch = &pv_csw;
 
     /* 64-bit PV guest by default. */
-    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
 
     d->arch.pv.xpti = is_hardware_domain(d) ? opt_xpti_hwdom : opt_xpti_domu;
 
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 17ddf9ea1f..32d90a543f 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -302,6 +302,7 @@  void pv_ring3_init_hypercall_page(void *p)
     }
 }
 
+#ifdef CONFIG_PV32
 void pv_ring1_init_hypercall_page(void *p)
 {
     unsigned int i;
@@ -329,6 +330,7 @@  void pv_ring1_init_hypercall_page(void *p)
         *(u8  *)(p+ 7) = 0xc3;    /* ret */
     }
 }
+#endif
 
 /*
  * Local variables:
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index 500df7a3e7..9f66a69be7 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -98,8 +98,10 @@  void __dummy__(void)
     OFFSET(VCPU_nsvm_hap_enabled, struct vcpu, arch.hvm.nvcpu.u.nsvm.ns_hap_enabled);
     BLANK();
 
-    OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.is_32bit_pv);
+#ifdef CONFIG_PV
+    OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
     BLANK();
+#endif
 
     OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
     OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4192c636b1..ae155d6522 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -254,6 +254,8 @@  struct pv_domain
 
     atomic_t nr_l4_pages;
 
+    /* Is a 32-bit PV guest? */
+    bool is_32bit;
     /* XPTI active? */
     bool xpti;
     /* Use PCID feature? */
@@ -333,8 +335,6 @@  struct arch_domain
     /* NB. protected by d->event_lock and by irq_desc[irq].lock */
     struct radix_tree_root irq_pirq;
 
-    /* Is a 32-bit PV (non-HVM) guest? */
-    bool_t is_32bit_pv;
     /* Is shared-info page in 32-bit format? */
     bool_t has_32bit_shinfo;
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 195e7ee583..6101761d25 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -985,7 +985,11 @@  static always_inline bool is_pv_vcpu(const struct vcpu *v)
 #ifdef CONFIG_COMPAT
 static always_inline bool is_pv_32bit_domain(const struct domain *d)
 {
-    return is_pv_domain(d) && d->arch.is_32bit_pv;
+#ifdef CONFIG_PV32
+    return is_pv_domain(d) && d->arch.pv.is_32bit;
+#else
+    return false;
+#endif
 }
 
 static always_inline bool is_pv_32bit_vcpu(const struct vcpu *v)
@@ -995,7 +999,14 @@  static always_inline bool is_pv_32bit_vcpu(const struct vcpu *v)
 
 static always_inline bool is_pv_64bit_domain(const struct domain *d)
 {
-    return is_pv_domain(d) && !d->arch.is_32bit_pv;
+    if ( !is_pv_domain(d) )
+        return false;
+
+#ifdef CONFIG_PV32
+    return !d->arch.pv.is_32bit;
+#else
+    return true;
+#endif
 }
 
 static always_inline bool is_pv_64bit_vcpu(const struct vcpu *v)