[Skiboot] [PATCH V2 08/10] libmctp: Import libmctp library handling MCTP protocol

[Frederic Barrat]

On 15/03/2022 13:49, Christophe Lombard wrote:
> The Management Component Transport Protocol (MCTP) is a protocol defined by
> the DMTF Platform Management Component Intercommunications sub-team of the
> DMTF Pre-OS Workgroup. MCTP is designed to support communications between
> different intelligent hardware components that make up a platform
> management subsystem that is provides monitoring and control functions
> inside a managed system. DMTF standard "DSP2016"
> 
> This library is intended to be a portable implementation of the Management
> Component Transport Protocol (MCTP), as defined by DMTF standard "DSP0236",
> plus transport binding specifications.
> MCTP has been designed to carry multiple types of manageability-related
> traffic across a common medium. The base MCTP specifications define
> message types for supporting the initialization and configuration of MCTP
> itself, and to support vendor-specific messages over MCTP.
> Other message types, such as message types to support a Platform Level
> Data Model (PLDM).
> 
> The source is located here: https://github.com/openbmc/libmctp and use as
> is, without any update.
> 
> The libmctp code is integrated into the folder ./libmctp as a set of
> sources, compilated if the compiler flag CONFIG_PLDM is set.


Typo "compilated" => "compiled" (also in the next 2 patches)

It may be worth re-synchronizing with latest upstream, since this is now 
one year old?


 > diff --git a/Makefile.main b/Makefile.main
 > index 2a346a6c..738e0857 100644
 > --- a/Makefile.main
 > +++ b/Makefile.main
 > @@ -75,7 +75,7 @@ DBG=-g
 >
 >   CPPFLAGS := -I$(SRC)/include -Iinclude -MMD -include 
$(SRC)/include/config.h
 >   CPPFLAGS += -I$(SRC)/libfdt -I$(SRC)/libflash -I$(SRC)/libxz 
-I$(SRC)/libc/include -I$(SRC)
 > -CPPFLAGS += -I$(SRC)/libpore
 > +CPPFLAGS += -I$(SRC)/libpore -I$(SRC)/libmctp
 >   CPPFLAGS += -D__SKIBOOT__ -nostdinc
 >   CPPFLAGS += -isystem $(shell $(CC) -print-file-name=include)
 >   CPPFLAGS += -DBITS_PER_LONG=64
 > @@ -91,6 +91,13 @@ else
 >   CPPFLAGS += -DHAVE_BIG_ENDIAN
 >   endif
 >
 > +# Also add the asm/byte-order.h style ones used by libmctp
 > +ifeq ($(LITTLE_ENDIAN),1)
 > +CPPFLAGS += -D__LITTLE_ENDIAN_BITFIELD
 > +else
 > +CPPFLAGS += -D__BIG_ENDIAN_BITFIELD
 > +endif
 > +


It would be simpler to merge with the previous "ifeq 
($(LITTLE_ENDIAN),1)" just above, since they are almost similar.


> diff --git a/libmctp/Makefile.inc b/libmctp/Makefile.inc
> new file mode 100644
> index 00000000..24d6fcf5
> --- /dev/null
> +++ b/libmctp/Makefile.inc
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
> +# Copyright 2022 IBM Corp.
> +
> +LIBMCTP_DIR ?= libmctp
> +SUBDIRS += $(LIBMCTP_DIR)
> +
> +LIBMCTP_OBJS = crc32.o core.o alloc.o log.o
> +LIBMCTP_BINDINGS ?= astlpc
> +
> +LIBMCTP_OBJS += $(LIBMCTP_BINDINGS:%=%.o)
> +
> +CFLAGS_$(LIBMCTP_DIR)/ = -I$(SRC)/ccan/endian/ \


The "-I$(SRC)/ccan/endian/" is useless, as you now have a 
include/endian.h file, which is in the default path and will also 
include the file found in ccan/


> diff --git a/libmctp/config.h b/libmctp/config.h

You may want to add a note in the commit message that even though the 
config.h file is typically automatically generated by 'configure', it's 
not the case here and it has been carefully edited to match the 
environment provided in the skiboot repo.

   Fred