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

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

Alex Williamson wrote:
> On Thu, 2009-05-28 at 11:05 -0400, Beth Kon wrote:
>   
>> This leads to the question of whether qemu should verify that 
>> command-line smbios tables make "sense" (e.g. number of type 4 tables == 
>> smp_cpus).  The SMBIOS spec has many cases where multiples of a table 
>> are required. Covering all of them would be very burdensome with little 
>> or no payback. But I think it is worth putting in a check for table 4, 
>> since it is relatively simple and is pretty likely to be specified on 
>> the command line.
>>     
>
> I think that makes sense, we can add other checks as needed.
>
>   
>> One other nit I fixed was detection of a bad filename.
>>
>> So if you agree, I'd like to submit your rombios32.c patch along with my 
>> table 4 check patch (combined here for simplicity).
>>     
>
> Patch comment below....
>
>   
>> diff --git a/hw/smbios.c b/hw/smbios.c
>> index ced90ce..a8b52c7 100644
>> --- a/hw/smbios.c
>> +++ b/hw/smbios.c
>> @@ -173,8 +173,8 @@ int smbios_entry_add(const char *t)
>>          struct smbios_table *table;
>>          int size = get_image_size(buf);
>>  
>> -        if (size < sizeof(struct smbios_structure_header)) {
>> -            fprintf(stderr, "Cannot read smbios file %s", buf);
>> +        if (size == -1 || size < sizeof(struct
>> smbios_structure_header)) {
>> +            fprintf(stderr, "Cannot read smbios file %s\n", buf);
>>              exit(1);
>>          }
>>  
>> @@ -196,6 +196,9 @@ int smbios_entry_add(const char *t)
>>  
>>          header = (struct smbios_structure_header *)(table->data);
>>          smbios_check_collision(header->type, SMBIOS_TABLE_ENTRY);
>> +        if (header->type == 4) {
>> +            smbios_type4_count++;
>> +        }
>>  
>>          smbios_entries_len += sizeof(*table) + size;
>>          (*(uint16_t *)smbios_entries) =
>> diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
>> index cbd5f15..f3e75f8 100755
>> --- a/kvm/bios/rombios32.c
>> +++ b/kvm/bios/rombios32.c
>> @@ -2531,13 +2531,14 @@ smbios_load_external(int type, char **p,
>> unsigned *nr_structs,
>>              *max_struct_size = *p - (char *)header;
>>      }
>>  
>> -    /* Mark that we've reported on this type */
>> -    used_bitmap[(type >> 6) & 0x3] |= (1ULL << (type & 0x3f));
>> +    if (start != *p) {
>> +        /* Mark that we've reported on this type */
>> +        used_bitmap[(type >> 6) & 0x3] |= (1ULL << (type & 0x3f));
>> +        return 1;
>> +    }
>>  
>> -    return (start != *p);
>> -#else /* !BX_QEMU */
>> +#endif /* !BX_QEMU */
>>      return 0;
>> -#endif
>>  }
>>  
>>  void smbios_init(void)
>> diff --git a/pc-bios/bios.bin b/pc-bios/bios.bin
>> index 70bd7ad..50d5365 100644
>> Binary files a/pc-bios/bios.bin and b/pc-bios/bios.bin differ
>> diff --git a/sysemu.h b/sysemu.h
>> index 47d001e..0b982ed 100644
>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -115,6 +115,7 @@ extern int rtc_td_hack;
>>  extern int alt_grab;
>>  extern int usb_enabled;
>>  extern int smp_cpus;
>> +extern int smbios_type4_count;
>>  extern int cursor_hide;
>>  extern int graphic_rotate;
>>  extern int no_quit;
>> diff --git a/vl.c b/vl.c
>> index db8265b..c9cc5b7 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -246,6 +246,7 @@ int singlestep = 0;
>>  const char *assigned_devices[MAX_DEV_ASSIGN_CMDLINE];
>>  int assigned_devices_index;
>>  int smp_cpus = 1;
>> +int smbios_type4_count = 0;
>>  const char *vnc_display;
>>  int acpi_enabled = 1;
>>  int no_hpet = 0;
>> @@ -5775,6 +5776,14 @@ int main(int argc, char **argv, char **envp)
>>          }
>>      }
>>  
>> +#ifdef TARGET_I386
>> +    if (smbios_type4_count && smbios_type4_count != smp_cpus) {
>> +         fprintf(stderr,
>> +                 "count of SMBIOS type 4 tables != SMP CPUs
>> specified.\n");
>> +         exit(1);
>> +    }
>> +#endif
>> +
>>     
>
> I'm not thrilled with adding globals and externs in unrelated parts of
> the code.  I think we can keep this all internal to smbios.c if we make
> a new smbios_validate_table() function that gets called from
> smbios_get_table().  Thanks,
>
> Alex
>
>
>   
Yep, I agree. I didn't like that either. Your solution is better. I'll 
change that and submit the userspace part and, as discussed, you can 
submit the rombios32.c change.