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

[Neil Horman]

On Tue, Aug 07, 2018 at 09:29:44AM +0200, Jean Delvare wrote:
> Hi Neil,
> 
> On Mon, 6 Aug 2018 16:07:56 -0400, Neil Horman wrote:
> > On Mon, Aug 06, 2018 at 03:43:34PM +0200, Jean Delvare wrote:
> > > 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.
> 
> That's tempting, however it does not seem like these macros are dealing
> with unaligned access. If we are running on a little-endian machine
> which does not support unaligned memory access (hello IA-64!), we end
> up with the following definitions:
> 
> #  define le16toh(x) (x)
> #  define le32toh(x) (x)
> #  define le64toh(x) (x)
> 
> which will surely crash.
I don't think so, at least not on linux, where the kernel installs a handler to
fix up unaligned accesses.  Either way, it should be pretty easily fixable I
think by defining the macros as:

#define WORD(x) le16toh(get_unaligned(x))
...
That may not be exactly right (might need to serialize the macro use), but its
close.

Either way, I guess it doesn't really matter, this was more about not
maintaining duplicate code for endian macros, but since its already written and
likely won't change again for another decade, theres less value in this.

> So we still need our own macros for the
> ALIGNMENT_WORKAROUND case. And then I don't think there is much benefit
> in using le*toh macros in one case, and custom code in the other, as
> this is the same amount of code we have now?
> 
My suggestion was less about the amount of code, and more about maintaining
multiple implementations....i.e. why maintain your own definition when the OS
offers it to you.

> Also, endian.h doesn't seem to be available on Solaris? I no longer
> have access to such system for proper testing, but at least online man
> pages don't have any reference to it.
> 

I don't either, but you appear to be right, solaris reuires that you cook your
own versions of these macros with something like this:
#elif defined(__sun)

#include <sys/byteorder.h>
#ifdef _LITTLE_ENDIAN
#define __BYTE_ORDER __LITTLE_ENDIAN
#define le16toh(x) get_unaligned(x)
#else
#define le16toh(x) bswap_16(get_unaligned(x))
#define __BYTE_ORDER __BIG_ENDIAN
#endif


Which is starting to get onerous in its own right....

> > > (...)
> > > 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.
> 
> May I suggest that you dump the SMBIOS table of said system to a file,
> so you can test your code on another host using --from-dump, thus no
> longer depending on the availability of said system? It would be great
> if you are also able to share that dump with me, so that I can add it
> to my test suite.
> 
Sure, as soon as I get it back up and running.  Also note, I'm starting to think
it might have a problem in the offset of its protocol record area (I'll need
another system to compare it too, which I'll have later this week I think).
Either way, it might be useful to have an erroneous dump for testing purposes

Neil

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