Re: [PATCH 3/3] xl/libxl: Add OEM string support to smbios

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, Sep 8, 2022 at 9:14 AM Anthony PERARD <anthony.perard@xxxxxxxxxx> wrote:
>
> On Wed, Aug 10, 2022 at 03:48:27PM -0400, Jason Andryuk wrote:
> > Add support for OEM strings in the SMBIOS type 11.
> >
> > hvmloader checks them sequentially, so hide the implementation detail.
> > Allow multiple plain oem= items and assign the numeric values
> > internally.
> >
> > Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> > ---
> >  docs/man/xl.cfg.5.pod.in           |  4 ++
> >  tools/golang/xenlight/types.gen.go | 99 ++++++++++++++++++++++++++++++
> >  tools/libs/light/libxl_types.idl   | 99 ++++++++++++++++++++++++++++++
> >  tools/xl/xl_parse.c                | 15 +++++
> >  4 files changed, 217 insertions(+)
> >
> > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > index 7edf5d23f3..7947bf07ea 100644
> > --- a/docs/man/xl.cfg.5.pod.in
> > +++ b/docs/man/xl.cfg.5.pod.in
> > @@ -2104,8 +2104,12 @@ Each B<SMBIOS_SPEC_STRING> is a C<KEY=VALUE> string 
> > from the following list:
> >
> >  =item B<battery_device_name=STRING>
> >
> > +=item B<oem=STRING>
> > +
> >  =back
> >
> > +oem= strings can be specified mutiple times up to a limit of 99.
>
> This could be move to just before =back I think.

Moved like you suggest, the man page output is:
"""
           oem=STRING
               oem= strings can be specified mutiple times up to a limit of
               99.

       ms_vm_genid="OPTION"
"""

I'll also re-word to "oem= can be specified up to 99 times."

> > +
> >  =item B<ms_vm_genid="OPTION">
> >
> >  Provide a VM generation ID to the guest.
> > diff --git a/tools/libs/light/libxl_types.idl 
> > b/tools/libs/light/libxl_types.idl
> > index d04207748e..58f404af37 100644
> > --- a/tools/libs/light/libxl_types.idl
> > +++ b/tools/libs/light/libxl_types.idl
> > @@ -436,6 +436,105 @@ libxl_smbios_type = Enumeration("smbios_type", [
> >      (15, "enclosure_asset_tag"),
> >      (16, "battery_manufacturer"),
> >      (17, "battery_device_name"),
> > +    (18, "oem_1"),
> > +    (19, "oem_2"),
> > +    (20, "oem_3"),
> [...]
> > +    (115, "oem_98"),
> > +    (116, "oem_99"),
>
> Instead of this long lists of enum value, could we just have one "oem"
> type, and allow it to be used more than once? I know that would mean
> having a special case in libxl, but that also mean that libxl will be
> the only one to deal with the implementation detail on how to write oem
> string for hvmloader (as opposed to deal with this in every application
> using libxl).

I'll pursue a single "oem" type.  Also, I'll check the count in both
libxl and xl.  While only the libxl check is needed, it seems better
to detect issues while parsing the config instead of only later during
the domain build.

Thanks,
Jason