[Linux-ia64] [patch] 2.4.20-021210 misaligned sal error record

[Linux-ia64] [patch] 2.4.20-021210 misaligned sal error record

[Keith Owens]

The fr entries in struct sal_processor_static_info are 16 bytes wide
but may not start on a 16 byte boundary in the sal record (it depends
on the size of min_state).  Stop gcc from assuming that the fr entries
are 16 byte aligned.

This was misaligning the fr records within sal_processor_static_info.
In addition, because gcc thought that sal_processor_static_info
contained a 16 byte field, sal_processor_static_info was being forced
to 16 byte alignment within struct err_rec, destroying the mapping of
the sal error record (preceding sal record header is only 40 bytes).

Within sal_log_processor_info, the sal_log_mod_error_info_t entries are
fixed size but have a variable number of entries.  Change the
definition from 16 (max) to 0 entries and add a function to calculate
the address of the processor static info within sal_log_processor_info,
based on what sal actually supplied.  Use that function in
ia64_init_handler() to get the min_state data.

Against 2.4.20-ia64-021210.

Index: 20.5/include/asm-ia64/sal.h
--- 20.5/include/asm-ia64/sal.h Wed, 11 Dec 2002 20:58:53 +1100 kaos (linux-2.4/s/47_sal.h 1.1.3.2.3.1.1.1.1.3 644)
+++ 20.5(w)/include/asm-ia64/sal.h Tue, 25 Feb 2003 11:17:04 +1100 kaos (linux-2.4/s/47_sal.h 1.1.3.2.3.1.1.1.1.3 644)
@@ -351,7 +351,7 @@ typedef struct sal_processor_static_info
     u64                     ar[128];
     u64                     rr[8];
     struct ia64_fpreg       fr[128];
