From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f54.google.com (mail-ej1-f54.google.com [209.85.218.54]) by mx.groups.io with SMTP id smtpd.web11.2331.1613620294769890471 for ; Wed, 17 Feb 2021 19:51:35 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=d3onsyBb; spf=pass (domain: gmail.com, ip: 209.85.218.54, mailfrom: bruce.ashfield@gmail.com) Received: by mail-ej1-f54.google.com with SMTP id lu16so1761246ejb.9 for ; Wed, 17 Feb 2021 19:51:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=2wZIAZC9yxZ2mDBnWjpNcEl9yGwCYZmfu5a5UnmBEgk=; b=d3onsyBbgEtzSgcBV6Ws0COGWzN0vV1nW2XiVLgQDTnFU39KO6Qh6XZyDSbn87cZ9f hDPdRTSes7GGkFDH2vpdQhQrvyQE/SDlh7XDE+nI7Q/Fk4ChxdGxexWWxPiJ6WWf502l WPcQNDdFzfFMi+hMxa6kCk6A8NFFMpAuEzTko1uuz5skhi/y7pUDyJG9C4ohIVnvct5P CW8N18hZExMENn6xABX2CjDoC0yzrlCzunKLW8DpHV+2ukyZP0JvWkn88rHT3DoBuyl3 CJ/ZdqQzRvEJEyGV17cBIBX8k9xzFDyIdIqLAXG1W1v5vmolq6CKoQdCFCxdD2k9OgMr ZVzA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2wZIAZC9yxZ2mDBnWjpNcEl9yGwCYZmfu5a5UnmBEgk=; b=RtzC/dB5sYg26IEKDGsQqrTFw711hnvYoNcJgRcVfJlMYEE+vot5mL6Q+e0bp1J/od huRZmeDeg6/HvmZk8suaGggCfEtU3tPShC8RcQJUvDX5m4UOGu8FIDF44eOSrKkCXd8m fSsLPjUS4b0vAA+i3GRJXFMNTKq9EUVmEBkYqQkBnmztYgVhUNvsbFh3l3g43BlvI1I6 n/5bd+inOrAfSjw+5YO23+rQBv3j9Bt5MMv4dSYmvTa02kBOxDHsv8up30vY6HIvSojI 5TKGikvvUnNr15GQldRvzGU1AiRyOjsH+MW2/42SbiA6DHUBmc08Kix0JUdbf0hZrXsU uPFw== X-Gm-Message-State: AOAM530RESzEQBB+bKgSmwJM5pb0WriEqTl4IYamSgrlGJZwwD/wpBMJ TKQB33PurxDwOh2bwQhRpSqXlQQUVw0RUAlvi6U= X-Google-Smtp-Source: ABdhPJw+22Nid1mXd8x8AOeBAWMmaYbdiUOlhboMoElKNOM6ixt+zpp29be3BGgP1+h2i+MVjZAo3ZhG/87rKvPFQ/Q= X-Received: by 2002:a17:906:a295:: with SMTP id i21mr2079378ejz.334.1613620293192; Wed, 17 Feb 2021 19:51:33 -0800 (PST) MIME-Version: 1.0 References: <20210208181747.44789-1-mcroce@linux.microsoft.com> In-Reply-To: From: "Bruce Ashfield" Date: Wed, 17 Feb 2021 22:51:21 -0500 Message-ID: Subject: Re: [OE-core] [PATCH] recipes-kernel: add libbpf To: Matteo Croce Cc: Patches and discussions about the oe-core layer Content-Type: multipart/alternative; boundary="00000000000060cea305bb944006" --00000000000060cea305bb944006 Content-Type: text/plain; charset="UTF-8" On Wed, Feb 17, 2021 at 9:32 PM Matteo Croce wrote: > On Mon, Feb 8, 2021 at 9:13 PM Bruce Ashfield > wrote: > > > > On Mon, Feb 8, 2021 at 1:18 PM Matteo Croce > wrote: > > > > > > From: Matteo Croce > > > > > > Add a recipe to build libbpf from https://github.com/libbpf/libbpf > > > The only patch fixes a build issue, and it's already merged upstream. > > > > Thanks for the submission! I have a few comments / questions. > > > > To get this into oe-core, we should be commenting / documenting why it > > should be in core, versus another layer. The standard criteria is that > > there are enough varied users and that the functionality is common > > enough, that it belongs in core. > > > > There should also be some sort of oe-selftest for the functionality, > > otherwise, it is hard to detect breakages. Some sort of application > > that uses the library and that can be executed in qemu would be > > enough. > > > > What are the kernel requirements ? CONFIG_BPF is selected by other > > kernel configs (it has no menu entry, so it must be), is it that, or > > something else that is the requirement (classic BFP?). If that option > > is now always on, is that true for the reference kernel versions in > > master (5.4 and 5.10). > > > > Something like this will work? I see a similar trick in the uprobe recipe: > > do_compile() { > if grep -q "CONFIG_BPF_SYSCALL=y" ${STAGING_KERNEL_BUILDDIR}/.config > then > oe_runmake > else > bbnote "BFP syscall is not enabled" > fi > } > > Since it already depends on virtual/kernel, that will indeed work. I can't say that I'm a huge fan of checking the .config, since it leaves us open to changing config option names, but that is way less common now than it used to be, so it isn't a problem as a basic sanity check. As you mention, uprobe already sets the prior art on it anyway :D > > > Finally, does this work across all the supported architectures ? if > > not, we'll need compatibility settings. > > > > I'm adding this: > > COMPATIBLE_HOST = "(x86_64.*|i.86.*|aarch64).*-linux" > > That's reasonable. Interested folks can add other architectures if they are willing to help out and support. > > > > > > Signed-off-by: Matteo Croce > > > --- > > > ...01-install-don-t-preserve-file-owner.patch | 29 +++++++++++++++++++ > > > meta/recipes-kernel/libbpf/libbpf_0.3.bb | 25 ++++++++++++++++ > > > 2 files changed, 54 insertions(+) > > > create mode 100644 > meta/recipes-kernel/libbpf/libbpf/0001-install-don-t-preserve-file-owner.patch > > > create mode 100644 meta/recipes-kernel/libbpf/libbpf_0.3.bb > > > > > > diff --git > a/meta/recipes-kernel/libbpf/libbpf/0001-install-don-t-preserve-file-owner.patch > b/meta/recipes-kernel/libbpf/libbpf/0001-install-don-t-preserve-file-owner.patch > > > new file mode 100644 > > > index 0000000000..4e65d8d80a > > > --- /dev/null > > > +++ > b/meta/recipes-kernel/libbpf/libbpf/0001-install-don-t-preserve-file-owner.patch > > > @@ -0,0 +1,29 @@ > > > +From 7df10d91db6f533cc0f6c09f4ae8ad92918c6160 Mon Sep 17 00:00:00 2001 > > > +From: Matteo Croce > > > +Date: Tue, 26 Jan 2021 12:41:47 +0100 > > > +Subject: [PATCH] install: don't preserve file owner > > > + > > > +'cp -p' preserve file ownership, this may leave files owned by the > > > +current in user in /lib . > > > + > > > > We need the upstream status documented here, not just in the commit > message. > > > > You want the upstream commit, or just saying that it will go into 0.4 is > enough? > > We normally do something like: Upstream-status: submitted (0.4 release) That way it is abundantly clear that we aren't carrying it ourselves, and when we can drop it. > > > +Signed-off-by: Matteo Croce > > > +--- > > > + Makefile | 2 +- > > > + 1 file changed, 1 insertion(+), 1 deletion(-) > > > + > > > +diff --git a/Makefile b/Makefile > > > +index da33613..ab66edc 100644 > > > +--- a/Makefile > > > ++++ b/Makefile > > > +@@ -130,7 +130,7 @@ define do_s_install > > > + $(Q)if [ ! -d '$(DESTDIR)$2' ]; then \ > > > + $(INSTALL) -d -m 755 '$(DESTDIR)$2'; \ > > > + fi; > > > +- $(Q)cp -fpR $1 '$(DESTDIR)$2' > > > ++ $(Q)cp -fR $1 '$(DESTDIR)$2' > > > + endef > > > + > > > + install: all install_headers install_pkgconfig > > > +-- > > > +2.29.2 > > > + > > > diff --git a/meta/recipes-kernel/libbpf/libbpf_0.3.bb > b/meta/recipes-kernel/libbpf/libbpf_0.3.bb > > > new file mode 100644 > > > index 0000000000..402e57257f > > > --- /dev/null > > > +++ b/meta/recipes-kernel/libbpf/libbpf_0.3.bb > > > @@ -0,0 +1,25 @@ > > > +SUMMARY = "Library for BPF handling" > > > +DESCRIPTION = "Library for BPF handling" > > > +HOMEPAGE = "https://github.com/libbpf/libbpf" > > > +SECTION = "libs" > > > +LICENSE = "LGPLv2.1+" > > > + > > > +LIC_FILES_CHKSUM = > "file://../LICENSE.LPGL-2.1;md5=b370887980db5dd40659b50909238dbd" > > > + > > > +DEPENDS = "zlib elfutils" > > > + > > > +SRC_URI = "git://github.com/libbpf/libbpf.git;protocol=https" > > > +SRCREV = "051a4009f94d5633a8f734ca4235f0a78ee90469" > > > + > > > +# Backported from version 0.4 > > > +SRC_URI += "file://0001-install-don-t-preserve-file-owner.patch" > > > + > > > +S = "${WORKDIR}/git/src" > > > + > > > +inherit pkgconfig > > > > I see inherit pkgconfig, but no PACKAGECONFIG setting. Is it actually > > used here ? > > > > > + > > > +do_install() { > > > + make install DESTDIR=${D} LIBDIR=${libdir} > > > > Rather than a bare 'make' call, this should likely be oe_runmake > > > > Right. > While at it, what about adding DESTDIR and LIBDIR to ${EXTRA_OEMAKE}? > They are ignored in the build phase > > yep. that is doable. oe_runmake will pass them along if in the right variable. Bruce > > Bruce > > > > > +} > > > + > > > +BBCLASSEXTEND = "native" > > > -- > > > 2.29.2 > > > > > > > > > > > > > > > > > > -- > > - Thou shalt not follow the NULL pointer, for chaos and madness await > > thee at its end > > - "Use the force Harry" - Gandalf, Star Trek II > > Regards, > -- > per aspera ad upstream > -- - Thou shalt not follow the NULL pointer, for chaos and madness await thee at its end - "Use the force Harry" - Gandalf, Star Trek II --00000000000060cea305bb944006 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Wed, Feb 17, 2021 at 9:32 PM Matteo Croce <mcroce@linux.microsoft.com&= gt; wrote:
On Mo= n, Feb 8, 2021 at 9:13 PM Bruce Ashfield <bruce.ashfield@gmail.com> wrote:
>
> On Mon, Feb 8, 2021 at 1:18 PM Matteo Croce <mcroce@linux.microsoft.com&g= t; wrote:
> >
> > From: Matteo Croce <mcroce@microsoft.com>
> >
> > Add a recipe to build libbpf from https://github.com/libb= pf/libbpf
> > The only patch fixes a build issue, and it's already merged = upstream.
>
> Thanks for the submission! I have a few comments / questions.
>
> To get this into oe-core, we should be commenting / documenting why i= t
> should be in core, versus another layer. The standard criteria is tha= t
> there are enough varied users and that the functionality is common > enough, that it belongs in core.
>
> There should also be some sort of oe-selftest for the functionality,<= br> > otherwise, it is hard to detect breakages. Some sort of application > that uses the library and that can be executed in qemu would be
> enough.
>
> What are the kernel requirements ? CONFIG_BPF is selected by other > kernel configs (it has no menu entry, so it must be), is it that, or<= br> > something else that is the requirement (classic BFP?). If that option=
> is now always on, is that true for the reference kernel versions in > master (5.4 and 5.10).
>

