Re: [dmidecode] [PATCH] update dmidecode to parse Modern Management Controller blocks

[Neil Horman]

On Mon, Aug 06, 2018 at 03:43:34PM +0200, Jean Delvare wrote:
> On Sun, 2018-08-05 at 12:49 -0400, Neil Horman wrote:
> > On Sat, Aug 04, 2018 at 10:59:39PM +0200, Jean Delvare wrote:
> > > But now that you are raising the point again, I took a look at the most
> > > recent specification and found the following note (apparently added in
> > > 2.8.0, but I did not pay attention at that time):
> > > 
> > > NOTE The Entry Point Structure and all SMBIOS structures assume a 
> > > little-endian ordering convention, unless
> > > explicitly specified otherwise, i.e., multi-byte numbers (WORD, DWORD, 
> > > etc.) are stored with the low-order
> > > byte at the lowest address and the high-order byte at the highest address.
> > 
> > Ok, cool, so this suggests that we should be using the ntoh* macros
> > consistently.
> 
> I'm not sure how ntoh* would help here. We now know that the DMI table
> is using little-endian. Network byte order, on the other hand, is big-
> endian. Did you mean le*toh functions?
> 
Yes, sorry, I'm specifically referring to the endian conversion functions in
endian.h (leXtoh and htoleX).  They are not strictly speaking posix compliant,
but are fairly universally available.

> As I understand it, that's pretty much what I open-coded with my macros
> in types.h? I'm not sure what would be the benefit of switching to
> le*toh functions instead. Using WORD() and DWORD() has the advantage
> that these are the terms used in the SMBIOS specification, so it makes
> it easier to match the code with the specification, in my opinion. Do
> you think differently?
> 
I think you missunderstood what I was suggesting.

the WORD and DWORD macros are exacty what map to leXtoh in types.h.
What I'm saying is, instead of maintaining a reinvented wheel, why not convert
types.h such that they only contain these defintions:

#define WORD(x) le16toh(x)
#define DWORD(x) le32toh(x)
#deine QWORD(x) le64toh(x)
 
That seems far more concise than maintaining a reimplementation of those macros,
and still maintains correlation to the spec.

> > > Which means I got it wrong and types.h needs to be revisited. This also
> > > suggests that nobody ever tried dmidecode on a big-endian system, or
> > > they would have noticed.
> > 
> > The only system I know of that would have been consistently big endian I 
> > think
> > would have been old pre power-9 systems, which IIRC never supported smbios.
> 
> Correct.
> 
> > > > That said, dmidecode can also read from a dump
> > > > file, so its possible that someone might take a dump on an intel 
> > > > system, and go
> > > > read it on an old power system.
> > > 
> > > This is a good point. The endianness of the original system is not
> > > recorded in the dump, so if the data endianness would depend on the
> > > original host, we would be in trouble. But as it seems now clear that
> > > all DMI data must be little-endian, we should be on the safe side. It's
> > > currently broken, but easy to fix.
> > 
> > Agreed, if we just assume consistent little endian from the source (be it
> > in-memory or file), we can move forward.
> 
> I think that's where we are header, yes. I made some preliminary tests
> on a POWER7 system. I have a patch almost ready, I'll post it for
> review soon.
> 
> > > Reading a dump from a powerpc system might be challenge because
> > > typically distributions don't package dmidecode on architectures which
> > > do not implement SMBIOS. But I could try building it from sources on
> > > the machine directly.
> > > 
> > > > I would say, it wouldn't hurt to update dmidecode to be endian aware, 
> > > > but such
> > > > an update is likely outside the scope of this patch.
> > > 
> > > Actually we need to update it to be endian agnostic ;-) but yes, that
> > > would be done in a separate patch.
> > 
> > Ok, so are you comfortable with this patch as it is?  I'm happy to start a
> > review of the code to consistently use the endian conversion macros if you 
> > like.
> 
> I did not read your patch yet, will do when I finally have time. You
> should be using WORD() etc like the rest of the code is doing, for
> consistency.
> 
I'll send a v3 then, the system I was validating this on just died on me.  As
soon as I get it back up, I'll test a version with the WORD/DWORD macros used.

Neil

> Thanks,
> -- 
> Jean Delvare
> SUSE L3 Support
>