[PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET

[PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET

[Jan Beulich]

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

This wasn't defined correctly, thus allowing in the
num_online_cpus() == NR_CPUS case to pass a corrupted MFN to
Dom0.

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

--- 2010-06-15.orig/xen/common/trace.c	2010-06-28 11:58:37.000000000 +0200
+++ 2010-06-15/xen/common/trace.c	2010-06-28 11:58:37.000000000 +0200
@@ -51,7 +51,7 @@ static struct t_info *t_info;
 #define T_INFO_PAGES 2  /* Size fixed at 2 pages for now. */
 #define T_INFO_SIZE ((T_INFO_PAGES)*(PAGE_SIZE))
 /* t_info.tbuf_size + list of mfn offsets + 1 to round up / sizeof uint32_t */
-#define T_INFO_FIRST_OFFSET ((sizeof(int16_t) + NR_CPUS * sizeof(int16_t) + 1) / sizeof(uint32_t))
+#define T_INFO_FIRST_OFFSET (((2 + NR_CPUS) * sizeof(uint16_t)) / sizeof(uint32_t))
 static DEFINE_PER_CPU_READ_MOSTLY(struct t_buf *, t_bufs);
 static DEFINE_PER_CPU_READ_MOSTLY(unsigned char *, t_data);
 static DEFINE_PER_CPU_READ_MOSTLY(spinlock_t, t_lock);




[-- Attachment #2: trace-t_info-first-offset.patch --]
[-- Type: text/plain, Size: 934 bytes --]

This wasn't defined correctly, thus allowing in the
num_online_cpus() == NR_CPUS case to pass a corrupted MFN to
Dom0.

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

--- 2010-06-15.orig/xen/common/trace.c	2010-06-28 11:58:37.000000000 +0200
+++ 2010-06-15/xen/common/trace.c	2010-06-28 11:58:37.000000000 +0200
@@ -51,7 +51,7 @@ static struct t_info *t_info;
 #define T_INFO_PAGES 2  /* Size fixed at 2 pages for now. */
 #define T_INFO_SIZE ((T_INFO_PAGES)*(PAGE_SIZE))
 /* t_info.tbuf_size + list of mfn offsets + 1 to round up / sizeof uint32_t */
-#define T_INFO_FIRST_OFFSET ((sizeof(int16_t) + NR_CPUS * sizeof(int16_t) + 1) / sizeof(uint32_t))
+#define T_INFO_FIRST_OFFSET (((2 + NR_CPUS) * sizeof(uint16_t)) / sizeof(uint32_t))
 static DEFINE_PER_CPU_READ_MOSTLY(struct t_buf *, t_bufs);
 static DEFINE_PER_CPU_READ_MOSTLY(unsigned char *, t_data);
 static DEFINE_PER_CPU_READ_MOSTLY(spinlock_t, t_lock);

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

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

Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET

[George Dunlap]

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

Good catch.  This fix produces the correct index, but:
* It's not as clear, IMHO, where the math is coming from
* I think it may give the wrong result if the t_info struct ever
changes (e.g., more data before the cpu offset list)

The bug in the original math was that I should have added 3 to round
up, rather than 1.

I'm attaching a patch that will hopefully fix the bug and make it more
clear.  Thoughts?

 -George

On Tue, Jun 29, 2010 at 4:32 PM, Jan Beulich <JBeulich@novell.com> wrote:
> This wasn't defined correctly, thus allowing in the
> num_online_cpus() == NR_CPUS case to pass a corrupted MFN to
> Dom0.
>
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>
> --- 2010-06-15.orig/xen/common/trace.c  2010-06-28 11:58:37.000000000 +0200
> +++ 2010-06-15/xen/common/trace.c       2010-06-28 11:58:37.000000000 +0200
> @@ -51,7 +51,7 @@ static struct t_info *t_info;
>  #define T_INFO_PAGES 2  /* Size fixed at 2 pages for now. */
>  #define T_INFO_SIZE ((T_INFO_PAGES)*(PAGE_SIZE))
>  /* t_info.tbuf_size + list of mfn offsets + 1 to round up / sizeof uint32_t */
> -#define T_INFO_FIRST_OFFSET ((sizeof(int16_t) + NR_CPUS * sizeof(int16_t) + 1) / sizeof(uint32_t))
> +#define T_INFO_FIRST_OFFSET (((2 + NR_CPUS) * sizeof(uint16_t)) / sizeof(uint32_t))
>  static DEFINE_PER_CPU_READ_MOSTLY(struct t_buf *, t_bufs);
>  static DEFINE_PER_CPU_READ_MOSTLY(unsigned char *, t_data);
>  static DEFINE_PER_CPU_READ_MOSTLY(spinlock_t, t_lock);
>
>
>
>

[-- Attachment #2: trace-t_info-first-offset-v2.patch --]
[-- Type: text/x-diff, Size: 1308 bytes --]

Fix T_INFO_FIRST_OFFSET calculation

This wasn't defined correctly, thus allowing in the
num_online_cpus() == NR_CPUS case to pass a corrupted MFN to
Dom0.

Reported-by: Jan Beulich <jbeulich@novell.com>
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 39f71ef422bd xen/common/trace.c
--- a/xen/common/trace.c	Tue Jun 29 17:23:53 2010 +0100
+++ b/xen/common/trace.c	Wed Jun 30 15:50:33 2010 +0100
@@ -50,8 +50,11 @@
 static struct t_info *t_info;
 #define T_INFO_PAGES 2  /* Size fixed at 2 pages for now. */
 #define T_INFO_SIZE ((T_INFO_PAGES)*(PAGE_SIZE))
-/* t_info.tbuf_size + list of mfn offsets + 1 to round up / sizeof uint32_t */
-#define T_INFO_FIRST_OFFSET ((sizeof(int16_t) + NR_CPUS * sizeof(int16_t) + 1) / sizeof(uint32_t))
+/* Return the number of elements _type necessary to store at least _x bytes of data
+ * i.e., sizeof(_type) * ans >= _x. */
+#define fit_to_type(_type, _x) (((_x)+sizeof(_type)-1) / sizeof(_type))
+/* t_info.tbuf_size + list of mfn offsets rounded up / sizeof uint32_t */
+#define T_INFO_FIRST_OFFSET fit_to_type( uint32_t, (sizeof(int16_t) + NR_CPUS * sizeof(int16_t)) )
 static DEFINE_PER_CPU_READ_MOSTLY(struct t_buf *, t_bufs);
 static DEFINE_PER_CPU_READ_MOSTLY(unsigned char *, t_data);
 static DEFINE_PER_CPU_READ_MOSTLY(spinlock_t, t_lock);

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

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

Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET

[Jan Beulich]

>>> On 30.06.10 at 16:52, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
> Good catch.  This fix produces the correct index, but:
> * It's not as clear, IMHO, where the math is coming from
> * I think it may give the wrong result if the t_info struct ever
> changes (e.g., more data before the cpu offset list)

That part your patch doesn't address either - rather than
sizeof(uint16_t) as the first part of the expression you'd need to
use sizeof(struct t_info) or offsetof(struct t_info, mfn_offset).

Btw., didn't we agree that public headers shouldn't make use of
language extensions? struct t_info uses a variable sized array,
which is an extension (standard only in C99).

> The bug in the original math was that I should have added 3 to round
> up, rather than 1.

Correct.

> I'm attaching a patch that will hopefully fix the bug and make it more
> clear.  Thoughts?

Leaving aside the comment above this looks okay to me.

Jan

Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET

[George Dunlap]

Jan Beulich wrote:
> That part your patch doesn't address either - rather than
> sizeof(uint16_t) as the first part of the expression you'd need to
> use sizeof(struct t_info) or offsetof(struct t_info, mfn_offset).
>   
I was assuming that when someone changed struct t_info that they'd 
modify this macro as well; I suppose then that the two complaints are 
really different aspects of the same one -- that it might not be clear 
to the person who adjusts struct t_info how to translate those changes 
into T_INFO_FIRST_OFFSET.  I think this way is more clear.

I suppose even better might be to calculate 
t_info.mfn_mfn_offset[NR_CPUS] (or perhaps ...[num_possible_cpus]).  
Hmm... let me see what I can come up with.
> Btw., didn't we agree that public headers shouldn't make use of
> language extensions? struct t_info uses a variable sized array,
> which is an extension (standard only in C99).
>   
I'm not an expert in this.  It's lot more hassle to lay out the data the 
way I'd like without it.  I'll defer judgment to Keir.

 -George

Re: Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET

[George Dunlap]

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

Here's a version that calculates t_info_first_offset during
initialization, based on the actual layout of struct t_info and
NR_CPUs.

 -George

On Wed, Jun 30, 2010 at 4:28 PM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> Jan Beulich wrote:
>>
>> That part your patch doesn't address either - rather than
>> sizeof(uint16_t) as the first part of the expression you'd need to
>> use sizeof(struct t_info) or offsetof(struct t_info, mfn_offset).
>>
>
> I was assuming that when someone changed struct t_info that they'd modify
> this macro as well; I suppose then that the two complaints are really
> different aspects of the same one -- that it might not be clear to the
> person who adjusts struct t_info how to translate those changes into
> T_INFO_FIRST_OFFSET.  I think this way is more clear.
>
> I suppose even better might be to calculate t_info.mfn_mfn_offset[NR_CPUS]
> (or perhaps ...[num_possible_cpus]).  Hmm... let me see what I can come up
> with.
>>
>> Btw., didn't we agree that public headers shouldn't make use of
>> language extensions? struct t_info uses a variable sized array,
>> which is an extension (standard only in C99).
>>
>
> I'm not an expert in this.  It's lot more hassle to lay out the data the way
> I'd like without it.  I'll defer judgment to Keir.
>
> -George
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

[-- Attachment #2: trace-t_info-first-offset-v3.patch --]
[-- Type: text/x-diff, Size: 2815 bytes --]

Fix T_INFO_FIRST_OFFSET calculation

This wasn't defined correctly, thus allowing in the
num_online_cpus() == NR_CPUS case to pass a corrupted MFN to
Dom0.

Reported-by: Jan Beulich <jbeulich@novell.com>
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 39f71ef422bd xen/common/trace.c
--- a/xen/common/trace.c	Tue Jun 29 17:23:53 2010 +0100
+++ b/xen/common/trace.c	Wed Jun 30 17:01:59 2010 +0100
@@ -50,12 +50,11 @@
 static struct t_info *t_info;
 #define T_INFO_PAGES 2  /* Size fixed at 2 pages for now. */
 #define T_INFO_SIZE ((T_INFO_PAGES)*(PAGE_SIZE))
-/* t_info.tbuf_size + list of mfn offsets + 1 to round up / sizeof uint32_t */
-#define T_INFO_FIRST_OFFSET ((sizeof(int16_t) + NR_CPUS * sizeof(int16_t) + 1) / sizeof(uint32_t))
 static DEFINE_PER_CPU_READ_MOSTLY(struct t_buf *, t_bufs);
 static DEFINE_PER_CPU_READ_MOSTLY(unsigned char *, t_data);
 static DEFINE_PER_CPU_READ_MOSTLY(spinlock_t, t_lock);
 static int data_size;
+static u32 t_info_first_offset __read_mostly;
 
 /* High water mark for trace buffers; */
 /* Send virtual interrupt when buffer level reaches this point */
@@ -75,13 +74,29 @@
 /* which tracing events are enabled */
 static u32 tb_event_mask = TRC_ALL;
 
+/* Return the number of elements _type necessary to store at least _x bytes of data
+ * i.e., sizeof(_type) * ans >= _x. */
+#define fit_to_type(_type, _x) (((_x)+sizeof(_type)-1) / sizeof(_type))
+
+static void calc_tinfo_first_offset(void)
+{
+    int offset_in_bytes;
+    
+    offset_in_bytes = ((char *)&((struct t_info *)(NULL))->mfn_offset[NR_CPUS]) - (char *)(NULL);
+
+    t_info_first_offset = fit_to_type(uint32_t, offset_in_bytes);
+
+    printk("%s: NR_CPUs %d, offset_in_bytes %d, t_info_first_offset %u\n",
+           __func__, NR_CPUS, offset_in_bytes, (unsigned)t_info_first_offset);
+}
+
 /**
  * check_tbuf_size - check to make sure that the proposed size will fit
  * in the currently sized struct t_info.
  */
 static inline int check_tbuf_size(int size)
 {
-    return (num_online_cpus() * size + T_INFO_FIRST_OFFSET) > (T_INFO_SIZE / sizeof(uint32_t));
+    return (num_online_cpus() * size + t_info_first_offset) > (T_INFO_SIZE / sizeof(uint32_t));
 }
 
 /**
@@ -100,7 +115,7 @@
     unsigned long nr_pages;
     /* Start after a fixed-size array of NR_CPUS */
     uint32_t *t_info_mfn_list = (uint32_t *)t_info;
-    int offset = T_INFO_FIRST_OFFSET;
+    int offset = t_info_first_offset;
 
     BUG_ON(check_tbuf_size(opt_tbuf_size));
 
@@ -293,6 +308,10 @@
 void __init init_trace_bufs(void)
 {
     int i;
+
+    /* Calculate offset in u32 of first mfn */
+    calc_tinfo_first_offset();
+
     /* t_info size fixed at 2 pages for now.  That should be big enough / small enough
      * until it's worth making it dynamic. */
     t_info = alloc_xenheap_pages(1, 0);

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

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

Re: Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET

[George Dunlap]

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

Oops, please use this version, which used the appropraite gkprintk() settings...

 -George

On Wed, Jun 30, 2010 at 5:10 PM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> Here's a version that calculates t_info_first_offset during
> initialization, based on the actual layout of struct t_info and
> NR_CPUs.
>
>  -George
>
> On Wed, Jun 30, 2010 at 4:28 PM, George Dunlap
> <george.dunlap@eu.citrix.com> wrote:
>> Jan Beulich wrote:
>>>
>>> That part your patch doesn't address either - rather than
>>> sizeof(uint16_t) as the first part of the expression you'd need to
>>> use sizeof(struct t_info) or offsetof(struct t_info, mfn_offset).
>>>
>>
>> I was assuming that when someone changed struct t_info that they'd modify
>> this macro as well; I suppose then that the two complaints are really
>> different aspects of the same one -- that it might not be clear to the
>> person who adjusts struct t_info how to translate those changes into
>> T_INFO_FIRST_OFFSET.  I think this way is more clear.
>>
>> I suppose even better might be to calculate t_info.mfn_mfn_offset[NR_CPUS]
>> (or perhaps ...[num_possible_cpus]).  Hmm... let me see what I can come up
>> with.
>>>
>>> Btw., didn't we agree that public headers shouldn't make use of
>>> language extensions? struct t_info uses a variable sized array,
>>> which is an extension (standard only in C99).
>>>
>>
>> I'm not an expert in this.  It's lot more hassle to lay out the data the way
>> I'd like without it.  I'll defer judgment to Keir.
>>
>> -George
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>>
>

[-- Attachment #2: trace-t_info-first-offset-v3.patch --]
[-- Type: text/x-diff, Size: 2830 bytes --]

Fix T_INFO_FIRST_OFFSET calculation

This wasn't defined correctly, thus allowing in the
num_online_cpus() == NR_CPUS case to pass a corrupted MFN to
Dom0.

Reported-by: Jan Beulich <jbeulich@novell.com>
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 39f71ef422bd xen/common/trace.c
--- a/xen/common/trace.c	Tue Jun 29 17:23:53 2010 +0100
+++ b/xen/common/trace.c	Wed Jun 30 17:11:10 2010 +0100
@@ -50,12 +50,11 @@
 static struct t_info *t_info;
 #define T_INFO_PAGES 2  /* Size fixed at 2 pages for now. */
 #define T_INFO_SIZE ((T_INFO_PAGES)*(PAGE_SIZE))
-/* t_info.tbuf_size + list of mfn offsets + 1 to round up / sizeof uint32_t */
-#define T_INFO_FIRST_OFFSET ((sizeof(int16_t) + NR_CPUS * sizeof(int16_t) + 1) / sizeof(uint32_t))
 static DEFINE_PER_CPU_READ_MOSTLY(struct t_buf *, t_bufs);
 static DEFINE_PER_CPU_READ_MOSTLY(unsigned char *, t_data);
 static DEFINE_PER_CPU_READ_MOSTLY(spinlock_t, t_lock);
 static int data_size;
+static u32 t_info_first_offset __read_mostly;
 
 /* High water mark for trace buffers; */
 /* Send virtual interrupt when buffer level reaches this point */
@@ -75,13 +74,29 @@
 /* which tracing events are enabled */
 static u32 tb_event_mask = TRC_ALL;
 
+/* Return the number of elements _type necessary to store at least _x bytes of data
+ * i.e., sizeof(_type) * ans >= _x. */
+#define fit_to_type(_type, _x) (((_x)+sizeof(_type)-1) / sizeof(_type))
+
+static void calc_tinfo_first_offset(void)
+{
+    int offset_in_bytes;
+    
+    offset_in_bytes = ((char *)&((struct t_info *)(NULL))->mfn_offset[NR_CPUS]) - (char *)(NULL);
+
+    t_info_first_offset = fit_to_type(uint32_t, offset_in_bytes);
+
+    gdprintk(XENLOG_INFO, "%s: NR_CPUs %d, offset_in_bytes %d, t_info_first_offset %u\n",
+           __func__, NR_CPUS, offset_in_bytes, (unsigned)t_info_first_offset);
+}
+
 /**
  * check_tbuf_size - check to make sure that the proposed size will fit
  * in the currently sized struct t_info.
  */
 static inline int check_tbuf_size(int size)
 {
-    return (num_online_cpus() * size + T_INFO_FIRST_OFFSET) > (T_INFO_SIZE / sizeof(uint32_t));
+    return (num_online_cpus() * size + t_info_first_offset) > (T_INFO_SIZE / sizeof(uint32_t));
 }
 
 /**
@@ -100,7 +115,7 @@
     unsigned long nr_pages;
     /* Start after a fixed-size array of NR_CPUS */
     uint32_t *t_info_mfn_list = (uint32_t *)t_info;
-    int offset = T_INFO_FIRST_OFFSET;
+    int offset = t_info_first_offset;
 
     BUG_ON(check_tbuf_size(opt_tbuf_size));
 
@@ -293,6 +308,10 @@
 void __init init_trace_bufs(void)
 {
     int i;
+
+    /* Calculate offset in u32 of first mfn */
+    calc_tinfo_first_offset();
+
     /* t_info size fixed at 2 pages for now.  That should be big enough / small enough
      * until it's worth making it dynamic. */
     t_info = alloc_xenheap_pages(1, 0);

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

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

Re: Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET

[Keir Fraser]

I've lost track of all these trace patches. Please send a new set of patches
with finalised Sign-offs and Acks as appropriate when you reach agreement.

 -- Keir

On 30/06/2010 17:12, "George Dunlap" <George.Dunlap@eu.citrix.com> wrote:

> Oops, please use this version, which used the appropraite gkprintk()
> settings...
> 
>  -George
> 
> On Wed, Jun 30, 2010 at 5:10 PM, George Dunlap
> <George.Dunlap@eu.citrix.com> wrote:
>> Here's a version that calculates t_info_first_offset during
>> initialization, based on the actual layout of struct t_info and
>> NR_CPUs.
>> 
>>  -George
>> 
>> On Wed, Jun 30, 2010 at 4:28 PM, George Dunlap
>> <george.dunlap@eu.citrix.com> wrote:
>>> Jan Beulich wrote:
>>>> 
>>>> That part your patch doesn't address either - rather than
>>>> sizeof(uint16_t) as the first part of the expression you'd need to
>>>> use sizeof(struct t_info) or offsetof(struct t_info, mfn_offset).
>>>> 
>>> 
>>> I was assuming that when someone changed struct t_info that they'd modify
>>> this macro as well; I suppose then that the two complaints are really
>>> different aspects of the same one -- that it might not be clear to the
>>> person who adjusts struct t_info how to translate those changes into
>>> T_INFO_FIRST_OFFSET.  I think this way is more clear.
>>> 
>>> I suppose even better might be to calculate t_info.mfn_mfn_offset[NR_CPUS]
>>> (or perhaps ...[num_possible_cpus]).  Hmm... let me see what I can come up
>>> with.
>>>> 
>>>> Btw., didn't we agree that public headers shouldn't make use of
>>>> language extensions? struct t_info uses a variable sized array,
>>>> which is an extension (standard only in C99).
>>>> 
>>> 
>>> I'm not an expert in this.  It's lot more hassle to lay out the data the way
>>> I'd like without it.  I'll defer judgment to Keir.
>>> 
>>> -George
>>> 
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xensource.com
>>> http://lists.xensource.com/xen-devel
>>> 
>> 

Re: Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET

[George Dunlap]

Jan,

I've got all the ones I've acked / sent my own versions of in a 
mercurial queue, easy to patchbomb.  Let me know if you have any more 
feedback on my versions.

My only objection to the last one was the volatile stuff; if you're OK 
with it, I'll send a series with everything but the volatile stuff, and 
we can continue talking about it.

 -George

Keir Fraser wrote:
> I've lost track of all these trace patches. Please send a new set of patches
> with finalised Sign-offs and Acks as appropriate when you reach agreement.
>
>  -- Keir
>
> On 30/06/2010 17:12, "George Dunlap" <George.Dunlap@eu.citrix.com> wrote:
>
>   
>> Oops, please use this version, which used the appropraite gkprintk()
>> settings...
>>
>>  -George
>>
>> On Wed, Jun 30, 2010 at 5:10 PM, George Dunlap
>> <George.Dunlap@eu.citrix.com> wrote:
>>     
>>> Here's a version that calculates t_info_first_offset during
>>> initialization, based on the actual layout of struct t_info and
>>> NR_CPUs.
>>>
>>>  -George
>>>
>>> On Wed, Jun 30, 2010 at 4:28 PM, George Dunlap
>>> <george.dunlap@eu.citrix.com> wrote:
>>>       
>>>> Jan Beulich wrote:
>>>>         
>>>>> That part your patch doesn't address either - rather than
>>>>> sizeof(uint16_t) as the first part of the expression you'd need to
>>>>> use sizeof(struct t_info) or offsetof(struct t_info, mfn_offset).
>>>>>
>>>>>           
>>>> I was assuming that when someone changed struct t_info that they'd modify
>>>> this macro as well; I suppose then that the two complaints are really
>>>> different aspects of the same one -- that it might not be clear to the
>>>> person who adjusts struct t_info how to translate those changes into
>>>> T_INFO_FIRST_OFFSET.  I think this way is more clear.
>>>>
>>>> I suppose even better might be to calculate t_info.mfn_mfn_offset[NR_CPUS]
>>>> (or perhaps ...[num_possible_cpus]).  Hmm... let me see what I can come up
>>>> with.
>>>>         
>>>>> Btw., didn't we agree that public headers shouldn't make use of
>>>>> language extensions? struct t_info uses a variable sized array,
>>>>> which is an extension (standard only in C99).
>>>>>
>>>>>           
>>>> I'm not an expert in this.  It's lot more hassle to lay out the data the way
>>>> I'd like without it.  I'll defer judgment to Keir.
>>>>
>>>> -George
>>>>
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> Xen-devel@lists.xensource.com
>>>> http://lists.xensource.com/xen-devel
>>>>
>>>>         
>
>
>   

Re: Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET

[Jan Beulich]

>>> On 30.06.10 at 18:12, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
>+    offset_in_bytes = ((char *)&((struct t_info *)(NULL))->mfn_offset[NR_CPUS]) - (char *)(NULL);

Why can't this just be offsetof(struct t_info, mfn_offset[NR_CPUS])?

Jan

Re: Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET

[Jan Beulich]

>>> On 30.06.10 at 20:12, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> I've got all the ones I've acked / sent my own versions of in a 
> mercurial queue, easy to patchbomb.  Let me know if you have any more 
> feedback on my versions.
> 
> My only objection to the last one was the volatile stuff; if you're OK 
> with it, I'll send a series with everything but the volatile stuff, and 
> we can continue talking about it.

Sounds good to me.

Jan

Re: Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET

[George Dunlap]

On 01/07/10 10:49, Jan Beulich wrote:
>>>> On 30.06.10 at 18:12, George Dunlap<George.Dunlap@eu.citrix.com>  wrote:
>> +    offset_in_bytes = ((char *)&((struct t_info *)(NULL))->mfn_offset[NR_CPUS]) - (char *)(NULL);
>
> Why can't this just be offsetof(struct t_info, mfn_offset[NR_CPUS])?

Ah... forgot about that one.  Thanks, I'll modify that in my patch series.

  -George