bios: resolve memory device roll over reporting issues with >32G guests

PATCH Applied Bill Rieske 2 participants 4 messages Nov 7, 2008 – Nov 16, 2008 KVM

lore

Share & tools

lore

Bill Rieske

author

2008-11-07 22:15 UTC #0 lore raw

PATCH 1/1 bios: resolve memory device roll over reporting issues with >32G guests

This presents more than one memory device and associated memory structures if the memory is larger than 16G

Signed-off-by Bill Rieske <brieske@@novell.com>

bios/rombios32.c +68 ~~-25~~

index a91b155..bc69945 100755
@@ -173,6 +173,26 @@ static inline int isdigit(int c)
     return c >= '0' && c <= '9';
 }
 
+char *itoa(char *a, unsigned int i)
+{
+    unsigned int _i = i, x = 0;
+
+    do {
+        x++;
+        _i /= 10;
+    } while ( _i != 0 );
+
+    a += x;
+    *a-- = '\0';
+
+    do {
+        *a-- = (i % 10) + '0';
+        i /= 10;
+    } while ( i != 0 );
+
+    return a + 1;
+}
+
 void *memset(void *d1, int val, size_t len)
 {
     uint8_t *d = d1;

@@ -220,6 +240,16 @@ size_t strlen(const char *s)
     return s1 - s;
 }
 
