Re: [patch] 2.4.21-ia64-030702 arch/ia64/kernel/unwind.c

Re: [patch] 2.4.21-ia64-030702 arch/ia64/kernel/unwind.c

[Keith Owens]

On Sun, 20 Jul 2003 19:51:06 +1000, 
Keith Owens <kaos@sgi.com> wrote:
>This has been fixed once but must have slipped through the cracks.  It
>only shows up when you turn on unwind debugging.

And another one, again it only shows up with unwind debugging.

--- linux/arch/ia64/kernel/unwind.c	2003-07-03 12:03:40.000000000 +1000
+++ linux/arch/ia64/kernel/unwind.c	2003-07-24 16:45:34.000000000 +1000
@@ -1955,7 +1955,7 @@
 		   "  bsp    0x%lx\n"
 		   "  sof    0x%lx\n"
 		   "  ip     0x%lx\n",
-		   info->bsp, sof, info->ip);
+		   __FUNCTION__, info->bsp, sof, info->ip);
 	find_save_locs(info);
 }
 
@@ -1973,7 +1973,7 @@
 		   "  bsp    0x%lx\n"
 		   "  sol    0x%lx\n"
 		   "  ip     0x%lx\n",
-		   info->bsp, sol, info->ip);
+		   __FUNCTION__, info->bsp, sol, info->ip);
 	find_save_locs(info);
 }
 

Re: [patch] 2.4.21-ia64-030702 arch/ia64/kernel/unwind.c

[Bjorn Helgaas]

On Sunday 20 July 2003 3:51 am, Keith Owens wrote:
> This has been fixed once but must have slipped through the cracks.  It
> only shows up when you turn on unwind debugging.
> 
> 
> --- 2.4.21/arch/ia64/kernel/unwind.c	2003-07-20 19:48:22.000000000 +1000
> +++ 2.4.21/arch/ia64/kernel/unwind.c	2003-07-20 19:48:13.000000000 +1000
> @@ -1934,7 +1934,7 @@
>  		   "  pr     0x%lx\n"
>  		   "  sw     0x%lx\n"
>  		   "  sp     0x%lx\n",
> -		   __FUNCTION__, (unsigned long) task, rbslimit, rbstop, stktop, stklimit,
> +		   __FUNCTION__, (unsigned long) t, rbslimit, rbstop, stktop, stklimit,
>  		   info->pr, (unsigned long) info->sw, info->sp);
>  	STAT(unw.stat.api.init_time += ia64_get_itc() - start; local_irq_restore(flags));
>  }

Applied for 2.4.  Sorry I missed this.

Bjorn

Re: [patch] 2.4.21-ia64-030702 arch/ia64/kernel/unwind.c

[Bjorn Helgaas]

On Thursday 24 July 2003 2:59 am, Keith Owens wrote:
> On Sun, 20 Jul 2003 19:51:06 +1000, 
> Keith Owens <kaos@sgi.com> wrote:
> >This has been fixed once but must have slipped through the cracks.  It
> >only shows up when you turn on unwind debugging.
> 
> And another one, again it only shows up with unwind debugging.
> 
> --- linux/arch/ia64/kernel/unwind.c	2003-07-03 12:03:40.000000000 +1000
> +++ linux/arch/ia64/kernel/unwind.c	2003-07-24 16:45:34.000000000 +1000
> @@ -1955,7 +1955,7 @@
>  		   "  bsp    0x%lx\n"
>  		   "  sof    0x%lx\n"
>  		   "  ip     0x%lx\n",
> -		   info->bsp, sof, info->ip);
> +		   __FUNCTION__, info->bsp, sof, info->ip);
>  	find_save_locs(info);
>  }
>  
> @@ -1973,7 +1973,7 @@
>  		   "  bsp    0x%lx\n"
>  		   "  sol    0x%lx\n"
>  		   "  ip     0x%lx\n",
> -		   info->bsp, sol, info->ip);
> +		   __FUNCTION__, info->bsp, sol, info->ip);
>  	find_save_locs(info);
>  }

I applied this one, too.  And I'll push the changes to bkbits and kernel.org
in a minute.

Bjorn

Re: [patch] 2.4.21-ia64-030702 arch/ia64/kernel/mca.c

[Bjorn Helgaas]

On Saturday 19 July 2003 12:25 am, Keith Owens wrote:
> Patches to arch/ia64/kernel/mca.c against 2.4.21-ia64-030702.
>
> Generic:
>   Issue a message to identify INIT/MCA records printed on the next boot.

