|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 04/07] HVM firmware passthrough: SMBIOS support
On Mon, 2012-03-19 at 22:04 +0000, Ross Philipson wrote:
> From:
> Ross Philipson
> <Ross.Philipson@xxxxxxxxxx>
> To:
> xen-devel@xxxxxxxxxxxxxxxxxxx
> <xen-devel@xxxxxxxxxxxxxxxxxxx>
> Subject:
> [Xen-devel] [PATCH 04/07] HVM
> firmware passthrough: SMBIOS
> support
> Date:
> Mon, 19 Mar 2012 22:04:09 +0000
> Message-id:
> <831D55AF5A11D64C9B4B43F59EEBF720724FB1BAAC@xxxxxxxxxxxxxxxxxxxxxxxxx>
>
>
> Pass through support for the SMBIOS tables including a few new
> DMTF defines types and support of OEM defined tables. Passed in
> SMBIOS types override the default internal values. Flags control
> the new tables so that with default values the SMBIOS tables look
> exactly as they do before the patches.
>
> Signed-off-by: Ross Philipson <ross.philipson@xxxxxxxxxx>
>
>
>
>
>
>
>
>
> differences
> between files
> attachment
> (hvm-firmware-passthrough-04.patch)
>
> # HG changeset patch
> # Parent a0d871dadf1d482d77bf57a059a55098e2bfb078
> Pass through support for the SMBIOS tables including a few new
> DMTF defines types and support of OEM defined tables. Passed in
> SMBIOS types override the default internal values. Flags control
> the new tables so that with default values the SMBIOS tables look
> exactly as they do before the patches.
>
> Signed-off-by: Ross Philipson <ross.philipson@xxxxxxxxxx>
>
> diff -r a0d871dadf1d tools/firmware/hvmloader/smbios.c
> --- a/tools/firmware/hvmloader/smbios.c Mon Mar 19 16:45:12 2012 -0400
> +++ b/tools/firmware/hvmloader/smbios.c Mon Mar 19 16:52:13 2012 -0400
> @@ -23,8 +23,10 @@
> #include <stdint.h>
> #include <xen/xen.h>
> #include <xen/version.h>
> +#include <xen/hvm/hvm_defs.h>
> #include "smbios_types.h"
> #include "util.h"
> +#include "modules.h"
> #include "hypercall.h"
>
> static int
> @@ -49,6 +51,8 @@ static void *
> smbios_type_1_init(void *start, const char *xen_version,
> uint8_t uuid[16]);
> static void *
> +smbios_type_2_init(void *start);
> +static void *
> smbios_type_3_init(void *start);
> static void *
> smbios_type_4_init(void *start, unsigned int cpu_number,
> @@ -64,8 +68,14 @@ smbios_type_19_init(void *start, uint32_
> static void *
> smbios_type_20_init(void *start, uint32_t memory_size_mb, int
> instance);
> static void *
> +smbios_type_22_init(void *start);
> +static void *
> smbios_type_32_init(void *start);
> static void *
> +smbios_type_39_init(void *start);
> +static void *
> +smbios_type_vendor_oem_init(void *start);
> +static void *
> smbios_type_127_init(void *start);
>
> static void
> @@ -85,6 +95,35 @@ get_cpu_manufacturer(char *buf, int len)
> strncpy(buf, "unknown", len);
> }
>
> +static uint32_t
> +get_smbios_control_flags(void)
> +{
> + static uint64_t flags;
> + static int read = 0;
> +
> + if ( !read )
> + {
> + flags = strtoll(xenstore_read(HVM_XS_SMBIOS_FLAGS, "0"),
> NULL, 0);
Rather than encoding these as a bit field, written as ascii into
xenstore and then parsed back into a number again I think you may as
well just have a node for each individual flag.
> + read = 1;
> + }
> +
> + return (uint32_t)flags;
> +}
> +
> +static void*
> +get_smbios_passthrough_table(uint8_t type, uint32_t *length_out)
> +{
> + struct hvm_modules_iterator iter;
> + uint32_t flags = get_smbios_control_flags();
> +
> + if ( flags & HVM_SMBIOS_DISABLE_PASSTHROUGH )
This check is the same as just not providing the tables?
> + return NULL;
> +
> + init_smbios_module_iterator(&iter, type, 0);
> +
> + return hvm_find_module_entry(&iter, length_out);
> +}
> +
> static int
> write_smbios_tables(void *ep, void *start,
> uint32_t vcpus, uint64_t memsize,
> [...]
> @@ -338,6 +393,18 @@ smbios_type_1_init(void *start, const ch
> char uuid_str[37];
> struct smbios_type_1 *p = (struct smbios_type_1 *)start;
> const char *s;
> + void *ptable;
> + uint32_t length;
> +
> + ptable = get_smbios_passthrough_table(1, &length);
> + if ( (ptable != NULL)&&(length > 0) )
> + {
> + memcpy(start, ptable, length);
> +
> + /* Fix up, use consistent handles */
> + p->header.handle = 0x100;
> + return (start + length);
> + }
This seems like a pretty common pattern, could it be pulled up into the
caller of all these init functions?
I know it's a pre-existing problem but perhaps giving the handle numbers
#defined names would make things a bit more obvious. For the OEM tables
and the potential for clashes (which you commented on in that function)
at least then there would be a sort of index of the ones which are
used.
I suppose you could also then push responsibility for maintaining this
consistency up to whoever was providing the table and if they wanted
they could use there own numbering and ensure consistency across the
board themselves.
> memset(p, 0, sizeof(*p));
>
> @@ -380,12 +447,90 @@ smbios_type_1_init(void *start, const ch
> return start+1;
> }
>
> +/* Type 2 -- System Board */
> +static void *
> +smbios_type_2_init(void *start)
> +{
> + struct smbios_type_2 *p = (struct smbios_type_2 *)start;
> + uint8_t *bptr;
> + const char *s;
> + void *ptable;
> + uint32_t length;
> +
> + if ( (get_smbios_control_flags() &
> HVM_SMBIOS_INCLUDE_SYSTEM_BOARD) == 0 )
Is there any reason you wouldn't want this table now that you've added
it?
> + return start;
> +
> + ptable = get_smbios_passthrough_table(2, &length);
> + if ( (ptable != NULL)&&(length > 0) )
> + {
> + memcpy(start, ptable, length);
> +
> + /* Fix up, use consistent handles, set chassis handle */
> + p->header.handle = 0x200;
> +
> + if ( p->header.length >= 13 )
> + {
> + bptr = ((uint8_t*)start) + 11;
> + if ( *((uint16_t*)bptr) != 0 )
> + *((uint16_t*)bptr) = 0x300; /* current chassis handle
This is to match the handle of the type 3 table?
I don't see a corresponding thing in the constructed version of this
table below, is that normal?
> */
> + }
> +
> + return (start + length);
> + }
> +
> + memset(p, 0, sizeof(*p));
> +
> + p->header.type = 2;
> + p->header.length = sizeof(struct smbios_type_2);
> + p->header.handle = 0x200;
> +
> + p->manufacturer_str = 1;
> + p->product_name_str = 2;
> + p->version_str = 0;
> + p->serial_number_str = 0;
> +
> + start += sizeof(struct smbios_type_2);
> +
> + s = xenstore_read("bios-strings/board-manufacturer", "Xen");
> + strcpy((char *)start, s);
> + start += strlen(s) + 1;
> +
> + s = xenstore_read("bios-strings/board-product-name", "HVM domU");
> + strcpy((char *)start, s);
> + start += strlen(s) + 1;
> +
> + /* no internal defaults for this if the value is not set */
> + s = xenstore_read("bios-strings/board-serial-number", NULL);
> + if ( (s != NULL)&&(*s != '\0') )
> + {
> + strcpy((char *)start, s);
> + start += strlen(s) + 1;
> + p->serial_number_str = 3;
> + }
> +
> + *((uint8_t *)start) = 0;
> + return start+1;
> +}
[...]
> @@ -403,9 +548,20 @@ smbios_type_3_init(void *start)
> p->security_status = 0x02; /* unknown */
>
> start += sizeof(struct smbios_type_3);
> -
> - strcpy((char *)start, "Xen");
> - start += strlen("Xen") + 1;
> +
> + s = xenstore_read("bios-strings/enclosure-manufacturer", "Xen");
> + strcpy((char *)start, s);
> + start += strlen(s) + 1;
> +
> + /* no internal defaults for this if the value is not set */
> + s = xenstore_read("bios-strings/enclosure-serial-number", NULL);
> + if ( (s != NULL)&&(*s != '\0') )
> + {
> + strcpy((char *)start, s);
> + start += strlen(s) + 1;
> + p->serial_number_str = 2;
> + }
> +
> *((uint8_t *)start) = 0;
> return start+1;
> }
[...]
> @@ -609,6 +777,66 @@ smbios_type_20_init(void *start, uint32_
> return start+2;
> }
>
> +/* Type 22 -- Portable Battery */
> +static void *
> +smbios_type_22_init(void *start)
> +{
> + struct smbios_type_22 *p = (struct smbios_type_22 *)start;
> + static const char *smbios_release_date = __SMBIOS_DATE__;
> + void *ptable;
> + uint32_t length;
> +
> + if ( (get_smbios_control_flags() &
> HVM_SMBIOS_INCLUDE_PORTABLE_BATTERY)
> + == 0 )
> + return start;
Could an argument be made here that either you want to pass through some
underlying table or you don't want the table at all? Or is there a use
case for having hvmloader fake one up?
> +
> + ptable = get_smbios_passthrough_table(22, &length);
> + if ( (ptable != NULL)&&(length > 0) )
> + {
> + memcpy(start, ptable, length);
> +
> + /* Fix up, use consistent handles */
> + p->header.handle = 0x1600;
> + return (start + length);
> + }
> +
> + memset(p, 0, sizeof(*p));
> +
> + p->header.type = 22;
> + p->header.length = sizeof(struct smbios_type_22);
> + p->header.handle = 0x1600;
> +
> + p->location_str = 1;
> + p->manufacturer_str = 2;
> + p->manufacturer_date_str = 3;
> + p->serial_number_str = 0;
> + p->device_name_str = 4;
> + p->device_chemistry = 0x2; /* Unknown */
> + p->device_capacity = 0; /* Unknown */
> + p->device_voltage = 0; /* Unknown */
> + p->sbds_version_number = 0;
> + p->max_error = 0xff; /* Unknown */
> + p->sbds_serial_number = 0;
> + p->sbds_manufacturer_date = 0;
> + p->sbds_device_chemistry = 0;
> + p->design_capacity_multiplier = 0;
> + p->oem_specific = 0;
> +
> + start += sizeof(struct smbios_type_22);
> +
> + strcpy((char *)start, "Primary");
> + start += strlen("Primary") + 1;
> + strcpy((char *)start, "Xen");
> + start += strlen("Xen") + 1;
> + strcpy((char *)start, smbios_release_date);
> + start += strlen(smbios_release_date) + 1;
> + strcpy((char *)start, "XEN-VBAT");
> + start += strlen("XEN-VBAT") + 1;
> +
> + *((uint8_t *)start) = 0;
> + return start+1;
> +}
> +
> /* Type 32 -- System Boot Information */
> static void *
> smbios_type_32_init(void *start)
> @@ -628,6 +856,71 @@ smbios_type_32_init(void *start)
> return start+2;
> }
>
> +/* Type 39 -- Power Supply */
> +static void *
> +smbios_type_39_init(void *start)
> +{
> + struct smbios_type_39 *p = (struct smbios_type_39 *)start;
> + void *ptable;
> + uint32_t length;
> +
> + if ( (get_smbios_control_flags() &
> HVM_SMBIOS_INCLUDE_POWER_SUPPLY)
> + == 0 )
> + return start;
> +
> + /* No default values - only present if the table is passed in */
> + ptable = get_smbios_passthrough_table(39, &length);
> + if ( (ptable == NULL)||(length == 0) )
> + return start;
> +
> + memcpy(start, ptable, length);
> +
> + /* Fix up, use consistent handles */
> + p->header.handle = 0x2700;
> +
> + /* These devices are not present */
> + p->input_voltage_probe_handle = 0xFFFF;
> + p->cooling_device_handle = 0xFFFF;
> + p->input_current_probe_handle = 0xFFFF;
I think responsibility for setting these up correctly ought to lie with
whoever passed the table in.
> +
> + return (start + length);
> +}
> +
> +static void *
> +smbios_type_vendor_oem_init(void *start)
> +{
> + struct hvm_modules_iterator iter;
> + uint32_t flags = get_smbios_control_flags();
> + void *ptable;
> + uint32_t length;
> +
> + if ( ((flags & HVM_SMBIOS_INCLUDE_VENDOR_OEM) == 0)||
> + (flags & HVM_SMBIOS_DISABLE_PASSTHROUGH) )
> + return start;
> +
> + /* Looking for any and all vendor/OEM defined tables passed in.
> */
> + init_smbios_module_iterator(&iter, 128, 255);
> +
> + do
> + {
> + ptable = hvm_find_module_entry(&iter, &length);
> + if ( (ptable != NULL)&&(length > 0) )
> + {
> + /*
> + * Vendor/OEM table, copy it in. Note the handle values
> cannot
> + * be changed since it is unknown what is in each of
> these tables
> + * but they could contain handle references to other
> tables. This
> + * means a slight risk of collision with the tables above
> but that
> + * would have to be dealt with on a case by case basis.
> + */
> + memcpy(start, ptable, length);
> + start += length;
Do you also need to track the size of the largest table as well as the
total number, for the purposes of filling in the fields in the entry
point?
> + }
> + } while ( ptable != NULL );
> +
> + return start;
> +}
> +
> /* Type 127 -- End of Table */
> static void *
> smbios_type_127_init(void *start)
> diff -r a0d871dadf1d tools/firmware/hvmloader/smbios_types.h
> --- a/tools/firmware/hvmloader/smbios_types.h Mon Mar 19 16:45:12
> 2012 -0400
> +++ b/tools/firmware/hvmloader/smbios_types.h Mon Mar 19 16:52:13
> 2012 -0400
> @@ -84,6 +84,15 @@ struct smbios_type_1 {
> uint8_t family_str;
> } __attribute__ ((packed));
>
> +/* SMBIOS type 2 - Base Board Information */
> +struct smbios_type_2 {
> + struct smbios_structure_header header;
> + uint8_t manufacturer_str;
> + uint8_t product_name_str;
> + uint8_t version_str;
> + uint8_t serial_number_str;
> +} __attribute__ ((packed));
> +
> /* SMBIOS type 3 - System Enclosure */
> struct smbios_type_3 {
> struct smbios_structure_header header;
> @@ -173,6 +182,26 @@ struct smbios_type_20 {
> uint8_t interleaved_data_depth;
> } __attribute__ ((packed));
>
> +/* SMBIOS type 22 - Portable battery */
> +struct smbios_type_22 {
> + struct smbios_structure_header header;
> + uint8_t location_str;
> + uint8_t manufacturer_str;
> + uint8_t manufacturer_date_str;
> + uint8_t serial_number_str;
> + uint8_t device_name_str;
> + uint8_t device_chemistry;
> + uint16_t device_capacity;
> + uint16_t device_voltage;
> + uint8_t sbds_version_number;
> + uint8_t max_error;
> + uint16_t sbds_serial_number;
> + uint16_t sbds_manufacturer_date;
> + uint8_t sbds_device_chemistry;
> + uint8_t design_capacity_multiplier;
> + uint32_t oem_specific;
> +} __attribute__ ((packed));
> +
> /* SMBIOS type 32 - System Boot Information */
> struct smbios_type_32 {
> struct smbios_structure_header header;
> @@ -180,6 +209,24 @@ struct smbios_type_32 {
> uint8_t boot_status;
> } __attribute__ ((packed));
>
> +/* SMBIOS type 39 - Power Supply */
> +struct smbios_type_39 {
> + struct smbios_structure_header header;
> + uint8_t power_unit_group;
> + uint8_t location_str;
> + uint8_t device_name_str;
> + uint8_t manufacturer_str;
> + uint8_t serial_number_str;
> + uint8_t asset_tag_number_str;
> + uint8_t model_part_number_str;
> + uint8_t revision_level_str;
> + uint16_t max_capacity;
> + uint16_t characteristics;
> + uint16_t input_voltage_probe_handle;
> + uint16_t cooling_device_handle;
> + uint16_t input_current_probe_handle;
> +} __attribute__ ((packed));
> +
> /* SMBIOS type 127 -- End-of-table */
> struct smbios_type_127 {
> struct smbios_structure_header header;
>
>
>
>
>
>
>
> plain text
> document
> attachment
> (ATT00001.txt)
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |