On 05/12/2015 10:56 AM, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1220527 > > This type of information defines attributes of a system > baseboard. With one caveat: in qemu they call it family, while > in the specification it's referred to as type. I'm sticking with > the latter. Hmm... It seems qemu_smbios_type2_opts has 'type' (and it's a number, more on that later...)... The "type1_opts" has 'family'... Or am I looking at the wrong place... I just pulled/built the latest sources from git://git.qemu.org/qemu.git and see: ./qemu-system-x86_64 --help | grep smbios -smbios file=binary -smbios type=0[,vendor=str][,version=str][,date=str][,release=%d.%d] -smbios type=1[,manufacturer=str][,product=str][,version=str][,serial=str] -smbios type=2[,manufacturer=str][,product=str][,version=str][,serial=str] -smbios type=3[,manufacturer=str][,version=str][,serial=str][,asset=str] -smbios type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str] -smbios type=17[,loc_pfx=str][,bank=str][,manufacturer=str][,serial=str] > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > --- > docs/formatdomain.html.in | 37 +++++- > docs/schemas/domaincommon.rng | 24 ++++ > src/conf/domain_conf.c | 57 +++++++++ > src/libvirt_private.syms | 1 + > src/qemu/qemu_command.c | 48 +++++++ > src/util/virsysinfo.c | 160 +++++++++++++++++++++++- > src/util/virsysinfo.h | 14 +++ > tests/qemuxml2argvdata/qemuxml2argv-smbios.args | 2 + > tests/qemuxml2argvdata/qemuxml2argv-smbios.xml | 9 ++ > 9 files changed, 346 insertions(+), 6 deletions(-) > According to how I read page 34 of the pdf file from .0: NOTE If more than one Type 2 structure is provided by an SMBIOS implementation, each structure shall include the Number of Contained Object Handles and Contained Object Handles fields to specify which system elements are contained on which boards. It seems we can have one of these in the SMBIOS... our .rng supports it, but we don't document it that way nor does the code handle more than one. Although I'll note that the Type0 (BIOS) and Type1 (SYSTEM) only support one, but the .rng has "oneOrMore"... If QEMU doesn't support more than 1, then we will need to document/ describe that. I assume there could be some other hypervisor that may want to support more than one. Also it seems that based on settings found in Type2, there could be a need/use for Type3 data. > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index e0b6ba7..dc92aa3 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -375,6 +375,13 @@ > <entry name='product'>Virt-Manager</entry> > <entry name='version'>0.9.4</entry> > </system> > + <baseBoard> > + <entry name='manufacturer'>LENOVO</entry> > + <entry name='product'>20BE0061MC</entry> > + <entry name='version'>0B98401 Pro</entry> > + <entry name='serial'>W1KS427111E</entry> > + <entry name='type'>Motherboard</entry> > + </baseBoard> According to how I read that .pdf - board type is read/passed as an ENUM not a STRING. Table 15 of the spec on page 36 describes the values (01 -> 0D currently). > </sysinfo> > ...</pre> > > @@ -435,11 +442,31 @@ > <dt><code>family</code></dt> > <dd>Identify the family a particular computer belongs to.</dd> > </dl> > - NB: Incorrectly supplied entries in either the <code>bios</code> > - or <code>system</code> blocks will be ignored without error. > - Other than <code>uuid</code> validation and <code>date</code> > - format checking, all values are passed as strings to the > - hypervisor driver. > + </dd> > + <dt><code>baseBoard</code></dt> > + <dd> > + This is block 2 of SMBIOS, with entry names drawn from: > + <dl> > + <dt><code>manufacturer</code></dt> > + <dd>Manufacturer of BIOS</dd> > + <dt><code>product</code></dt> > + <dd>Product Name</dd> > + <dt><code>version</code></dt> > + <dd>Version of the product</dd> > + <dt><code>serial</code></dt> > + <dd>Serial number</dd> > + <dt><code>asset</code></dt> > + <dd>Asset tag</dd> > + <dt><code>location</code></dt> > + <dd>Location in chassis</dd> > + <dt><code>type</code></dt> > + <dd>Board type</dd> > + </dl> > + NB: Incorrectly supplied entries in either the <code>bios</code> or s/in either/for/ s/or/, / > + <code>system</code> or <code>baseBoard</code> blocks will be > + ignored without error. Other than <code>uuid</code> validation and > + <code>date</code> format checking, all values are passed as strings > + to the hypervisor driver. > </dd> > </dl> > </dd> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index c151e92..2bc84b5 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -4240,6 +4240,18 @@ > </oneOrMore> > </element> > </optional> > + <optional> > + <element name="baseBoard"> > + <oneOrMore> > + <element name="entry"> > + <attribute name="name"> > + <ref name="sysinfo-baseBoard-name"/> > + </attribute> > + <ref name="sysinfo-value"/> > + </element> > + </oneOrMore> > + </element> > + </optional> > </interleave> > </element> > </define> > @@ -4265,6 +4277,18 @@ > </choice> > </define> > > + <define name="sysinfo-baseBoard-name"> > + <choice> > + <value>manufacturer</value> > + <value>product</value> > + <value>version</value> > + <value>serial</value> > + <value>asset</value> > + <value>location</value> > + <value>type</value> > + </choice> > + </define> > + > <define name="sysinfo-value"> > <data type="string"> > <param name='pattern'>[a-zA-Z0-9/\-_\. \(\)]+</param> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 209416d..0f41375 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -10997,6 +10997,52 @@ virSysinfoSystemParseXML(xmlNodePtr node, > return ret; > } > > +static int > +virSysinfoBaseBoardParseXML(xmlNodePtr node, > + xmlXPathContextPtr ctxt, > + virSysinfoBaseBoardDefPtr *baseBoard) > +{ > + int ret = -1; > + virSysinfoBaseBoardDefPtr def; > + > + if (!xmlStrEqual(node->name, BAD_CAST "baseBoard")) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("XML does not contain expected 'baseBoard' element")); > + return ret; > + } > + > + if (VIR_ALLOC(def) < 0) > + goto cleanup; > + > + def->manufacturer = > + virXPathString("string(entry[@name='manufacturer'])", ctxt); > + def->product = > + virXPathString("string(entry[@name='product'])", ctxt); > + def->version = > + virXPathString("string(entry[@name='version'])", ctxt); > + def->serial = > + virXPathString("string(entry[@name='serial'])", ctxt); > + def->asset = > + virXPathString("string(entry[@name='asset'])", ctxt); > + def->location = > + virXPathString("string(entry[@name='location'])", ctxt); > + def->type = > + virXPathString("string(entry[@name='type'])", ctxt); While I suppose 'type' could be read as string, it is a BYTE ENUM value. > + > + if (!def->manufacturer && !def->product && !def->version && > + !def->serial && !def->asset && !def->location && !def->type) { > + virSysinfoBaseBoardDefFree(def); > + def = NULL; > + } > + > + *baseBoard = def; > + def = NULL; > + ret = 0; > + cleanup: > + virSysinfoBaseBoardDefFree(def); > + return ret; > +} > + > static virSysinfoDefPtr > virSysinfoParseXML(xmlNodePtr node, > xmlXPathContextPtr ctxt, > @@ -11051,6 +11097,17 @@ virSysinfoParseXML(xmlNodePtr node, > ctxt->node = oldnode; > } > > + /* Extract system base board metadata */ And painfully - there can be more than one according the spec... > + if ((tmpnode = virXPathNode("./baseBoard[1]", ctxt)) != NULL) { > + oldnode = ctxt->node; > + ctxt->node = tmpnode; > + if (virSysinfoBaseBoardParseXML(tmpnode, ctxt, &def->baseBoard) < 0) { > + ctxt->node = oldnode; > + goto error; > + } > + ctxt->node = oldnode; > + } > + > cleanup: > VIR_FREE(type); > return def; > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index bd619d3..3178fe9 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -2176,6 +2176,7 @@ virVasprintfInternal; > > > # util/virsysinfo.h > +virSysinfoBaseBoardDefFree; > virSysinfoBIOSDefFree; > virSysinfoDefFree; > virSysinfoFormat; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 489cab3..c13089e 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -6738,6 +6738,48 @@ static char *qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def, > return NULL; > } > > +static char *qemuBuildSmbiosBaseBoardStr(virSysinfoBaseBoardDefPtr def) > +{ > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + > + if (!def) > + return NULL; > + > + virBufferAddLit(&buf, "type=2"); > + > + /* 1:Manufacturer */ > + if (def->manufacturer) > + virBufferAsprintf(&buf, ",manufacturer=%s", > + def->manufacturer); > + /* 1:Product Name */ > + if (def->product) > + virBufferAsprintf(&buf, ",product=%s", def->product); > + /* 1:Version */ > + if (def->version) > + virBufferAsprintf(&buf, ",version=%s", def->version); > + /* 1:Serial Number */ > + if (def->serial) > + virBufferAsprintf(&buf, ",serial=%s", def->serial); > + /* 1:Asset Tag */ > + if (def->asset) > + virBufferAsprintf(&buf, ",asset=%s", def->asset); > + /* 1:Location */ > + if (def->location) > + virBufferAsprintf(&buf, ",location=%s", def->location); > + /* 1:Type */ > + if (def->type) > + virBufferAsprintf(&buf, ",family=%s", def->type); And painfully if we handle more than one definition, then we'll have to deal with that here. Again, unless I'm looking at the wrong place - there is a 'type', but I'm not sure how it's passed > + > + if (virBufferCheckError(&buf) < 0) > + goto error; > + > + return virBufferContentAndReset(&buf); > + > + error: > + virBufferFreeAndReset(&buf); > + return NULL; > +} > + > static char * > qemuBuildClockArgStr(virDomainClockDefPtr def) > { > @@ -8925,6 +8967,12 @@ qemuBuildCommandLine(virConnectPtr conn, > virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL); > VIR_FREE(smbioscmd); > } > + > + smbioscmd = qemuBuildSmbiosBaseBoardStr(source->baseBoard); > + if (smbioscmd != NULL) { > + virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL); > + VIR_FREE(smbioscmd); > + } > } > } > > diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c > index 4c939ec..bb21e1b 100644 > --- a/src/util/virsysinfo.c > +++ b/src/util/virsysinfo.c > @@ -91,6 +91,20 @@ void virSysinfoSystemDefFree(virSysinfoSystemDefPtr def) > VIR_FREE(def); > } > > +void virSysinfoBaseBoardDefFree(virSysinfoBaseBoardDefPtr def) > +{ > + if (def == NULL) > + return; > + > + VIR_FREE(def->manufacturer); > + VIR_FREE(def->product); > + VIR_FREE(def->version); > + VIR_FREE(def->serial); > + VIR_FREE(def->asset); > + VIR_FREE(def->location); > + VIR_FREE(def->type); > + VIR_FREE(def); > +} > > /** > * virSysinfoDefFree: > @@ -108,6 +122,7 @@ void virSysinfoDefFree(virSysinfoDefPtr def) > > virSysinfoBIOSDefFree(def->bios); > virSysinfoSystemDefFree(def->system); > + virSysinfoBaseBoardDefFree(def->baseBoard); > > for (i = 0; i < def->nprocessor; i++) { > VIR_FREE(def->processor[i].processor_socket_destination); > @@ -719,6 +734,77 @@ virSysinfoParseSystem(const char *base, virSysinfoSystemDefPtr *system) > } > > static int > +virSysinfoParseBaseBoard(const char *base, virSysinfoBaseBoardDefPtr *baseBoard) > +{ > + int ret = -1; > + const char *cur, *eol = NULL; > + virSysinfoBaseBoardDefPtr def; > + > + if ((cur = strstr(base, "Base Board Information")) == NULL) > + return 0; > + > + if (VIR_ALLOC(def) < 0) > + return ret; > + > + base = cur; > + if ((cur = strstr(base, "Manufacturer: ")) != NULL) { > + cur += 14; > + eol = strchr(cur, '\n'); > + if (eol && VIR_STRNDUP(def->manufacturer, cur, eol - cur) < 0) > + goto cleanup; > + } > + if ((cur = strstr(base, "Product Name: ")) != NULL) { > + cur += 14; > + eol = strchr(cur, '\n'); > + if (eol && VIR_STRNDUP(def->product, cur, eol - cur) < 0) > + goto cleanup; > + } > + if ((cur = strstr(base, "Version: ")) != NULL) { > + cur += 9; > + eol = strchr(cur, '\n'); > + if (eol && VIR_STRNDUP(def->version, cur, eol - cur) < 0) > + goto cleanup; > + } > + if ((cur = strstr(base, "Serial Number: ")) != NULL) { > + cur += 15; > + eol = strchr(cur, '\n'); > + if (eol && VIR_STRNDUP(def->serial, cur, eol - cur) < 0) > + goto cleanup; > + } > + if ((cur = strstr(base, "Asset Tag: ")) != NULL) { > + cur += 11; > + eol = strchr(cur, '\n'); > + if (eol && VIR_STRNDUP(def->asset, cur, eol - cur) < 0) > + goto cleanup; > + } > + if ((cur = strstr(base, "Location In Chassis: ")) != NULL) { > + cur += 21; > + eol = strchr(cur, '\n'); > + if (eol && VIR_STRNDUP(def->location, cur, eol - cur) < 0) > + goto cleanup; > + } > + if ((cur = strstr(base, "Type: ")) != NULL) { > + cur += 6; > + eol = strchr(cur, '\n'); > + if (eol && VIR_STRNDUP(def->type, cur, eol - cur) < 0) > + goto cleanup; > + } More than one issue as well... > + > + if (!def->manufacturer && !def->product && !def->version && > + !def->serial && !def->asset && !def->location && !def->type) { > + virSysinfoBaseBoardDefFree(def); > + def = NULL; > + } > + > + *baseBoard = def; > + def = NULL; > + ret = 0; > + cleanup: > + virSysinfoBaseBoardDefFree(def); > + return ret; > +} > + > +static int > virSysinfoParseProcessor(const char *base, virSysinfoDefPtr ret) > { > const char *cur, *tmp_base; > @@ -940,7 +1026,7 @@ virSysinfoRead(void) > return NULL; > } > > - cmd = virCommandNewArgList(path, "-q", "-t", "0,1,4,17", NULL); > + cmd = virCommandNewArgList(path, "-q", "-t", "0,1,2,4,17", NULL); > VIR_FREE(path); > virCommandSetOutputBuffer(cmd, &outbuf); > if (virCommandRun(cmd, NULL) < 0) > @@ -957,6 +1043,9 @@ virSysinfoRead(void) > if (virSysinfoParseSystem(outbuf, &ret->system) < 0) > goto error; > > + if (virSysinfoParseBaseBoard(outbuf, &ret->baseBoard) < 0) > + goto error; > + How do we handle more than one? > ret->nprocessor = 0; > ret->processor = NULL; > if (virSysinfoParseProcessor(outbuf, ret) < 0) > @@ -1027,6 +1116,32 @@ virSysinfoSystemFormat(virBufferPtr buf, virSysinfoSystemDefPtr def) > } > > static void > +virSysinfoBaseBoardFormat(virBufferPtr buf, virSysinfoBaseBoardDefPtr def) > +{ > + if (!def) > + return; > + > + virBufferAddLit(buf, "<baseBoard>\n"); > + virBufferAdjustIndent(buf, 2); > + virBufferEscapeString(buf, "<entry name='manufacturer'>%s</entry>\n", > + def->manufacturer); > + virBufferEscapeString(buf, "<entry name='product'>%s</entry>\n", > + def->product); > + virBufferEscapeString(buf, "<entry name='version'>%s</entry>\n", > + def->version); > + virBufferEscapeString(buf, "<entry name='serial'>%s</entry>\n", > + def->serial); > + virBufferEscapeString(buf, "<entry name='asset'>%s</entry>\n", > + def->asset); > + virBufferEscapeString(buf, "<entry name='location'>%s</entry>\n", > + def->location); > + virBufferEscapeString(buf, "<entry name='type'>%s</entry>\n", > + def->type); How do we handle more than one... > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</baseBoard>\n"); > +} > + > +static void > virSysinfoProcessorFormat(virBufferPtr buf, virSysinfoDefPtr def) > { > size_t i; > @@ -1157,6 +1272,7 @@ virSysinfoFormat(virBufferPtr buf, virSysinfoDefPtr def) > > virSysinfoBIOSFormat(buf, def->bios); > virSysinfoSystemFormat(buf, def->system); > + virSysinfoBaseBoardFormat(buf, def->baseBoard); > virSysinfoProcessorFormat(buf, def); > virSysinfoMemoryFormat(buf, def); > > @@ -1227,6 +1343,7 @@ virSysinfoSystemIsEqual(virSysinfoSystemDefPtr src, > desc, NULLSTR(src->name), NULLSTR(dst->name)); \ > } \ > } while (0) > + ^^ This should be in patch 2... > CHECK_FIELD(manufacturer, "system vendor"); > CHECK_FIELD(product, "system product"); > CHECK_FIELD(version, "system version"); > @@ -1241,6 +1358,44 @@ virSysinfoSystemIsEqual(virSysinfoSystemDefPtr src, > return identical; > } > > +static bool > +virSysinfoBaseBoardIsEqual(virSysinfoBaseBoardDefPtr src, > + virSysinfoBaseBoardDefPtr dst) > +{ > + bool identical = false; > + > + if (!src && !dst) > + return true; > + > + if ((src && !dst) || (!src && dst)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Target base board does not match source")); > + goto cleanup; > + } > + > +#define CHECK_FIELD(name, desc) \ > + do { \ > + if (STRNEQ_NULLABLE(src->name, dst->name)) { \ > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ > + _("Target sysinfo %s %s does not match source %s"), \ > + desc, NULLSTR(src->name), NULLSTR(dst->name)); \ > + } \ > + } while (0) > + > + CHECK_FIELD(manufacturer, "base board vendor"); > + CHECK_FIELD(product, "base board product"); > + CHECK_FIELD(version, "base board version"); > + CHECK_FIELD(serial, "base board serial"); > + CHECK_FIELD(asset, "base board asset"); > + CHECK_FIELD(location, "base board location"); > + CHECK_FIELD(type, "base board type"); > +#undef CHECK_FIELD > + > + identical = true; > + cleanup: > + return identical; > +} > + > bool virSysinfoIsEqual(virSysinfoDefPtr src, > virSysinfoDefPtr dst) > { > @@ -1269,6 +1424,9 @@ bool virSysinfoIsEqual(virSysinfoDefPtr src, > if (!virSysinfoSystemIsEqual(src->system, dst->system)) > goto cleanup; > > + if (!virSysinfoBaseBoardIsEqual(src->baseBoard, dst->baseBoard)) > + goto cleanup; > + > identical = true; > > cleanup: > diff --git a/src/util/virsysinfo.h b/src/util/virsysinfo.h > index c8cc1e8..fd727a6 100644 > --- a/src/util/virsysinfo.h > +++ b/src/util/virsysinfo.h > @@ -86,6 +86,18 @@ struct _virSysinfoSystemDef { > char *family; > }; > > +typedef struct _virSysinfoBaseBoardDef virSysinfoBaseBoardDef; > +typedef virSysinfoBaseBoardDef *virSysinfoBaseBoardDefPtr; > +struct _virSysinfoBaseBoardDef { > + char *manufacturer; > + char *product; > + char *version; > + char *serial; > + char *asset; > + char *location; > + char *type; > +}; > + > typedef struct _virSysinfoDef virSysinfoDef; > typedef virSysinfoDef *virSysinfoDefPtr; > struct _virSysinfoDef { > @@ -93,6 +105,7 @@ struct _virSysinfoDef { > > virSysinfoBIOSDefPtr bios; > virSysinfoSystemDefPtr system; > + virSysinfoBaseBoardDefPtr baseBoard; > > size_t nprocessor; > virSysinfoProcessorDefPtr processor; > @@ -105,6 +118,7 @@ virSysinfoDefPtr virSysinfoRead(void); > > void virSysinfoBIOSDefFree(virSysinfoBIOSDefPtr def); > void virSysinfoSystemDefFree(virSysinfoSystemDefPtr def); > +void virSysinfoBaseBoardDefFree(virSysinfoBaseBoardDefPtr def); > void virSysinfoDefFree(virSysinfoDefPtr def); > > int virSysinfoFormat(virBufferPtr buf, virSysinfoDefPtr def) > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args > index e939aca..7336cbe 100644 > --- a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args > +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args > @@ -4,5 +4,7 @@ pc -m 214 -smp 1 -smbios 'type=0,vendor=LENOVO,version=6FET82WW (3.12 )' \ > -smbios 'type=1,manufacturer=Fedora,product=Virt-Manager,version=0.8.2-3.fc14,\ > serial=32dfcb37-5af1-552b-357c-be8c3aa38310,\ > uuid=c7a5fdbd-edaf-9455-926a-d65c16db1809,sku=1234567890,family=Red Hat' \ > +-smbios 'type=2,manufacturer=Hewlett-Packard,product=0B4Ch,version=D,\ > +serial=CZC1065993,asset=CZC1065993,location=Upside down,family=Motherboard' \ Adjust here... > -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda \ > /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios.xml b/tests/qemuxml2argvdata/qemuxml2argv-smbios.xml > index a2caeea..924596c 100644 > --- a/tests/qemuxml2argvdata/qemuxml2argv-smbios.xml > +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios.xml > @@ -18,6 +18,15 @@ > <entry name='sku'>1234567890</entry> > <entry name='family'>Red Hat</entry> > </system> > + <baseBoard> > + <entry name='manufacturer'>Hewlett-Packard</entry> > + <entry name='product'>0B4Ch</entry> > + <entry name='version'>D</entry> > + <entry name='serial'>CZC1065993</entry> > + <entry name='asset'>CZC1065993</entry> > + <entry name='location'>Upside down</entry> > + <entry name='type'>Motherboard</entry> This needs to be 01 -> 0D, not a string.... Also I don't see how qemu code actually processes this. > + </baseBoard> > </sysinfo> > <os> > <type arch='i686' machine='pc'>hvm</type> > If we're going to support it, then an example with two (or more entries). John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list