ia64_log_print() already prints a note on the next boot if the log
contains saved records.  That note could be expanded, but the
logic around the new return values seems too complicated to me.

>   gp passed to SAL_VECTOR_OS_MCA should be a physical address.

I applied the bits to convert the GP passed to SAL_VECTOR_OS_MCA
to physical.  Actually, I copied the use of ia64_tpa() from 2.5, which
I believe should be safe since that's only done when mappings should
be valid.

>   Enable console logging for INIT/MCA messages.

I guess this refers to the SGI-specific addition to ia64_log_print().
I'm hoping to decrease, not increase, the error record decoding
code in the kernel.  Particularly for INIT, this is code that is
(1) platform-specific, and (2) only used after a non-recoverable
error.  It seems better to move such code to an off-line environment.

>   OEM data can be variable sized, do not use sizeof().

I don't quite understand this one.  Don't these:

> -				      (int)sizeof(sal_log_plat_specific_err_info_t) - 1,
> +				      ((char*)psei->oem_data - (char*)psei),

compute the same value?  I see that OEM data can be of variable
size (and we use "u8 oem_data[1]" as a placeholder), but isn't this
expression just computing the size of the fixed part of it?

>   Convert #ifdef CONFIG_SGI_SN2 to ia64_platform_is("sn2") to allow the
>   building of generic kernels.

I don't see any conversion -- just the addition of several ia64_platform_is()
checks, which I object to, especially when they change the behavior of
things that are not obviously platform-dependent.  For example, I think
we should come up with a strategy for breaking SAL locks and clearing INIT
logs that everybody can agree on.  Error handling is confusing enough
without making it platform-dependent.

Bjorn

Re: [patch] 2.4.21-ia64-030702 arch/ia64/kernel/mca.c

[Keith Owens]

On Thu, 7 Aug 2003 17:03:22 -0600, 
Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
>On Saturday 19 July 2003 12:25 am, Keith Owens wrote:
>> Patches to arch/ia64/kernel/mca.c against 2.4.21-ia64-030702.
>>
>> Generic:
>>   Issue a message to identify INIT/MCA records printed on the next boot.
>
>ia64_log_print() already prints a note on the next boot if the log
>contains saved records.  That note could be expanded, but the
>logic around the new return values seems too complicated to me.

ia64_mca_log_sal_error_record() returns 0 for no records and also for
success, making it impossible for the caller to determine if there were
any records.  ia64_mca_check_errors() only prints the message "The
above MCA error records were posted by SAL for a prior failure" if
there is at least one record at boot time, i.e. it needs the return
code from ia64_mca_log_sal_error_record() to distinguish between no
records and success.

>>   Enable console logging for INIT/MCA messages.
>
>I guess this refers to the SGI-specific addition to ia64_log_print().
>I'm hoping to decrease, not increase, the error record decoding
>code in the kernel.  Particularly for INIT, this is code that is
>(1) platform-specific, and (2) only used after a non-recoverable
>error.  It seems better to move such code to an off-line environment.

OK in the long run.  But until salinfo takes over as the official
method of handling log records, the mca.c code is still used.  These
small changes make the mca.c code more reliable.

>>   OEM data can be variable sized, do not use sizeof().
>
>I don't quite understand this one.  Don't these:
>
>> -				      (int)sizeof(sal_log_plat_specific_err_info_t) - 1,
>> +				      ((char*)psei->oem_data - (char*)psei),
>
>compute the same value?  I see that OEM data can be of variable
>size (and we use "u8 oem_data[1]" as a placeholder), but isn't this
>expression just computing the size of the fixed part of it?

platform_plat_specific_err_print() maps to ia64_log_prt_oem_data()
which expects a header length and a section length.
ia64_log_prt_oem_data() calculates the data length as the difference of
the first two parameters.

>>   Convert #ifdef CONFIG_SGI_SN2 to ia64_platform_is("sn2") to allow the
>>   building of generic kernels.
>
>I don't see any conversion -- just the addition of several ia64_platform_is()
>checks

The SGI ia64 tree had CONFIG_SGI_SN2 around some code.  That has to be
converted to a run time test to allow generic kernels to boot on SN2.
I incorrectly thought that the CONFIG_SGI_SN2 were in the base tree.

>which I object to, especially when they change the behavior of
>things that are not obviously platform-dependent.  For example, I think
>we should come up with a strategy for breaking SAL locks and clearing INIT
>logs that everybody can agree on.  Error handling is confusing enough
>without making it platform-dependent.

I agree, but so far only SN has any requirements for special processing
in mca.c.  Without a second platform, it is not clear what the coding
model should be.  I suggest you put in the sn2 changes and we review
them when another platform has similar requirements.

