[PATCH] x86: re-order struct arch_domain fields

[PATCH] x86: re-order struct arch_domain fields

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 5029 bytes --]

... to reduce padding holes. While doing this I noticed vtsc_usercount
is a PV-only thing, so it gets moved straight to struct pv_domain.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1767,7 +1767,7 @@ void pv_soft_rdtsc(struct vcpu *v, struc
     if ( guest_kernel_mode(v, regs) )
         d->arch.vtsc_kerncount++;
     else
-        d->arch.vtsc_usercount++;
+        d->arch.pv_domain.vtsc_usercount++;
 
     if ( (int64_t)(now - d->arch.vtsc_last) > 0 )
         d->arch.vtsc_last = now;
@@ -2020,17 +2020,15 @@ static void dump_softtsc(unsigned char k
             printk(",khz=%"PRIu32, d->arch.tsc_khz);
         if ( d->arch.incarnation )
             printk(",inc=%"PRIu32, d->arch.incarnation);
-        if ( !(d->arch.vtsc_kerncount | d->arch.vtsc_usercount) )
-        {
-            printk("\n");
-            continue;
-        }
-        if ( is_hvm_domain(d) )
+        if ( is_hvm_domain(d) && d->arch.vtsc_kerncount )
             printk(",vtsc count: %"PRIu64" total\n",
                    d->arch.vtsc_kerncount);
-        else
+        else if ( is_pv_domain(d) &&
+                  (d->arch.vtsc_kerncount | d->arch.pv_domain.vtsc_usercount) )
             printk(",vtsc count: %"PRIu64" kernel, %"PRIu64" user\n",
-                   d->arch.vtsc_kerncount, d->arch.vtsc_usercount);
+                   d->arch.vtsc_kerncount, d->arch.pv_domain.vtsc_usercount);
+        else
+            printk("\n");
         domcnt++;
     }
 
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -242,6 +242,8 @@ struct pv_domain
 
     /* map_domain_page() mapping cache. */
     struct mapcache_domain mapcache;
+
+    uint64_t vtsc_usercount; /* not used for hvm */
 };
 
 struct arch_domain
@@ -250,13 +252,16 @@ struct arch_domain
 
     unsigned int hv_compat_vstart;
 
-    bool_t s3_integrity;
+    /* Maximum physical-address bitwidth supported by this guest. */
+    unsigned int physaddr_bitsize;
 
     /* I/O-port admin-specified access capabilities. */
     struct rangeset *ioport_caps;
     uint32_t pci_cf8;
     uint8_t cmos_idx;
 
+    bool_t s3_integrity;
+
     struct list_head pdev_list;
 
     union {
@@ -270,6 +275,18 @@ struct arch_domain
      * page_alloc lock */
     int page_alloc_unlock_level;
 
+    /* Continuable domain_relinquish_resources(). */
+    enum {
+        RELMEM_not_started,
+        RELMEM_shared,
+        RELMEM_xen,
+        RELMEM_l4,
+        RELMEM_l3,
+        RELMEM_l2,
+        RELMEM_done,
+    } relmem;
+    struct page_list_head relmem_list;
+
     /* nestedhvm: translate l2 guest physical to host physical */
     struct p2m_domain *nested_p2m[MAX_NESTEDP2M];
     mm_lock_t nested_p2m_lock;
@@ -277,27 +294,16 @@ struct arch_domain
     /* NB. protected by d->event_lock and by irq_desc[irq].lock */
     struct radix_tree_root irq_pirq;
 
-    /* Maximum physical-address bitwidth supported by this guest. */
-    unsigned int physaddr_bitsize;
-
     /* 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;
+
     /* Domain cannot handle spurious page faults? */
     bool_t suppress_spurious_page_faults;
 
-    /* Continuable domain_relinquish_resources(). */
-    enum {
-        RELMEM_not_started,
-        RELMEM_shared,
-        RELMEM_xen,
-        RELMEM_l4,
-        RELMEM_l3,
-        RELMEM_l2,
-        RELMEM_done,
-    } relmem;
-    struct page_list_head relmem_list;
+    /* Is PHYSDEVOP_eoi to automatically unmask the event channel? */
+    bool_t auto_unmask;
 
     cpuid_input_t *cpuids;
 
@@ -315,22 +321,18 @@ struct arch_domain
     uint32_t incarnation;    /* incremented every restore or live migrate
                                 (possibly other cases in the future */
     uint64_t vtsc_kerncount; /* for hvm, counts all vtsc */
-    uint64_t vtsc_usercount; /* not used for hvm */
 
     /* Pseudophysical e820 map (XENMEM_memory_map).  */
     spinlock_t e820_lock;
     struct e820entry *e820;
     unsigned int nr_e820;
 
-    /* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
-     * unmask the event channel */
-    bool_t auto_unmask;
+    unsigned int psr_rmid; /* RMID assigned to the domain for CMT */
+
     /* Shared page for notifying that explicit PIRQ EOI is required. */
     unsigned long *pirq_eoi_map;
     unsigned long pirq_eoi_map_mfn;
-
-    unsigned int psr_rmid; /* RMID assigned to the domain for CMT */
-} __cacheline_aligned;
+};
 
 #define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
 