+char *
+strcpy(char *dest, const char *src)
+{
+    char *p = dest;
+    while ( *src )
+        *p++ = *src++;
+    *p = 0;
+    return dest;
+}
+
 /* from BSD ppp sources */
 int vsnprintf(char *buf, int buflen, const char *fmt, va_list args)
 {

@@ -1914,7 +1944,7 @@ smbios_type_4_init(void *start, unsigned int cpu_number)
 
 /* Type 16 -- Physical Memory Array */
 static void *
-smbios_type_16_init(void *start, uint32_t memsize)
+smbios_type_16_init(void *start, uint32_t memsize, int nr_mem_devs)
 {
     struct smbios_type_16 *p = (struct smbios_type_16*)start;

@@ -1927,7 +1957,7 @@ smbios_type_16_init(void *start, uint32_t memsize)
     p->error_correction = 0x01; /* other */
     p->maximum_capacity = memsize * 1024;
     p->memory_error_information_handle = 0xfffe; /* none provided */
-    p->number_of_memory_devices = 1;
+    p->number_of_memory_devices = nr_mem_devs;
 
     start += sizeof(struct smbios_type_16);
     *((uint16_t *)start) = 0;

@@ -1937,20 +1967,20 @@ smbios_type_16_init(void *start, uint32_t memsize)
 
 /* Type 17 -- Memory Device */
 static void *
-smbios_type_17_init(void *start, uint32_t memory_size_mb)
+smbios_type_17_init(void *start, uint32_t memory_size_mb, int instance)
 {
+    char buf[16];
     struct smbios_type_17 *p = (struct smbios_type_17 *)start;
 
     p->header.type = 17;
     p->header.length = sizeof(struct smbios_type_17);
-    p->header.handle = 0x1100;
+    p->header.handle = 0x1100 + instance;
 
     p->physical_memory_array_handle = 0x1000;
     p->total_width = 64;
     p->data_width = 64;
-    /* truncate memory_size_mb to 16 bits and clear most significant
-       bit [indicates size in MB] */
-    p->size = (uint16_t) memory_size_mb & 0x7fff;
+/* TODO: should assert in case something is wrong   ASSERT((memory_size_mb & ~0x7fff) == 0); */ 
+    p->size = memory_size_mb;
     p->form_factor = 0x09; /* DIMM */
     p->device_set = 0;
     p->device_locator_str = 1;

@@ -1959,8 +1989,11 @@ smbios_type_17_init(void *start, uint32_t memory_size_mb)
     p->type_detail = 0;
 
     start += sizeof(struct smbios_type_17);
-    memcpy((char *)start, "DIMM 1", 7);
-    start += 7;
+    memcpy((char *)start, "DIMM ", 6);
+    start += strlen("DIMM ");
+    itoa(buf, instance);
+    strcpy(start, buf);
+    start += strlen(buf) + 1;
     *((uint8_t *)start) = 0;
 
     return start+1;

@@ -1968,16 +2001,16 @@ smbios_type_17_init(void *start, uint32_t memory_size_mb)
 
 /* Type 19 -- Memory Array Mapped Address */
 static void *
-smbios_type_19_init(void *start, uint32_t memory_size_mb)
+smbios_type_19_init(void *start, uint32_t memory_size_mb, int instance)
 {
     struct smbios_type_19 *p = (struct smbios_type_19 *)start;
 
     p->header.type = 19;
     p->header.length = sizeof(struct smbios_type_19);
-    p->header.handle = 0x1300;
+    p->header.handle = 0x1300 + instance;
 
-    p->starting_address = 0;
-    p->ending_address = (memory_size_mb * 1024) - 1;
+    p->starting_address = instance << 24;
+    p->ending_address = p->starting_address + (memory_size_mb << 10) - 1;
     p->memory_array_handle = 0x1000;
     p->partition_width = 1;

@@ -1989,18 +2022,18 @@ smbios_type_19_init(void *start, uint32_t memory_size_mb)
 
 /* Type 20 -- Memory Device Mapped Address */
 static void *
-smbios_type_20_init(void *start, uint32_t memory_size_mb)
+smbios_type_20_init(void *start, uint32_t memory_size_mb, int instance)
 {
     struct smbios_type_20 *p = (struct smbios_type_20 *)start;
 
     p->header.type = 20;
     p->header.length = sizeof(struct smbios_type_20);
-    p->header.handle = 0x1400;
+    p->header.handle = 0x1400 + instance;
 
-    p->starting_address = 0;
-    p->ending_address = (memory_size_mb * 1024) - 1;
-    p->memory_device_handle = 0x1100;
-    p->memory_array_mapped_address_handle = 0x1300;
+    p->starting_address = instance << 24;
+    p->ending_address = p->starting_address + (memory_size_mb << 10) - 1;
+    p->memory_device_handle = 0x1100 + instance;
+    p->memory_array_mapped_address_handle = 0x1300 + instance;
     p->partition_row_position = 1;
     p->interleave_position = 0;
     p->interleaved_data_depth = 0;

@@ -2051,6 +2084,7 @@ void smbios_init(void)
     char *start, *p, *q;
     int memsize = (ram_end == ram_size) ? ram_size / (1024 * 1024) :
                   (ram_end - (1ull << 32) + ram_size) / (1024 * 1024);
+    int i, nr_mem_devs;
 
 #ifdef BX_USE_EBDA_TABLES
     ebda_cur_addr = align(ebda_cur_addr, 16);

@@ -2062,23 +2096,32 @@ void smbios_init(void)
 
 	p = (char *)start + sizeof(struct smbios_entry_point);
 
-#define add_struct(fn) { \
+#define add_struct(fn) do{ \
     q = (fn); \
     nr_structs++; \
     if ((q - p) > max_struct_size) \
         max_struct_size = q - p; \
     p = q; \
-}
+}while (0)
 
     add_struct(smbios_type_0_init(p));
     add_struct(smbios_type_1_init(p));
     add_struct(smbios_type_3_init(p));
     for (cpu_num = 1; cpu_num <= smp_cpus; cpu_num++)
         add_struct(smbios_type_4_init(p, cpu_num));
-    add_struct(smbios_type_16_init(p, memsize));
-    add_struct(smbios_type_17_init(p, memsize));
-    add_struct(smbios_type_19_init(p, ram_end / (1024 * 1024)));
-    add_struct(smbios_type_20_init(p, ram_end / (1024 * 1024)));
+
+    /* Each 'memory device' covers up to 16GB of address space. */
+    nr_mem_devs = (memsize + 0x3fff) >> 14;
+    add_struct(smbios_type_16_init(p, memsize, nr_mem_devs));
+    for ( i = 0; i < nr_mem_devs; i++ )
+    {
+        uint32_t dev_memsize = ((i == (nr_mem_devs - 1))
+                                ? (memsize & 0x3fff) : 0x4000);
+        add_struct(smbios_type_17_init(p, dev_memsize, i));
+        add_struct(smbios_type_19_init(p, dev_memsize, i));
+        add_struct(smbios_type_20_init(p, dev_memsize, i));
+    }
+
     add_struct(smbios_type_32_init(p));
     add_struct(smbios_type_127_init(p));

Avi Kivity

2008-11-09 13:15 UTC #1 lore raw

Quoted 26 lines · Bill Rieske

The field within the Memory Device type 17 is only a word with the MSB being used to report MB/KB. Thereby, a guest with 32G and greater would report incorrect memory device information rolling over to 0.
This presents more than one memory device and associated memory structures if the memory is larger than 16G
Signed-off-by Bill Rieske <brieske@@novell.com>
bios/rombios32.c +11 -0
index a91b155..bc69945 100755
@@ -173,6 +173,26 @@ static inline int isdigit(int c)
     return c >= '0' && c <= '9';
 }
 
+char *itoa(char *a, unsigned int i)
+{
+    unsigned int _i = i, x = 0;
+
+    do {
+        x++;
+        _i /= 10;
+    } while ( _i != 0 );
+
+    a += x;
+    *a-- = '\0';

bios/rombios32.c static inline int isdigit(int c) · line 196

Quoted 10 lines

bios/rombios32.c +9 -0

+
+    do {
+        *a-- = (i % 10) + '0';
+        i /= 10;
+    } while ( i != 0 );
+
+    return a + 1;
+}
+

Instead of this, can you add an snprintf() (in a separate patch) and use it? There's already a vsnprintf() so all the heavy machinery is in place.

Quoted 8 lines

bios/rombios32.c +0 -0

 void *memset(void *d1, int val, size_t len)
 {

     uint8_t *d = d1;

bios/rombios32.c +1 -0

@@ -220,6 +240,16 @@ size_t strlen(const char *s)
     return s1 - s;
 }
 
+char *

bios/rombios32.c size_t strlen(const char *s) · line 253

Quoted 10 lines

bios/rombios32.c +9 -0

+strcpy(char *dest, const char *src)
+{
+    char *p = dest;
+    while ( *src )
+        *p++ = *src++;
+    *p = 0;
+    return dest;
+}
+

This would eliminate the need for strcpy(), I think.

Signature · 1 line

error compiling committee.c: too many arguments to function

Bill Rieske

author

2008-11-10 16:31 UTC #2 lore raw

Quoted 2 lines

Instead of this, can you add an snprintf() (in a separate patch) and use it? There's already a vsnprintf() so all the heavy machinery is in place.

Nice catch much cleaner

The field within the Memory Device type 17 is only a word with the MSB being used to report MB/KB. Thereby, a guest with 32G and greater would report incorrect memory device information rolling over to 0.
This presents more than one memory device and associated memory structures if the memory is larger than 16G

Signed-off-by Bill Rieske <brieske@@novell.com>

bios/rombios32.c +45 ~~-25~~

index a91b155..5442e70 100755
@@ -370,6 +370,17 @@ int vsnprintf(char *buf, int buflen, const char *fmt, va_list args)
     return buf - buf0;
 }
 
+int snprintf(char * buf, size_t size, const char *fmt, ...)
+{
+	va_list args;
+	int i;
+
+	va_start(args, fmt);
+	i=vsnprintf(buf,size,fmt,args);
+	va_end(args);
+	return i;
+}
+
 void bios_printf(int flags, const char *fmt, ...)
 {
     va_list ap;

@@ -1914,7 +1925,7 @@ smbios_type_4_init(void *start, unsigned int cpu_number)
 
 /* Type 16 -- Physical Memory Array */
 static void *
-smbios_type_16_init(void *start, uint32_t memsize)
+smbios_type_16_init(void *start, uint32_t memsize, int nr_mem_devs)
 {
     struct smbios_type_16 *p = (struct smbios_type_16*)start;

@@ -1927,7 +1938,7 @@ smbios_type_16_init(void *start, uint32_t memsize)
     p->error_correction = 0x01; /* other */
     p->maximum_capacity = memsize * 1024;
     p->memory_error_information_handle = 0xfffe; /* none provided */
-    p->number_of_memory_devices = 1;
+    p->number_of_memory_devices = nr_mem_devs;
 
     start += sizeof(struct smbios_type_16);
     *((uint16_t *)start) = 0;

@@ -1937,20 +1948,19 @@ smbios_type_16_init(void *start, uint32_t memsize)
 
 /* Type 17 -- Memory Device */
 static void *
-smbios_type_17_init(void *start, uint32_t memory_size_mb)
+smbios_type_17_init(void *start, uint32_t memory_size_mb, int instance)
 {
     struct smbios_type_17 *p = (struct smbios_type_17 *)start;
 
     p->header.type = 17;
     p->header.length = sizeof(struct smbios_type_17);
-    p->header.handle = 0x1100;
+    p->header.handle = 0x1100 + instance;
 
     p->physical_memory_array_handle = 0x1000;
     p->total_width = 64;
     p->data_width = 64;
-    /* truncate memory_size_mb to 16 bits and clear most significant
-       bit [indicates size in MB] */
-    p->size = (uint16_t) memory_size_mb & 0x7fff;
+/* TODO: should assert in case something is wrong   ASSERT((memory_size_mb & ~0x7fff) == 0); */ 
+    p->size = memory_size_mb;
     p->form_factor = 0x09; /* DIMM */
     p->device_set = 0;
     p->device_locator_str = 1;

@@ -1959,8 +1969,8 @@ smbios_type_17_init(void *start, uint32_t memory_size_mb)
     p->type_detail = 0;
 
     start += sizeof(struct smbios_type_17);
-    memcpy((char *)start, "DIMM 1", 7);
-    start += 7;
+    snprintf(start, 8, "DIMM %d", instance); 
+    start += strlen(start) + 1;
     *((uint8_t *)start) = 0;
 
     return start+1;

@@ -1968,16 +1978,16 @@ smbios_type_17_init(void *start, uint32_t memory_size_mb)
 
 /* Type 19 -- Memory Array Mapped Address */
 static void *
-smbios_type_19_init(void *start, uint32_t memory_size_mb)
+smbios_type_19_init(void *start, uint32_t memory_size_mb, int instance)
 {
     struct smbios_type_19 *p = (struct smbios_type_19 *)start;
 
     p->header.type = 19;
     p->header.length = sizeof(struct smbios_type_19);
-    p->header.handle = 0x1300;
+    p->header.handle = 0x1300 + instance;
 
-    p->starting_address = 0;
-    p->ending_address = (memory_size_mb * 1024) - 1;
+    p->starting_address = instance << 24;
+    p->ending_address = p->starting_address + (memory_size_mb << 10) - 1;
     p->memory_array_handle = 0x1000;
     p->partition_width = 1;

@@ -1989,18 +1999,18 @@ smbios_type_19_init(void *start, uint32_t memory_size_mb)
 
 /* Type 20 -- Memory Device Mapped Address */
 static void *
-smbios_type_20_init(void *start, uint32_t memory_size_mb)
+smbios_type_20_init(void *start, uint32_t memory_size_mb, int instance)
 {
     struct smbios_type_20 *p = (struct smbios_type_20 *)start;
 
     p->header.type = 20;
     p->header.length = sizeof(struct smbios_type_20);
-    p->header.handle = 0x1400;
+    p->header.handle = 0x1400 + instance;
 
-    p->starting_address = 0;
-    p->ending_address = (memory_size_mb * 1024) - 1;
-    p->memory_device_handle = 0x1100;
-    p->memory_array_mapped_address_handle = 0x1300;
+    p->starting_address = instance << 24;
+    p->ending_address = p->starting_address + (memory_size_mb << 10) - 1;
+    p->memory_device_handle = 0x1100 + instance;
+    p->memory_array_mapped_address_handle = 0x1300 + instance;
     p->partition_row_position = 1;
     p->interleave_position = 0;
     p->interleaved_data_depth = 0;

@@ -2051,6 +2061,7 @@ void smbios_init(void)
     char *start, *p, *q;
     int memsize = (ram_end == ram_size) ? ram_size / (1024 * 1024) :
                   (ram_end - (1ull << 32) + ram_size) / (1024 * 1024);
+    int i, nr_mem_devs;
 
 #ifdef BX_USE_EBDA_TABLES
     ebda_cur_addr = align(ebda_cur_addr, 16);

@@ -2062,23 +2073,32 @@ void smbios_init(void)
 
 	p = (char *)start + sizeof(struct smbios_entry_point);
 
-#define add_struct(fn) { \
+#define add_struct(fn) do{ \
     q = (fn); \
     nr_structs++; \
     if ((q - p) > max_struct_size) \
         max_struct_size = q - p; \
     p = q; \
-}
+}while (0)
 
     add_struct(smbios_type_0_init(p));
     add_struct(smbios_type_1_init(p));
     add_struct(smbios_type_3_init(p));
     for (cpu_num = 1; cpu_num <= smp_cpus; cpu_num++)
         add_struct(smbios_type_4_init(p, cpu_num));
-    add_struct(smbios_type_16_init(p, memsize));
-    add_struct(smbios_type_17_init(p, memsize));
-    add_struct(smbios_type_19_init(p, ram_end / (1024 * 1024)));
-    add_struct(smbios_type_20_init(p, ram_end / (1024 * 1024)));
+
+    /* Each 'memory device' covers up to 16GB of address space. */
+    nr_mem_devs = (memsize + 0x3fff) >> 14;
+    add_struct(smbios_type_16_init(p, memsize, nr_mem_devs));
+    for ( i = 0; i < nr_mem_devs; i++ )
+    {
+        uint32_t dev_memsize = ((i == (nr_mem_devs - 1))
+                                ? (memsize & 0x3fff) : 0x4000);
+        add_struct(smbios_type_17_init(p, dev_memsize, i));
+        add_struct(smbios_type_19_init(p, dev_memsize, i));
+        add_struct(smbios_type_20_init(p, dev_memsize, i));
+    }
+
     add_struct(smbios_type_32_init(p));
     add_struct(smbios_type_127_init(p));

Avi Kivity

Applied

2008-11-16 16:10 UTC #3 lore raw

Quoted 8 lines · Bill Rieske

Quoted 2 lines
Instead of this, can you add an snprintf() (in a separate patch) and use it? There's already a vsnprintf() so all the heavy machinery is in place.
Nice catch much cleaner
The field within the Memory Device type 17 is only a word with the MSB being used to report MB/KB. Thereby, a guest with 32G and greater would report incorrect memory device information rolling over to 0. This presents more than one memory device and associated memory structures if the memory is larger than 16G

Applied, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function