Re: [patch] 2.4.21-ia64-030702 arch/ia64/kernel/mca.c

[Bjorn Helgaas]

On Thursday 07 August 2003 6:41 pm, Keith Owens wrote:
> On Thu, 7 Aug 2003 17:03:22 -0600, 
> Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> >On Saturday 19 July 2003 12:25 am, Keith Owens wrote:
> >>   OEM data can be variable sized, do not use sizeof().
> >
> >I don't quite understand this one.  Don't these:
> >
> >> -				      (int)sizeof(sal_log_plat_specific_err_info_t) - 1,
> >> +				      ((char*)psei->oem_data - (char*)psei),
> >
> >compute the same value?  I see that OEM data can be of variable
> >size (and we use "u8 oem_data[1]" as a placeholder), but isn't this
> >expression just computing the size of the fixed part of it?
> 
> platform_plat_specific_err_print() maps to ia64_log_prt_oem_data()
> which expects a header length and a section length.
> ia64_log_prt_oem_data() calculates the data length as the difference of
> the first two parameters.

Yes, I read the code and I saw that, but that doesn't address my
question.  The "section length" (which we're computing here) is
really a compile-time constant based on the structure layout.
The "header length" is the variable one and the difference is
the size of OEM data.

I now see the real reason this change is needed, though: because
of structure padding, we can't assume that the variable-length
OEM data begins at sizeof(fixed_part)-1

I'll apply this change now for 2.4 (I hope somebody is tracking
these for 2.5).

Bjorn

Re: [patch] 2.4.21-ia64-030702 arch/ia64/kernel/mca.c

[Bjorn Helgaas]

On Thursday 07 August 2003 6:41 pm, Keith Owens wrote:
> On Thu, 7 Aug 2003 17:03:22 -0600, 
> Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> >I don't see any conversion -- just the addition of several ia64_platform_is()
> >checks which I object to, especially when they change the behavior of
> >things that are not obviously platform-dependent.  For example, I think
> >we should come up with a strategy for breaking SAL locks and clearing INIT
> >logs that everybody can agree on.  Error handling is confusing enough
> >without making it platform-dependent.
> 
> I agree, but so far only SN has any requirements for special processing
> in mca.c.  Without a second platform, it is not clear what the coding
> model should be.  I suggest you put in the sn2 changes and we review
> them when another platform has similar requirements.

I don't understand the changes well enough to be convinced that
they're SGI-specific.  For instance:

> +#ifdef CONFIG_SMP
> +	if (ia64_platform_is("sn2") && sal_lock.lock) {
> +		udelay(500000);
> +		sal_lock.lock = 0;
> +	}
> +#endif

ia64_sal_get_state_info() uses SAL_CALL(), which acquires sal_lock.
So it seems that every platform would deadlock if we MCA in the
middle of a previous SAL call.  Or is there something peculiar about
SGI firmware?

> +	if (ia64_platform_is("sn2")) {
> +		/* SN saves logs until next boot */
> +		;
> +	} else {
> +		/* Clear the INIT SAL logs now that they have been saved in the OS buffer */
> +		ia64_sal_clear_state_info(SAL_INFO_TYPE_INIT);
> +	}

You added back ia64_sal_clear_state_info() for all non-SGI platforms,
which I don't think we want.