@@ -486,7 +488,7 @@ struct arch_vcpu
         unsigned long eip;
     } mem_event;
 
-} __cacheline_aligned;
+};
 
 smap_check_policy_t smap_policy_change(struct vcpu *v,
                                        smap_check_policy_t new_policy);



[-- Attachment #2: x86-arch_domain-reorder.patch --]
[-- Type: text/plain, Size: 5068 bytes --]

x86: re-order struct arch_domain fields

... to reduce padding holes. While doing this I noticed vtsc_usercount
is a PV-only thing, so it gets moved straight to struct pv_domain.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1767,7 +1767,7 @@ void pv_soft_rdtsc(struct vcpu *v, struc
     if ( guest_kernel_mode(v, regs) )
         d->arch.vtsc_kerncount++;
     else
-        d->arch.vtsc_usercount++;
+        d->arch.pv_domain.vtsc_usercount++;
 
     if ( (int64_t)(now - d->arch.vtsc_last) > 0 )
         d->arch.vtsc_last = now;
@@ -2020,17 +2020,15 @@ static void dump_softtsc(unsigned char k
             printk(",khz=%"PRIu32, d->arch.tsc_khz);
         if ( d->arch.incarnation )
             printk(",inc=%"PRIu32, d->arch.incarnation);
-        if ( !(d->arch.vtsc_kerncount | d->arch.vtsc_usercount) )
-        {
-            printk("\n");
-            continue;
-        }
-        if ( is_hvm_domain(d) )
+        if ( is_hvm_domain(d) && d->arch.vtsc_kerncount )
             printk(",vtsc count: %"PRIu64" total\n",
                    d->arch.vtsc_kerncount);
-        else
+        else if ( is_pv_domain(d) &&
+                  (d->arch.vtsc_kerncount | d->arch.pv_domain.vtsc_usercount) )
             printk(",vtsc count: %"PRIu64" kernel, %"PRIu64" user\n",
-                   d->arch.vtsc_kerncount, d->arch.vtsc_usercount);
+                   d->arch.vtsc_kerncount, d->arch.pv_domain.vtsc_usercount);
+        else
+            printk("\n");
         domcnt++;
     }
 
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -242,6 +242,8 @@ struct pv_domain
 
     /* map_domain_page() mapping cache. */
     struct mapcache_domain mapcache;
+
+    uint64_t vtsc_usercount; /* not used for hvm */
 };
 
 struct arch_domain
@@ -250,13 +252,16 @@ struct arch_domain
 
     unsigned int hv_compat_vstart;
 
-    bool_t s3_integrity;
+    /* Maximum physical-address bitwidth supported by this guest. */
+    unsigned int physaddr_bitsize;
 
     /* I/O-port admin-specified access capabilities. */
     struct rangeset *ioport_caps;
     uint32_t pci_cf8;
     uint8_t cmos_idx;
 
