Re: [coreboot] [pathc][v2] Fam10 name string

[Peter Stuge]

On Wed, Apr 23, 2008 at 06:02:19PM -0600, Marc Jones wrote:
> Add CPUID processor name string support for Fam10 CPUs.
>
> Signed-off-by: Marc Jones

Sorry.
> +static inline void strcpy(char *dst, char *src)
> +{
> while (*src) *dst++ = *src++;
>
> +}

This isn't somewhere already?
> +static const struct {
> u8 Pg;
> u8 NC;
> u8 String1;
> char const *value;
>
> +} String1_socket_F[] = {

I'd use the same struct for all tables..
> if (Model > 0) {
> /* String1 */
> if (PkgTyp == 0) {		/* F1207 */
> 	for(i = 0; i < sizeof(String1_socket_F)/sizeof(String1_socket_F[0]); i++) {
> 		if ((String1_socket_F[i].Pg == Pg) &&
> 		    (String1_socket_F[i].NC == NC) &&
> 		    (String1_socket_F[i].String1 == String1)) {
> 			processor_name_string = String1_socket_F[i].value;
> 		}
> 	}
> } else if (PkgTyp == 1) {	/* AM2 */
> 	for(i = 0; i < sizeof(String1_socket_AM2)/sizeof(String1_socket_AM2[0]); i++) {
> 		if ((String1_socket_AM2[i].Pg == Pg) &&
> 		    (String1_socket_AM2[i].NC == NC) &&
> 		    (String1_socket_AM2[i].String1 == String1)) {
> 			processor_name_string = String1_socket_AM2[i].value;
> 		}
> 	}
> }

..and set a pointer to the appropriate struct, and have a single loop..
> if (processor_name_string) {
> 	strcpy(program_string, processor_name_string);
> 	/* Translate Model from 01-99 to ASCII and put it on the end.
> 	 * Numbers less than 10 should include a leading zero, e.g., 09.*/
> 	for (j = 0; j < 47; j++) {
> 		if(program_string[j] == 0) {
> 			program_string[j++] = (Model / 10) + '0';
> 			program_string[j++] = (Model % 10) + '0';

This code can produce strings that make the BIOS fail validation,
there will be one byte of buffer overrun for 47 byte long strings,
and it does not add a terminating 0 byte after the two digits, which
means also 46 byte long strings cause failure.

At least change the for loop condition to j < 45.
> 	/* String 2 */
> 	processor_name_string = NULL;
> 	if (PkgTyp == 0) {		/* F1207 */
> 		for(i = 0; i < sizeof(String2_socket_F)/sizeof(String2_socket_F[0]); i++) {
> 			if ((String2_socket_F[i].Pg == Pg) &&
> 			    (String2_socket_F[i].String2 == String2)) {
> 				processor_name_string = String2_socket_F[i].value;
> 			}
> 		}
> 	} else if (PkgTyp == 1) {	/* AM2 */
> 		for(i = 0; i < sizeof(String2_socket_AM2)/sizeof(String2_socket_AM2[0]); i++) {
> 			if ((String2_socket_AM2[i].Pg == Pg) &&
> 			    (String2_socket_AM2[i].NC == NC) &&
> 			    (String2_socket_AM2[i].String2 == String2)) {
> 				processor_name_string = String2_socket_AM2[i].value;
> 			}
> 		}
> 	}

again nicer to just pick a pointer and have a single loop..
> 	if (processor_name_string) {

that way no if is needed in either place..
> 		/* FIXME: check length but it should never happen  */
> 		strcpy(&program_string[j], processor_name_string);

..or simply always memcpy 48 bytes and clear the last byte.

//Peter