[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Re: [PATCH] Correct SMBIOS handling of multiple tables
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] Re: [PATCH] Correct SMBIOS handling of multiple tables |
Date: |
Thu, 28 May 2009 09:51:18 -0600 |
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