+    bool_t s3_integrity;
+
     struct list_head pdev_list;
 
     union {
@@ -270,6 +275,18 @@ struct arch_domain
      * page_alloc lock */
     int page_alloc_unlock_level;
 
+    /* Continuable domain_relinquish_resources(). */
+    enum {
+        RELMEM_not_started,
+        RELMEM_shared,
+        RELMEM_xen,
+        RELMEM_l4,
+        RELMEM_l3,
+        RELMEM_l2,
+        RELMEM_done,
+    } relmem;
+    struct page_list_head relmem_list;
+
     /* nestedhvm: translate l2 guest physical to host physical */
     struct p2m_domain *nested_p2m[MAX_NESTEDP2M];
     mm_lock_t nested_p2m_lock;
@@ -277,27 +294,16 @@ struct arch_domain
     /* NB. protected by d->event_lock and by irq_desc[irq].lock */
     struct radix_tree_root irq_pirq;
 
-    /* Maximum physical-address bitwidth supported by this guest. */
-    unsigned int physaddr_bitsize;
-
     /* 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;
+
     /* Domain cannot handle spurious page faults? */
     bool_t suppress_spurious_page_faults;
 
-    /* Continuable domain_relinquish_resources(). */
-    enum {
-        RELMEM_not_started,
-        RELMEM_shared,
-        RELMEM_xen,
-        RELMEM_l4,
-        RELMEM_l3,
-        RELMEM_l2,
-        RELMEM_done,
-    } relmem;
-    struct page_list_head relmem_list;
+    /* Is PHYSDEVOP_eoi to automatically unmask the event channel? */
+    bool_t auto_unmask;
 
     cpuid_input_t *cpuids;
 
@@ -315,22 +321,18 @@ struct arch_domain
     uint32_t incarnation;    /* incremented every restore or live migrate
                                 (possibly other cases in the future */
     uint64_t vtsc_kerncount; /* for hvm, counts all vtsc */
-    uint64_t vtsc_usercount; /* not used for hvm */
 
     /* Pseudophysical e820 map (XENMEM_memory_map).  */
     spinlock_t e820_lock;
     struct e820entry *e820;
     unsigned int nr_e820;
 
-    /* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
-     * unmask the event channel */
-    bool_t auto_unmask;
+    unsigned int psr_rmid; /* RMID assigned to the domain for CMT */
+
     /* Shared page for notifying that explicit PIRQ EOI is required. */
     unsigned long *pirq_eoi_map;
     unsigned long pirq_eoi_map_mfn;
-
-    unsigned int psr_rmid; /* RMID assigned to the domain for CMT */
-} __cacheline_aligned;
+};
 
 #define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
 
@@ -486,7 +488,7 @@ struct arch_vcpu
         unsigned long eip;
     } mem_event;
 
-} __cacheline_aligned;
+};
 
 smap_check_policy_t smap_policy_change(struct vcpu *v,
                                        smap_check_policy_t new_policy);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86: re-order struct arch_domain fields

[Andrew Cooper]

On 19/01/15 15:41, Jan Beulich wrote:
> ... to reduce padding holes. While doing this I noticed vtsc_usercount
> is a PV-only thing, so it gets moved straight to struct pv_domain.