Something like this will work? I see a similar trick in the uprobe recipe:=

do_compile() {
=C2=A0if grep -q "CONFIG_BPF_SYSCALL=3Dy" ${STAGING_KERNEL_BUILD= DIR}/.config
=C2=A0then
=C2=A0 oe_runmake
=C2=A0else
=C2=A0 bbnote "BFP syscall is not enabled"
=C2=A0fi
}


Since it already depends on virtual/kernel, that will ind= eed work. I can't say
that I'm a huge fan of checking the .config, since it leaves= us open to
cha= nging config option names, but that is way less common now than it
used to be, so it isn&#= 39;t a problem as a basic sanity check. As you mention,
uprobe already sets the prior art = on it anyway :D

=C2=A0

> Finally, does this work across all the supported architectures ? if > not, we'll need compatibility settings.
>

I'm adding this:

COMPATIBLE_HOST =3D "(x86_64.*|i.86.*|aarch64).*-linux"


That's reasonable. Interested folks can add other arc= hitectures if they
are willing to help out and support.

=C2=A0
> >
> > Signed-off-by: Matteo Croce <mcroce@microsoft.com>
> > ---
> >=C2=A0 ...01-install-don-t-preserve-file-owner.patch | 29 +++++++= ++++++++++++
> >=C2=A0 meta/recipes-kernel/libbpf/libbpf_0.3.bb=C2=A0 =C2=A0 =C2= =A0 | 25 ++++++++++++++++
> >=C2=A0 2 files changed, 54 insertions(+)
> >=C2=A0 create mode 100644 meta/recipes-kernel/libbpf/libbpf/0001-= install-don-t-preserve-file-owner.patch
> >=C2=A0 create mode 100644 meta/recipes-kernel/libbpf/libbpf_0.3.bb=
> >
> > diff --git a/meta/recipes-kernel/libbpf/libbpf/0001-install-don-= t-preserve-file-owner.patch b/meta/recipes-kernel/libbpf/libbpf/0001-instal= l-don-t-preserve-file-owner.patch
> > new file mode 100644
> > index 0000000000..4e65d8d80a
> > --- /dev/null
> > +++ b/meta/recipes-kernel/libbpf/libbpf/0001-install-don-t-prese= rve-file-owner.patch
> > @@ -0,0 +1,29 @@
> > +From 7df10d91db6f533cc0f6c09f4ae8ad92918c6160 Mon Sep 17 00:00:= 00 2001
> > +From: Matteo Croce <mcroce@microsoft.com>
> > +Date: Tue, 26 Jan 2021 12:41:47 +0100
> > +Subject: [PATCH] install: don't preserve file owner
> > +
> > +'cp -p' preserve file ownership, this may leave files o= wned by the
> > +current in user in /lib .
> > +
>
> We need the upstream status documented here, not just in the commit m= essage.
>

