[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 <marc.jones at amd.com>

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