The vtsc_{user,kernel}count split is curious.  They are both for stats
purposes alone, but there is nothing pv specific about the usercount. 
It frankly looks as if it has been mis-implemented for HVM, despite the
split appearing to be deliberate when it was introduced in c/s
bf2c44f8b469.  I am really not sure what to make of it.

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1767,7 +1767,7 @@ void pv_soft_rdtsc(struct vcpu *v, struc
>      if ( guest_kernel_mode(v, regs) )
>          d->arch.vtsc_kerncount++;
>      else
> -        d->arch.vtsc_usercount++;
> +        d->arch.pv_domain.vtsc_usercount++;
>  
>      if ( (int64_t)(now - d->arch.vtsc_last) > 0 )
>          d->arch.vtsc_last = now;
> @@ -2020,17 +2020,15 @@ static void dump_softtsc(unsigned char k
>              printk(",khz=%"PRIu32, d->arch.tsc_khz);
>          if ( d->arch.incarnation )
>              printk(",inc=%"PRIu32, d->arch.incarnation);
> -        if ( !(d->arch.vtsc_kerncount | d->arch.vtsc_usercount) )
> -        {
> -            printk("\n");
> -            continue;
> -        }
> -        if ( is_hvm_domain(d) )
> +        if ( is_hvm_domain(d) && d->arch.vtsc_kerncount )
>              printk(",vtsc count: %"PRIu64" total\n",
>                     d->arch.vtsc_kerncount);
> -        else
> +        else if ( is_pv_domain(d) &&
> +                  (d->arch.vtsc_kerncount | d->arch.pv_domain.vtsc_usercount) )

I realise you are simply rearanging the logic from before, but this
should really be a logic or rather than a bitwise or, and is probably
worth tweaking with the movement.

~Andrew

Re: [PATCH] x86: re-order struct arch_domain fields

[Jan Beulich]

>>> On 19.01.15 at 18:52, <andrew.cooper3@citrix.com> wrote:
> On 19/01/15 15:41, Jan Beulich wrote:
>> ... to reduce padding holes. While doing this I noticed vtsc_usercount
>> is a PV-only thing, so it gets moved straight to struct pv_domain.
> 
> The vtsc_{user,kernel}count split is curious.  They are both for stats
> purposes alone, but there is nothing pv specific about the usercount. 
> It frankly looks as if it has been mis-implemented for HVM, despite the
> split appearing to be deliberate when it was introduced in c/s
> bf2c44f8b469.  I am really not sure what to make of it.

It's clearly used for PV only at present. Anyone wanting to change
this (including if the current implementation was really buggy) could
easily move the field back out of struct pv_domain.

>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -1767,7 +1767,7 @@ void pv_soft_rdtsc(struct vcpu *v, struc
>>      if ( guest_kernel_mode(v, regs) )
>>          d->arch.vtsc_kerncount++;
>>      else
>> -        d->arch.vtsc_usercount++;
>> +        d->arch.pv_domain.vtsc_usercount++;
>>  
>>      if ( (int64_t)(now - d->arch.vtsc_last) > 0 )
>>          d->arch.vtsc_last = now;
>> @@ -2020,17 +2020,15 @@ static void dump_softtsc(unsigned char k
>>              printk(",khz=%"PRIu32, d->arch.tsc_khz);
>>          if ( d->arch.incarnation )
>>              printk(",inc=%"PRIu32, d->arch.incarnation);
>> -        if ( !(d->arch.vtsc_kerncount | d->arch.vtsc_usercount) )
>> -        {
>> -            printk("\n");
>> -            continue;
>> -        }
>> -        if ( is_hvm_domain(d) )
>> +        if ( is_hvm_domain(d) && d->arch.vtsc_kerncount )
>>              printk(",vtsc count: %"PRIu64" total\n",
>>                     d->arch.vtsc_kerncount);
>> -        else
>> +        else if ( is_pv_domain(d) &&
>> +                  (d->arch.vtsc_kerncount | d->arch.pv_domain.vtsc_usercount) )
> 
> I realise you are simply rearanging the logic from before, but this
> should really be a logic or rather than a bitwise or, and is probably
> worth tweaking with the movement.

I disagree - for the purpose here, | vs || doesn't matter at all, and
personally (working with compilers other than gcc) I prefer | where
possible as not all compilers know to optimize || to | in cases like
this on architectures where logical machine instructions produce
usable status flags (in fact, none of the three different vendor
Windows compilers I just tried do).

Jan

Re: [PATCH] x86: re-order struct arch_domain fields

[Jan Beulich]

>>> On 19.01.15 at 18:52, <andrew.cooper3@citrix.com> wrote:
> On 19/01/15 15:41, Jan Beulich wrote:
>> ... to reduce padding holes. While doing this I noticed vtsc_usercount
>> is a PV-only thing, so it gets moved straight to struct pv_domain.
> 
> The vtsc_{user,kernel}count split is curious.  They are both for stats
> purposes alone, but there is nothing pv specific about the usercount. 
> It frankly looks as if it has been mis-implemented for HVM, despite the
> split appearing to be deliberate when it was introduced in c/s
> bf2c44f8b469.  I am really not sure what to make of it.

Since I didn't hear back on my earlier response, I looked at this again:
Considering especially the explicit callers of hvm_get_guest_tsc_fixed(),
I also wonder whether the accounting for HVM makes sense in the
first place - to me, these two numbers are meant to be _only_ counting
actual emulations. Hence I first of all would think this ought to be
moved into hvm_rdtsc_intercept() (and maybe mirrored in
hvm_msr_read_intercept(), perhaps by refactoring the former to be
usable by the latter).

In that case it would indeed make sense to keep the user count for
non-PV, as then it really makes sense to check for user/kernel mode
there.

Jan

Re: [PATCH] x86: re-order struct arch_domain fields

[Andrew Cooper]

On 03/02/15 09:45, Jan Beulich wrote:
>>>> On 19.01.15 at 18:52, <andrew.cooper3@citrix.com> wrote:
>> On 19/01/15 15:41, Jan Beulich wrote:
>>> ... to reduce padding holes. While doing this I noticed vtsc_usercount
>>> is a PV-only thing, so it gets moved straight to struct pv_domain.
>> The vtsc_{user,kernel}count split is curious.  They are both for stats
>> purposes alone, but there is nothing pv specific about the usercount. 
>> It frankly looks as if it has been mis-implemented for HVM, despite the
>> split appearing to be deliberate when it was introduced in c/s
>> bf2c44f8b469.  I am really not sure what to make of it.
> Since I didn't hear back on my earlier response, I looked at this again:
> Considering especially the explicit callers of hvm_get_guest_tsc_fixed(),
> I also wonder whether the accounting for HVM makes sense in the
> first place - to me, these two numbers are meant to be _only_ counting
> actual emulations. Hence I first of all would think this ought to be
> moved into hvm_rdtsc_intercept() (and maybe mirrored in
> hvm_msr_read_intercept(), perhaps by refactoring the former to be
> usable by the latter).
>
> In that case it would indeed make sense to keep the user count for
> non-PV, as then it really makes sense to check for user/kernel mode
> there.
>
> Jan
>

I agree with your analysis.  Now that you point it out, the current code
does look wrong.

~Andrew