You want the upstream commit, or just saying that it will go into 0.4 is e= nough?


We normally do something like:

Upstream-status: submitted (0.4 release) <link t= o the pull request or mailing list submission>

That way it is abundantly= clear that we aren't carrying it ourselves, and when we can drop it.
=C2=A0
> > +Signed-off-by: Matteo Croce <mcroce@microsoft.com>
> > +---
> > + Makefile | 2 +-
> > + 1 file changed, 1 insertion(+), 1 deletion(-)
> > +
> > +diff --git a/Makefile b/Makefile
> > +index da33613..ab66edc 100644
> > +--- a/Makefile
> > ++++ b/Makefile
> > +@@ -130,7 +130,7 @@ define do_s_install
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0$(Q)if [ ! -d '$(DESTDIR)$2'= ]; then=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 \
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0$(INSTAL= L) -d -m 755 '$(DESTDIR)$2';=C2=A0 =C2=A0 \
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0fi;
> > +-=C2=A0 =C2=A0 =C2=A0 $(Q)cp -fpR $1 '$(DESTDIR)$2'
> > ++=C2=A0 =C2=A0 =C2=A0 $(Q)cp -fR $1 '$(DESTDIR)$2'
> > + endef
> > +
> > + install: all install_headers install_pkgconfig
> > +--
> > +2.29.2
> > +
> > diff --git a/meta/recipes-kernel/libbpf/libbpf_0.3.bb b/meta/reci= pes-kernel/libbpf/libbpf_0.3.bb
> > new file mode 100644
> > index 0000000000..402e57257f
> > --- /dev/null
> > +++ b/meta/recipes-kernel/libbpf/libbpf_0.3.bb
> > @@ -0,0 +1,25 @@
> > +SUMMARY =3D "Library for BPF handling"
> > +DESCRIPTION =3D "Library for BPF handling"
> > +HOMEPAGE =3D "https://github.com/libbpf/libbpf&= quot;
> > +SECTION =3D "libs"
> > +LICENSE =3D "LGPLv2.1+"
> > +
> > +LIC_FILES_CHKSUM =3D "file://../LICENSE.LPGL-2.1;md5=3Db37= 0887980db5dd40659b50909238dbd"
> > +
> > +DEPENDS =3D "zlib elfutils"
> > +
> > +SRC_URI =3D "git://github.com/l= ibbpf/libbpf.git;protocol=3Dhttps"
> > +SRCREV =3D "051a4009f94d5633a8f734ca4235f0a78ee90469"=
> > +
> > +# Backported from version 0.4
> > +SRC_URI +=3D "file://0001-install-don-t-preserve-file-owne= r.patch"
> > +
> > +S =3D "${WORKDIR}/git/src"
> > +
> > +inherit pkgconfig
>
> I see inherit pkgconfig, but no PACKAGECONFIG setting. Is it actually=
> used here ?
>
> > +
> > +do_install() {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0make install DESTDIR=3D${D} LIBDIR= =3D${libdir}
>
> Rather than a bare 'make' call, this should likely be oe_runm= ake
>

Right.
While at it, what about adding DESTDIR and LIBDIR to ${EXTRA_OEMAKE}?
They are ignored in the build phase


yep. that is doable. oe_runmake will pass them along if i= n the right variable.

Bru= ce

=C2=A0
> Bruce
>
> > +}
> > +
> > +BBCLASSEXTEND =3D "native"
> > --
> > 2.29.2
> >
> >
> >
> >
>
>
> --
> - Thou shalt not follow the NULL pointer, for chaos and madness await=
> thee at its end
> - "Use the force Harry" - Gandalf, Star Trek II

Regards,
--
per aspera ad upstream


--
- Thou shalt not follow the NULL pointer, for c= haos and madness await thee at its end
- "Use the force Harry"= - Gandalf, Star Trek II

--00000000000060cea305bb944006--