Re: [PATCH v1] IntelSiliconPkg: Add FVI_SMBIOS_TYPE definition in FirmwareVersionInfo.h

["Chang, Hunter" <hunter.chang@intel.com>]

Thanks for the review!

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Friday, January 6, 2023 9:01 AM
To: Oram, Isaac W <isaac.w.oram@intel.com>; Chang, Hunter <hunter.chang@intel.com>; devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>
Subject: RE: [PATCH v1] IntelSiliconPkg: Add FVI_SMBIOS_TYPE definition in FirmwareVersionInfo.h

Isaac,
So, no public doc defines the SMBIOS entry: SMBIOS_TABLE_TYPE_OEM_INTEL_FVI?

Then, I am ok with the new addition 0xDD.

Reviewed-by: Ray Ni <ray.ni@intel.com>



> -----Original Message-----
> From: Oram, Isaac W <isaac.w.oram@intel.com>
> Sent: Friday, January 6, 2023 3:26 AM
> To: Ni, Ray <ray.ni@intel.com>; Chang, Hunter 
> <hunter.chang@intel.com>; devel@edk2.groups.io
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; S, Ashraf 
> Ali <ashraf.ali.s@intel.com>
> Subject: RE: [PATCH v1] IntelSiliconPkg: Add FVI_SMBIOS_TYPE 
> definition in FirmwareVersionInfo.h
> 
> Ray,
> 
> I think it is ok because this file is "Intel common definitions for an 
> OEM defined SMBIOS table structure".  We could say that the folder 
> name is wrong or the whole file is in the wrong place.  But this addition seems consistent with the rest of the file contents.
> 
> Regards,
> Isaac
> 
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Thursday, January 5, 2023 1:39 AM
> To: Chang, Hunter <hunter.chang@intel.com>; devel@edk2.groups.io
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, Isaac 
> W <isaac.w.oram@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>
> Subject: RE: [PATCH v1] IntelSiliconPkg: Add FVI_SMBIOS_TYPE 
> definition in FirmwareVersionInfo.h
> 
> If it's not defined by any spec, it's probably not a good idea to put the definition in an industry header file.
> 
> 
> > -----Original Message-----
> > From: Chang, Hunter <hunter.chang@intel.com>
> > Sent: Thursday, January 5, 2023 3:54 PM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, 
> > Isaac W <isaac.w.oram@intel.com>; S, Ashraf Ali 
> > <ashraf.ali.s@intel.com>
> > Subject: RE: [PATCH v1] IntelSiliconPkg: Add FVI_SMBIOS_TYPE 
> > definition in FirmwareVersionInfo.h
> >
> > It is from Sipkg.dec
> > gSiPkgTokenSpaceGuid.PcdFviSmbiosType|0xDD
> >
> > 0x00~0x7F are reserved for and defined by smbios spec 0x80~0xFF are 
> > available for system- and OEM-specific information
> >
> > I think there is no spec define 0xDD.
> >
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Thursday, January 5, 2023 3:05 PM
> > To: Chang, Hunter <hunter.chang@intel.com>; devel@edk2.groups.io
> > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, 
> > Isaac W <isaac.w.oram@intel.com>; S, Ashraf Ali 
> > <ashraf.ali.s@intel.com>
> > Subject: RE: [PATCH v1] IntelSiliconPkg: Add FVI_SMBIOS_TYPE 
> > definition in FirmwareVersionInfo.h
> >
> > Which spec defines 0xDD?
> >
> > > -----Original Message-----
> > > From: Chang, Hunter <hunter.chang@intel.com>
> > > Sent: Wednesday, January 4, 2023 3:47 PM
> > > To: devel@edk2.groups.io
> > > Cc: Chang, Hunter <hunter.chang@intel.com>; Ni, Ray 
> > > <ray.ni@intel.com>; Chaganty, Rangasai V 
> > > <rangasai.v.chaganty@intel.com>; Oram, Isaac W 
> > > <isaac.w.oram@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>
> > > Subject: [PATCH v1] IntelSiliconPkg: Add FVI_SMBIOS_TYPE 
> > > definition in FirmwareVersionInfo.h
> > >
> > > From: Hunter Chang <hunter.chang@intel.com>
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4242
> > >
> > > Define a macro for SmbiosFeaturePkg usage which named 
> > > INTEL_FVI_SMBIOS_TYPE and initialized to 0xDD in 
> > > IndustryStandard/FirmwareVersionInfo.h
> > >
> > > Signed-off-by: Hunter Chang <hunter.chang@intel.com>
> > >
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> > > Cc: Isaac Oram <isaac.w.oram@intel.com>
> > > Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
> > > ---
> > >
> > > Silicon/Intel/IntelSiliconPkg/Include/IndustryStandard/FirmwareVer
> > > si
> > > on
> > > Info.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git
> > > a/Silicon/Intel/IntelSiliconPkg/Include/IndustryStandard/FirmwareV
> > > er
> > > si
> > > onInfo.h
> > > b/Silicon/Intel/IntelSiliconPkg/Include/IndustryStandard/FirmwareV
> > > er
> > > si
> > > onInfo.h
> > > index b30bc3f9e7..466cb8e7d2 100644
> > > ---
> > > a/Silicon/Intel/IntelSiliconPkg/Include/IndustryStandard/FirmwareV
> > > er
> > > si
> > > onInfo.h
> > > +++ b/Silicon/Intel/IntelSiliconPkg/Include/IndustryStandard/Firmw
> > > +++ ar
> > > +++ eV
> > > +++ ersionInfo.h
> > > @@ -18,6 +18,7 @@
> > >  #include <IndustryStandard/SmBios.h>
> > >
> > >
> > >
> > >  #define INTEL_FIRMWARE_VERSION_INFO_GROUP_NAME    "Firmware Version Info"
> > >
> > > +#define INTEL_FVI_SMBIOS_TYPE                     0xDD
> > >
> > >
> > >
> > >  #pragma pack(1)
> > >
> > >
> > >
> > > --
> > > 2.26.2.windows.1