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

[Jean Delvare]

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?

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?

> > 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.

Thanks,
-- 
Jean Delvare
SUSE L3 Support