|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 04/07] HVM firmware passthrough: SMBIOS support
> -----Original Message-----
> From: Ian Campbell
> Sent: Thursday, April 05, 2012 6:38 AM
> To: Ross Philipson
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: 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.
Yes that would be better.
>
> > + 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?
Yeah I need to rethink the flags in general.
>
> > + 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 can try to make some of it common.
>
> 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.
Yes I really like that idea of defining the handles we use. I will make things
much clearer.
>
> > 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?
My thinking was that by default if you passed nothing in and did not set any
flags it would be exactly the same as before I started.
>
> > + 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?
So per the spec, the table can vary in size. The default one is the minimum but
if the passed in one contains the chassis handle field, it should match the
actual chassis table's handle. There is a bug here though; it should be:
if ( p->header.length > 13 )
>
> > */
> > + }
> > +
> > + 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?
TBH I am not 100% sure about this one. The faked up table was introduced when
we started surfacing ACPI battery devices in the guests. There are no comments
or records as to what the tie might be. This would need some testing with HVM
guests.
>
> > +
> > + 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.
Agreed
>
> > +
> > + 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?
No. The start += length and the do_struct() macro do that work.
>
> > + }
> > + } 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 |