-} sal_processor_static_info_t;
+} __attribute__((packed)) sal_processor_static_info_t;
 
 typedef struct sal_log_processor_info
 {
@@ -373,11 +373,11 @@ typedef struct sal_log_processor_info
     u64                         proc_error_map;
     u64                         proc_state_parameter;
     u64                         proc_cr_lid;
-    sal_log_mod_error_info_t    cache_check_info[16];
-    sal_log_mod_error_info_t    tlb_check_info[16];
-    sal_log_mod_error_info_t    bus_check_info[16];
-    sal_log_mod_error_info_t    reg_file_check_info[16];
-    sal_log_mod_error_info_t    ms_check_info[16];
+    sal_log_mod_error_info_t    cache_check_info[0];
+    sal_log_mod_error_info_t    tlb_check_info[0];
+    sal_log_mod_error_info_t    bus_check_info[0];
+    sal_log_mod_error_info_t    reg_file_check_info[0];
+    sal_log_mod_error_info_t    ms_check_info[0];
     struct
     {
         u64 regs[5];
@@ -386,6 +386,23 @@ typedef struct sal_log_processor_info
     sal_processor_static_info_t processor_static_info;
 } sal_log_processor_info_t;
 
+/* Position of processor_static_info within processor_info is variable */
+static inline
+sal_processor_static_info_t *addr_processor_static_info(sal_log_processor_info_t *p)
+{
+	sal_processor_static_info_t *s +	(sal_processor_static_info_t *)(
+		((char *) &(p->processor_static_info)) + 
+		  (p->valid.num_cache_check +
+		   p->valid.num_tlb_check +
+		   p->valid.num_bus_check +
+		   p->valid.num_reg_file_check +
+		   p->valid.num_ms_check
+		  ) * sizeof(sal_log_mod_error_info_t)
+	);
+	return s;
+}
+
 /* platform error log structures */
 
 typedef struct sal_log_mem_dev_err_info
Index: 20.5/arch/ia64/kernel/mca.c
--- 20.5/arch/ia64/kernel/mca.c Wed, 11 Dec 2002 20:58:53 +1100 kaos (linux-2.4/s/c/5_mca.c 1.1.3.2.3.1.1.1.1.3 644)
+++ 20.5(w)/arch/ia64/kernel/mca.c Tue, 25 Feb 2003 11:14:08 +1100 kaos (linux-2.4/s/c/5_mca.c 1.1.3.2.3.1.1.1.1.3 644)
@@ -910,7 +910,7 @@ ia64_init_handler (struct pt_regs *regs)
 	plog_ptr=(ia64_err_rec_t *)IA64_LOG_CURR_BUFFER(SAL_INFO_TYPE_INIT);
 	proc_ptr = &plog_ptr->proc_err;
 
-	ia64_process_min_state_save(&proc_ptr->processor_static_info.min_state_area);
+	ia64_process_min_state_save(&(addr_processor_static_info(proc_ptr)->min_state_area));
 
 	/* Clear the INIT SAL logs now that they have been saved in the OS buffer */
 	ia64_sal_clear_state_info(SAL_INFO_TYPE_INIT);

Re: [Linux-ia64] [patch] 2.4.20-021210 misaligned sal error record

[David Mosberger]

>>>>> On Tue, 25 Feb 2003 11:24:35 +1100, Keith Owens <kaos@sgi.com> said:

  Keith> -    sal_log_mod_error_info_t    cache_check_info[16];
  Keith> -    sal_log_mod_error_info_t    tlb_check_info[16];
  Keith> -    sal_log_mod_error_info_t    bus_check_info[16];
  Keith> -    sal_log_mod_error_info_t    reg_file_check_info[16];
  Keith> -    sal_log_mod_error_info_t    ms_check_info[16];
  Keith> +    sal_log_mod_error_info_t    cache_check_info[0];
  Keith> +    sal_log_mod_error_info_t    tlb_check_info[0];
  Keith> +    sal_log_mod_error_info_t    bus_check_info[0];
  Keith> +    sal_log_mod_error_info_t    reg_file_check_info[0];
  Keith> +    sal_log_mod_error_info_t    ms_check_info[0];

Somehow I doubt a declaration of this form is a good idea.  If the
records are all variable length, wouldn't it be saner to just declare
this with something along the lines of:

  Keith> +    sal_log_mod_error_info_t    check_info[0];

and then provide separate macros to access the indiviual portions?

	--david

Re: [Linux-ia64] [patch] 2.4.20-021210 misaligned sal error record

[Keith Owens]

On Mon, 24 Feb 2003 17:42:48 -0800, 
David Mosberger <davidm@napali.hpl.hp.com> wrote:
>>>>>> On Tue, 25 Feb 2003 11:24:35 +1100, Keith Owens <kaos@sgi.com> said:
>
>  Keith> -    sal_log_mod_error_info_t    cache_check_info[16];
>  Keith> -    sal_log_mod_error_info_t    tlb_check_info[16];
>  Keith> -    sal_log_mod_error_info_t    bus_check_info[16];
>  Keith> -    sal_log_mod_error_info_t    reg_file_check_info[16];
>  Keith> -    sal_log_mod_error_info_t    ms_check_info[16];
>  Keith> +    sal_log_mod_error_info_t    cache_check_info[0];
>  Keith> +    sal_log_mod_error_info_t    tlb_check_info[0];
>  Keith> +    sal_log_mod_error_info_t    bus_check_info[0];
>  Keith> +    sal_log_mod_error_info_t    reg_file_check_info[0];
>  Keith> +    sal_log_mod_error_info_t    ms_check_info[0];
>
>Somehow I doubt a declaration of this form is a good idea.  If the
>records are all variable length, wouldn't it be saner to just declare
>this with something along the lines of:
>
>  Keith> +    sal_log_mod_error_info_t    check_info[0];
>
>and then provide separate macros to access the indiviual portions?

Either works, but nobody really cares about the various *check_info
sections.  What we care about is the processor static data that comes
after these sections and is addressed via the new function.  This was
the minimal change to correctly access the processor static data, it is
a one line change to mca.c.

Replacing the individual *check_info[0] sections with a single
check_info[0] means more changes to mca.c to use the new names.  It
also diverges from the SAL specification which lists the individual
fields.  Since the end result would be exactly the same as the existing
code, I went for the minimal change and kept SAL documentation
compatibility.  Blame the SAL docs, I do ;).

Re: [Linux-ia64] [patch] 2.4.20-021210 misaligned sal error record

[David Mosberger]

>>>>> On Tue, 25 Feb 2003 12:53:35 +1100, Keith Owens <kaos@sgi.com> said:

  Keith> On Mon, 24 Feb 2003 17:42:48 -0800, David Mosberger
  Keith> <davidm@napali.hpl.hp.com> wrote:
  >>>>>>> On Tue, 25 Feb 2003 11:24:35 +1100, Keith Owens
  >>>>>>> <kaos@sgi.com> said:
  >>
  Keith> - sal_log_mod_error_info_t cache_check_info[16]; -
  Keith> sal_log_mod_error_info_t tlb_check_info[16]; -
  Keith> sal_log_mod_error_info_t bus_check_info[16]; -
  Keith> sal_log_mod_error_info_t reg_file_check_info[16]; -
  Keith> sal_log_mod_error_info_t ms_check_info[16]; +
  Keith> sal_log_mod_error_info_t cache_check_info[0]; +
  Keith> sal_log_mod_error_info_t tlb_check_info[0]; +
  Keith> sal_log_mod_error_info_t bus_check_info[0]; +
  Keith> sal_log_mod_error_info_t reg_file_check_info[0]; +
  Keith> sal_log_mod_error_info_t ms_check_info[0];
  >>  Somehow I doubt a declaration of this form is a good idea.  If
  >> the records are all variable length, wouldn't it be saner to just
  >> declare this with something along the lines of:
  >> 
  Keith> + sal_log_mod_error_info_t check_info[0];
  >>  and then provide separate macros to access the indiviual
  >> portions?

  Keith> Either works, but nobody really cares about the various
  Keith> *check_info sections.  What we care about is the processor
  Keith> static data that comes after these sections and is addressed
  Keith> via the new function.  This was the minimal change to
  Keith> correctly access the processor static data, it is a one line
  Keith> change to mca.c.

  Keith> Replacing the individual *check_info[0] sections with a
  Keith> single check_info[0] means more changes to mca.c to use the
  Keith> new names.  It also diverges from the SAL specification which
  Keith> lists the individual fields.  Since the end result would be
  Keith> exactly the same as the existing code, I went for the minimal
  Keith> change and kept SAL documentation compatibility.  Blame the
  Keith> SAL docs, I do ;).

I don't think the proper solution requires many more changes to mca.c.
If we don't fix it properly, I'm sure it will trip someone again
later on.

Below is a proposed patch (btw: I think your patch had a bug:
addr_processor_static_info() didn't skip the cpuid structure).

The patch breaks mca_test(), which is used only if MCA_TEST is
defined.  But the old code worked for the wrong reasons, so I
think this needs updating anyhow.

Could someone test this to verify it works as intended (it does
compile, but that's as far as I tested it).

Thanks,

	--david

=== arch/ia64/kernel/mca.c 1.17 vs edited ==--- 1.17/arch/ia64/kernel/mca.c	Tue Feb  4 17:06:10 2003
+++ edited/arch/ia64/kernel/mca.c	Tue Mar  4 15:37:15 2003
@@ -825,7 +825,7 @@
 	plog_ptr=(ia64_err_rec_t *)IA64_LOG_CURR_BUFFER(SAL_INFO_TYPE_INIT);
 	proc_ptr = &plog_ptr->proc_err;
 
-	ia64_process_min_state_save(&proc_ptr->processor_static_info.min_state_area);
+	ia64_process_min_state_save(&SAL_LPI_PSI_INFO(proc_ptr)->min_state_area);
 
 	/* Clear the INIT SAL logs now that they have been saved in the OS buffer */
 	ia64_sal_clear_state_info(SAL_INFO_TYPE_INIT);
@@ -1620,7 +1620,7 @@
 	 *  absent. Also, current implementations only allocate space for number of
 	 *  elements used.  So we walk the data pointer from here on.
 	 */
-	p_data = &slpi->cache_check_info[0];
+	p_data = &slpi->info[0];
 
 	/* Print the cache check information if any*/
 	for (i = 0 ; i < slpi->valid.num_cache_check; i++, p_data++)
=== include/asm-ia64/sal.h 1.13 vs edited ==--- 1.13/include/asm-ia64/sal.h	Thu Feb  6 11:39:47 2003
+++ edited/include/asm-ia64/sal.h	Tue Mar  4 15:48:43 2003
@@ -336,6 +336,11 @@
 	struct ia64_fpreg fr[128];
 } sal_processor_static_info_t;
 
+struct sal_cpuid_info {
+	u64 regs[5];
+	u64 reserved;
+};
+
 typedef struct sal_log_processor_info {
 	sal_log_section_hdr_t header;
 	struct {
@@ -354,17 +359,34 @@
 	u64 proc_error_map;
 	u64 proc_state_parameter;
 	u64 proc_cr_lid;
-	sal_log_mod_error_info_t cache_check_info[16];
-	sal_log_mod_error_info_t tlb_check_info[16];
-	sal_log_mod_error_info_t bus_check_info[16];
-	sal_log_mod_error_info_t reg_file_check_info[16];
-	sal_log_mod_error_info_t ms_check_info[16];
-	struct {
-		u64 regs[5];
-		u64 reserved;
-	} cpuid_info;
-	sal_processor_static_info_t processor_static_info;
+	/*
+	 * The rest of this structure consists of variable-length arrays, which can't be
+	 * expressed in C.
+	 */
+	sal_log_mod_error_info_t info[0];
+	/*
+	 * This is what the rest looked like if C supported variable-length arrays:
+	 *
+	 * sal_log_mod_error_info_t cache_check_info[.valid.num_cache_check];
+	 * sal_log_mod_error_info_t tlb_check_info[.valid.num_tlb_check];
+	 * sal_log_mod_error_info_t bus_check_info[.valid.num_bus_check];
+	 * sal_log_mod_error_info_t reg_file_check_info[.valid.num_reg_file_check];
+	 * sal_log_mod_error_info_t ms_check_info[.valid.num_ms_check];
+	 * struct sal_cpuid_info cpuid_info;
+	 * sal_processor_static_info_t processor_static_info;
+	 */
 } sal_log_processor_info_t;
+
+/* Given a sal_log_processor_info_t pointer, return a pointer to the processor_static_info: */
+#define SAL_LPI_PSI_INFO(l)								\
+({	sal_log_processor_info_t *_l = (l);						\
+	((sal_processor_static_info_t *)						\
+	 ((char *) _l + ((_l->valid.num_cache_check + _l->valid.num_tlb_check		\
+			  + _l->valid.num_bus_check + _l->valid.num_reg_file_check	\
+			  + _l->valid.num_ms_check) * sizeof(sal_log_mod_error_info_t)	\
+			 + sizeof(struct sal_cpuid_info))));				\
+})
+
 
 /* platform error log structures */
 

Re: [Linux-ia64] [patch] 2.4.20-021210 misaligned sal error record

[Keith Owens]

On Tue, 4 Mar 2003 16:01:20 -0800, 
David Mosberger <davidm@napali.hpl.hp.com> wrote:
>Below is a proposed patch (btw: I think your patch had a bug:
>addr_processor_static_info() didn't skip the cpuid structure).

No bug, I start with ((char *) &(p->processor_static_info)) so gcc has
already factored in the size of cpuid_info.  Your patch removes
cpuid_info from sal_log_processor_info_t so your calculation has to add
sizeof(cpuid_info) back in.

My patch gives accurate kdb backtraces for MCA and INIT monarch so I
know the alignments are correct.  kdb v4.0, work in progress.

>+/* Given a sal_log_processor_info_t pointer, return a pointer to the processor_static_info: */
>+#define SAL_LPI_PSI_INFO(l)								\
>+({	sal_log_processor_info_t *_l = (l);						\
>+	((sal_processor_static_info_t *)						\
>+	 ((char *) _l + ((_l->valid.num_cache_check + _l->valid.num_tlb_check		\
>+			  + _l->valid.num_bus_check + _l->valid.num_reg_file_check	\
>+			  + _l->valid.num_ms_check) * sizeof(sal_log_mod_error_info_t)	\
>+			 + sizeof(struct sal_cpuid_info))));				\
>+})

Linus recommends static inline instead of #define unless there is no
choice.  static inline does type checking, #define does not.

static inline
sal_processor_static_info_t *SAL_LPI_PSI_INFO(sal_log_processor_info_t *l)
{
	sal_processor_static_info_t *s 	(sal_processor_static_info_t *)(
		(char *) l +
		  (l->valid.num_cache_check +
		   l->valid.num_tlb_check +
		   l->valid.num_bus_check +
		   l->valid.num_reg_file_check +
		   l->valid.num_ms_check
		  ) * sizeof(sal_log_mod_error_info_t) +
		sizeof(struct sal_cpuid_info)
	);
	return s;
}

>Could someone test this to verify it works as intended (it does
>compile, but that's as far as I tested it).

Testing with kdb v4.0 now.

Re: [Linux-ia64] [patch] 2.4.20-021210 misaligned sal error record

[David Mosberger]

>>>>> On Wed, 05 Mar 2003 11:33:33 +1100, Keith Owens <kaos@sgi.com> said:

  Keith> Linus recommends static inline instead of #define unless
  Keith> there is no choice.  static inline does type checking,
  Keith> #define does not.

That's a good principle, but doesn't really apply here.  You do
get type-checking:

#define SAL_LPI_PSI_INFO(l)				\
	sal_log_processor_info_t *_l = (l);

and for this type of work-around-C-limitation helper routine, I think
a capitalized macro actually results in more readable code.  It's a
matter of taste, of course.

	--david

Re: [Linux-ia64] [patch] 2.4.20-021210 misaligned sal error record

[Bjorn Helgaas]

On Monday 24 February 2003 5:24 pm, Keith Owens wrote:
> The fr entries in struct sal_processor_static_info are 16 bytes wide
> but may not start on a 16 byte boundary in the sal record (it depends
> on the size of min_state).  Stop gcc from assuming that the fr entries
> are 16 byte aligned.
> 
> This was misaligning the fr records within sal_processor_static_info.
> In addition, because gcc thought that sal_processor_static_info
> contained a 16 byte field, sal_processor_static_info was being forced
> to 16 byte alignment within struct err_rec, destroying the mapping of
> the sal error record (preceding sal record header is only 40 bytes).
> 
> Within sal_log_processor_info, the sal_log_mod_error_info_t entries are
> fixed size but have a variable number of entries.  Change the
> definition from 16 (max) to 0 entries and add a function to calculate
> the address of the processor static info within sal_log_processor_info,
> based on what sal actually supplied.  Use that function in
> ia64_init_handler() to get the min_state data.
> 
> Against 2.4.20-ia64-021210.

I applied the patch from 2.5 to 2.4 as well.