> @@ -2076,6 +2141,11 @@ ia64_log_proc_dev_err_info_print (sal_lo
> +		if (ia64_platform_is("sn2")) {
> +			platform_plat_specific_err_print((int)slpi->header.len,
> +			      0, (void*)slpi, prfunc);
> +			return;
> +		}

> @@ -2301,7 +2371,13 @@ ia64_log_print(int sal_info_type, prfunc
> -		prfunc("+MCA INIT ERROR LOG (UNIMPLEMENTED)\n");
> +		if (ia64_platform_is("sn2")) {
> +			prfunc("+BEGIN HARDWARE ERROR STATE AT INIT\n");
> +			platform_err = ia64_log_platform_info_print(IA64_LOG_CURR_BUFFER(sal_info_type), prfunc);
> +			prfunc("+END HARDWARE ERROR STATE AT INIT\n");
> +		} else {
> +			prfunc("+MCA INIT ERROR LOG (UNIMPLEMENTED)\n");
> +		}

Is there any reason not to do these on all platforms?  The
"platform_plat_specific_err_print" looks a lot like a platform vector
anyway, so I don't see the point of another test.

Bjorn

Re: [patch] 2.4.21-ia64-030702 arch/ia64/kernel/mca.c

[Keith Owens]

On Fri, 8 Aug 2003 11:15:30 -0600,
Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
>On Thursday 07 August 2003 6:41 pm, Keith Owens wrote:
>I don't understand the changes well enough to be convinced that
>they're SGI-specific.  For instance:
>
>> +#ifdef CONFIG_SMP
>> +    if (ia64_platform_is("sn2") && sal_lock.lock) {
>> +            udelay(500000);
>> +            sal_lock.lock = 0;
>> +    }
>> +#endif
>
>ia64_sal_get_state_info() uses SAL_CALL(), which acquires sal_lock.
>So it seems that every platform would deadlock if we MCA in the
>middle of a previous SAL call.  Or is there something peculiar
>about SGI firmware?

SAL spec 9.1.

  * Some SAL procedures are primarily intended for use during OS
    initialization and designed to be called on one processor. These are
    not required to be re-entrant. Some SAL procedures are required to be
    called on multiple processors simultaneously. These are required to
    be MP-safe but need not be re-entrant. Some SAL procedures may be
    re-invoked on the same processor, e.g. the invocation of the
    SAL_GET_STATE_INFO procedure for a CPE event may be interrupted by
    the invocation of the same procedure for an MCA event on the same
    processor.  Such procedures need to be re-entrant as well as MP-safe.
    These requirements are specified in Table 9.3. For the procedures
    that are not re-entrant, the operating system is required to enforce
    single threaded access.


Table 9-2. SAL Procedures

                         Function ID                                          Re-entrancy
  Procedure                                    Description
                           (hex)                                              Requirement
  SAL_SET_VECTORS          0x01000000   Register software code locations      None
                                        with SAL.
  SAL_GET_STATE_INFO       0x01000001   Return Machine State information      Yes
                                        obtained by SAL.
  SAL_GET_STATE_INFO_SIZE  0x01000002   Obtain size of Machine State          Yes
                                        information.
  SAL_CLEAR_STATE_INFO     0x01000003   Clear Machine State information.      Yes
  SAL_MC_RENDEZ            0x01000004   Cause the processor to go into a      MP-safe
                                        spin loop within SAL.
  SAL_MC_SET_PARAMS        0x01000005   Register the machine check interface  None
                                        layer with SAL.
  SAL_REGISTER_PHYSICAL_   0x01000006   Register the physical addresses of    None
  ADDR                                  locations needed by SAL.
  SAL_CACHE_FLUSH          0x01000008   Flush the instruction or data caches. MP-safe
  SAL_CACHE_INIT           0x01000009   Initialize the instruction and data   MP-safe
                                        caches.
  SAL_PCI_CONFIG_READ      0x01000010   Read from the PCI configuration       Yes
                                        space.
  SAL_PCI_CONFIG_WRITE     0x01000011   Write to the PCI configuration space. Yes
  SAL_FREQ_BASE            0x01000012   Return the base frequency of the      MP-safe
                                        platform.
  SAL_UPDATE_PAL           0x01000020   Update the contents of firmware       None
  blocks.


The current kernel code does not conform to those specifications.
SAL_GET_STATE_INFO_SIZE is supposed to be reentrant safe so it does not
deadlock, but the Linux code uses SAL_CALL() with a spinlock that will
hang for SAL_GET_STATE_INFO_SIZE during an existing SAL event.  I have
hit this case.

The real fix is to change the non-conforming functions in sal.h to use
SAL_CALL_NOLOCK.  But is it safe to do so, can all the platforms handle
reentrant SAL calls?

I know that SGI SAL code is reentrant safe so I break any existing SAL
lock, but only on SN2.  If the other platforms say that they are
reentrant safe and the locking in sal.h is fixed then this sn2 specific
patch is not required.  Until then, we need it in the kernel.


>> +    if (ia64_platform_is("sn2")) {
>> +            /* SN saves logs until next boot */
>> +            ;
>> +    } else {
>> +            /* Clear the INIT SAL logs now that they have been saved in the OS buffer */
>> +            ia64_sal_clear_state_info(SAL_INFO_TYPE_INIT);
>> +    }
>
>You added back ia64_sal_clear_state_info() for all non-SGI platforms,
>which I don't think we want.

Merge hangover from 2.4.20.  Delete this patch hunk.

>> @@ -2076,6 +2141,11 @@ ia64_log_proc_dev_err_info_print (sal_lo
>> +            if (ia64_platform_is("sn2")) {
>> +                    platform_plat_specific_err_print((int)slpi->header.len,
>> +                          0, (void*)slpi, prfunc);
>> +                    return;
>> +            }
>
>> @@ -2301,7 +2371,13 @@ ia64_log_print(int sal_info_type, prfunc
>> -            prfunc("+MCA INIT ERROR LOG (UNIMPLEMENTED)\n");
>> +            if (ia64_platform_is("sn2")) {
>> +                    prfunc("+BEGIN HARDWARE ERROR STATE AT INIT\n");
>> +                    platform_err = ia64_log_platform_info_print(IA64_LOG_CURR_BUFFER(sal_info_type), prfunc);
>> +                    prfunc("+END HARDWARE ERROR STATE AT INIT\n");
>> +            } else {
>> +                    prfunc("+MCA INIT ERROR LOG (UNIMPLEMENTED)\n");
>> +            }
>
>Is there any reason not to do these on all platforms?  The
>"platform_plat_specific_err_print" looks a lot like a platform vector
>anyway, so I don't see the point of another test.

I inherited these so am not sure why they were SN2 specific.  There is
no obvious reason why they should not be done for all platforms.

Re: [patch] 2.4.21-ia64-030702 arch/ia64/kernel/mca.c

[Alex Williamson]

> 
> The current kernel code does not conform to those specifications.
> SAL_GET_STATE_INFO_SIZE is supposed to be reentrant safe so it does not
> deadlock, but the Linux code uses SAL_CALL() with a spinlock that will
> hang for SAL_GET_STATE_INFO_SIZE during an existing SAL event.  I have
> hit this case.
> 
> The real fix is to change the non-conforming functions in sal.h to use
> SAL_CALL_NOLOCK.  But is it safe to do so, can all the platforms handle
> reentrant SAL calls?
> 
> I know that SGI SAL code is reentrant safe so I break any existing SAL
> lock, but only on SN2.  If the other platforms say that they are
> reentrant safe and the locking in sal.h is fixed then this sn2 specific
> patch is not required.  Until then, we need it in the kernel.

   See the modification I made to these for cmc/cpe polling.  I created
a new SAL call that runs with irqs enabled.  SAL_{GET,CLEAR}_STATE_INFO*
make use of them.  Nobody has spoken up yet about any platforms that
don't follow the SAL spec for these calls.  Bjorn accepted the changes,
but it doesn't look like they're in bk yet.  They're in 2.6.0-test3.
Does that eliminate this problem?

	Alex

Re: [patch] 2.4.21-ia64-030702 arch/ia64/kernel/mca.c

[Bjorn Helgaas]

On Saturday 09 August 2003 11:28 pm, Alex Williamson wrote:
> > I know that SGI SAL code is reentrant safe so I break any existing SAL
> > lock, but only on SN2.  If the other platforms say that they are
> > reentrant safe and the locking in sal.h is fixed then this sn2 specific
> > patch is not required.  Until then, we need it in the kernel.
> 
>    See the modification I made to these for cmc/cpe polling.  I created
> a new SAL call that runs with irqs enabled.  SAL_{GET,CLEAR}_STATE_INFO*
> make use of them.  Nobody has spoken up yet about any platforms that
> don't follow the SAL spec for these calls.  Bjorn accepted the changes,
> but it doesn't look like they're in bk yet.  They're in 2.6.0-test3.
> Does that eliminate this problem?

I did put in the change so SAL_{GET,CLEAR}_STATE_INFO are called
with no locking and with interrupts enabled.  Now that I think about
it, I'll probably regret it because of the risk you pointed out, Keith ;-)

I updated the BK tree and plain-text patches, and I think it would
be easier if you re-diffed (and possibly split up into logical units)
any remaining patches.

Bjorn

[patch] 2.4.21-ia64-030702 arch/ia64/kernel/unwind.c

[Keith Owens]

This has been fixed once but must have slipped through the cracks.  It
only shows up when you turn on unwind debugging.


--- 2.4.21/arch/ia64/kernel/unwind.c	2003-07-20 19:48:22.000000000 +1000
+++ 2.4.21/arch/ia64/kernel/unwind.c	2003-07-20 19:48:13.000000000 +1000
@@ -1934,7 +1934,7 @@
 		   "  pr     0x%lx\n"
 		   "  sw     0x%lx\n"
 		   "  sp     0x%lx\n",
-		   __FUNCTION__, (unsigned long) task, rbslimit, rbstop, stktop, stklimit,
+		   __FUNCTION__, (unsigned long) t, rbslimit, rbstop, stktop, stklimit,
 		   info->pr, (unsigned long) info->sw, info->sp);
 	STAT(unw.stat.api.init_time += ia64_get_itc() - start; local_irq_restore(flags));
 }