Re: [Qemu-devel] Re: [PATCH] Correct SMBIOS handling of multiple tables

[Beth Kon <eak@us.ibm.com>]

Alex Williamson wrote:
> On Wed, 2009-05-27 at 17:42 -0400, Beth Kon wrote:
>   
>> The current code prevents multiple entries of the same type table, as required,
>> for example, by table type 4.
>>     
>
> Hi Beth,
>
> Are you trying to add multiple type 4 entries from a single type 4
> binary passed in via -smbios file=<type4>?  ex:
>
> qemu -smp 2 -smbios file=type4.bin
>
> My intention was that once the user overrides an entry with a binary
> image, they're responsible for providing all of the entries of that
> type.  That means for a 2-way guest, if you want to override the type 4
> entry with a binary, you need to specify it twice on the command line.
> The code below would allow you to load a type 4 entry for each vCPU
> while only specifying one on the command line, but what if you want 2
> slightly different entries?  We have no way of associating a provided
> entry to the one needed at table creating time, which is why we load
> them all on the first pass and ignore requests to add the same type
> again later.  Thanks,
>
> Alex
>   
Hi Alex.

No, I wasn't trying to allow only one table to be specified on the 
command line for multiple same-type tables. The user is still 
responsible for providing all the entries. 

Without this patch, the current code has a problem even in the case of 
default tables (nothing specified on command line). Following the code 
path for add_struct(4, p, cpu_num) (for example) add_struct is called 
once for each cpu. add_struct calls smbios_load_external, and the first 
thing that does is check used_bitmap. The first pass through for table 4 
works. But if the vm is smp, the second pass through doesn't work 
because used_bitmap reports that a table 4 entry was already created, so 
smbios_load_external returns a 1 and add_struct becomes a noop in 
effect. I think this patch does what you intended, to override default 
creation with external creation if specified on the command line, and to 
get only one complete set of tables for each type.

>
>   
>> diff --git a/bios/rombios32.c b/bios/rombios32.c
>> index f861f81..43aa065 100644
>> --- a/bios/rombios32.c
>> +++ b/bios/rombios32.c
>> @@ -2146,6 +2146,10 @@ struct smbios_table {
>>  #define SMBIOS_FIELD_ENTRY 0
>>  #define SMBIOS_TABLE_ENTRY 1
>>  
>> +#ifdef BX_QEMU
>> +    static uint64_t smbios_used_bitmap[4] = { 0 };
>> +#endif
>> +
>>  static size_t
>>  smbios_load_field(int type, size_t offset, void *addr)
>>  {
>> @@ -2496,14 +2500,9 @@ smbios_load_external(int type, char **p, unsigned *nr_structs,
>>                       unsigned *max_struct_size)
>>  {
>>  #ifdef BX_QEMU
>> -    static uint64_t used_bitmap[4] = { 0 };
>>      char *start = *p;
>>      int i;
>>  
>> -    /* Check if we've already reported these tables */
>> -    if (used_bitmap[(type >> 6) & 0x3] & (1ULL << (type & 0x3f)))
>> -        return 1;
>> -
>>      /* Don't introduce spurious end markers */
>>      if (type == 127)
>>          return 0;
>> @@ -2555,7 +2554,7 @@ smbios_load_external(int type, char **p, unsigned *nr_structs,
>>      }
>>  
>>      /* Mark that we've reported on this type */
>> -    used_bitmap[(type >> 6) & 0x3] |= (1ULL << (type & 0x3f));
>> +    smbios_used_bitmap[(type >> 6) & 0x3] |= (1ULL << (type & 0x3f));
>>  
>>      return (start != *p);
>>  #else /* !BX_QEMU */
>> @@ -2612,6 +2611,9 @@ void smbios_init(void)
>>      add_struct(32, p);
>>      /* Add any remaining provided entries before the end marker */
>>      for (i = 0; i < 256; i++)
>> +        /* Check if we've already reported these tables */
>> +        if (smbios_used_bitmap[(i >> 6) & 0x3] & (1ULL << (i & 0x3f)))
>> +            continue;
>>          smbios_load_external(i, &p, &nr_structs, &max_struct_size);
>>      add_struct(127, p);
>>  
>>
>>     
>
>
>
>
>