[2/2] eal: handle compressed firmwares

Message ID 20210602095836.24901-3-david.marchand@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series Support compressed firmwares |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot success github build: passed
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing fail Testing issues

Commit Message

David Marchand June 2, 2021, 9:58 a.m. UTC
  Introduce an internal firmware loading helper to remove code duplication
in our drivers and handle xz compressed firmwares by calling libarchive.

This helper tries to look for .xz suffixes so that drivers are not aware
the firmwares have been compressed.

libarchive is set as an optional dependency: without libarchive, a
runtime warning is emitted so that users know there is a compressed
firmware.

Windows implementation is left as an empty stub.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 .github/workflows/build.yml    |   1 +
 .travis.yml                    |   1 +
 config/meson.build             |   9 +++
 drivers/net/bnx2x/bnx2x.c      |  35 ++++-------
 drivers/net/ice/ice_ethdev.c   |  60 ++++---------------
 drivers/net/nfp/nfp_net.c      |  57 ++++--------------
 drivers/net/qede/qede_main.c   |  44 ++++++--------
 lib/eal/include/rte_firmware.h |  35 +++++++++++
 lib/eal/unix/eal_firmware.c    | 106 +++++++++++++++++++++++++++++++++
 lib/eal/unix/meson.build       |   1 +
 lib/eal/version.map            |   2 +
 lib/eal/windows/eal.c          |   9 +++
 12 files changed, 218 insertions(+), 142 deletions(-)
 create mode 100644 lib/eal/include/rte_firmware.h
 create mode 100644 lib/eal/unix/eal_firmware.c
  

Comments

Jerin Jacob June 2, 2021, 11:13 a.m. UTC | #1
On Wed, Jun 2, 2021 at 3:29 PM David Marchand <david.marchand@redhat.com> wrote:
>
> Introduce an internal firmware loading helper to remove code duplication
> in our drivers and handle xz compressed firmwares by calling libarchive.
>
> This helper tries to look for .xz suffixes so that drivers are not aware
> the firmwares have been compressed.
>
> libarchive is set as an optional dependency: without libarchive, a
> runtime warning is emitted so that users know there is a compressed
> firmware.
>
> Windows implementation is left as an empty stub.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

> + */
> +
> +#ifndef __RTE_FIRMWARE_H__
> +#define __RTE_FIRMWARE_H__
> +
> +#include <sys/types.h>
> +
> +#include <rte_compat.h>
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Load a firmware in a dynamically allocated buffer, dealing with compressed
> + * files if libarchive is available.
> + *
> + * @param name
> + *      Firmware filename to load.
> + * @param buf

Adding out to express the output useful. i.e
@param[out] buf

> + *      Buffer allocated by this function. If this function succeeds, the
> + *      caller is responsible for freeing the buffer.

I think, we can chnange to "freeing the buffer using free()" to avoid
confusion with rte_free()

> + * @param bufsz

@param[out] bufsz

> + *      Size of the data in the buffer.
> + *
> + * @return
> + *      0 if successful.
> + *      Negative otherwise, buf and bufsize contents are invalid.
> + */
> +__rte_internal
> +int
> +rte_firmware_read(const char *name, void **buf, size_t *bufsz);
> +
> +#endif
> diff --git a/lib/eal/unix/eal_firmware.c b/lib/eal/unix/eal_firmware.c
> new file mode 100644
> index 0000000000..ea66fecfe9
> --- /dev/null
> +++ b/lib/eal/unix/eal_firmware.c
> @@ -0,0 +1,106 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2021 Red Hat, Inc.
> + */
> +
> +#ifdef RTE_HAS_LIBARCHIVE
> +#include <archive.h>
> +#endif
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +#include <rte_common.h>
> +#include <rte_firmware.h>
> +#include <rte_log.h>
> +
> +static int
> +firmware_read(const char *name, void **buf, size_t *bufsz)
> +{
> +       const size_t blocksize = 4096;
> +       int ret = -1;
> +       int err;
> +#ifdef RTE_HAS_LIBARCHIVE


I think, better to have small inline functions for libarchive variant
vs normal file accessors
in the group for open, access, read etc to avoid the ifdef clutter and
manage with one ifdef.



> +       struct archive_entry *entry;
> +       struct archive *a;
> +#else
> +       int fd;
> +#endif
> +
> +       *buf = NULL;
> +       *bufsz = 0;
> +
> +#ifdef RTE_HAS_LIBARCHIVE

See above

> +       a = archive_read_new();
> +       if (a == NULL || archive_read_support_format_raw(a) != ARCHIVE_OK ||
> +                       archive_read_support_filter_xz(a) != ARCHIVE_OK ||
> +                       archive_read_open_filename(a, name, blocksize) != ARCHIVE_OK ||
> +                       archive_read_next_header(a, &entry) != ARCHIVE_OK)
> +               goto out;
> +#else
> +       fd = open(name, O_RDONLY);
> +       if (fd < 0)
> +               goto out;
> +#endif
> +
> +       do {
> +               void *tmp;
> +
> +               tmp = realloc(*buf, *bufsz + blocksize);
> +               if (tmp == NULL) {
> +                       free(*buf);
> +                       *buf = NULL;
> +                       *bufsz = 0;
> +                       break;
> +               }
> +               *buf = tmp;
> +
> +#ifdef RTE_HAS_LIBARCHIVE

See above

> +               err = archive_read_data(a, RTE_PTR_ADD(*buf, *bufsz), blocksize);
> +#else
> +               err = read(fd, RTE_PTR_ADD(*buf, *bufsz), blocksize);
> +#endif
> +               if (err < 0) {
> +                       free(*buf);
> +                       *buf = NULL;
> +                       *bufsz = 0;
> +                       break;
> +               }
> +               *bufsz += err;
> +
> +       } while (err != 0);
> +
> +       if (*buf != NULL)
> +               ret = 0;
> +out:
> +#ifdef RTE_HAS_LIBARCHIVE

See above

> +       if (a != NULL)
> +               archive_read_free(a);
> +#else
> +       if (fd >= 0)
> +               close(fd);
> +#endif
> +       return ret;
> +}
> +
> +int
> +rte_firmware_read(const char *name, void **buf, size_t *bufsz)
> +{
> +       char path[PATH_MAX];
> +       int ret;
> +
> +       ret = firmware_read(name, buf, bufsz);
> +       if (ret < 0) {
> +               snprintf(path, sizeof(path), "%s.xz", name);
> +               path[PATH_MAX - 1] = '\0';
> +#ifndef RTE_HAS_LIBARCHIVE

See above

> +               if (access(path, F_OK) == 0) {
> +                       RTE_LOG(WARNING, EAL, "libarchive not available, %s cannot be decompressed\n",
> +                               path);
> +               }
> +#else
> +               ret = firmware_read(path, buf, bufsz);
> +#endif
>
  
Igor Russkikh June 2, 2021, 11:30 a.m. UTC | #2
> Introduce an internal firmware loading helper to remove code duplication
> in our drivers and handle xz compressed firmwares by calling libarchive.
> 
> This helper tries to look for .xz suffixes so that drivers are not aware
> the firmwares have been compressed.
> 
> libarchive is set as an optional dependency: without libarchive, a
> runtime warning is emitted so that users know there is a compressed
> firmware.
> 
> Windows implementation is left as an empty stub.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

for QEDE,

Reviewed-by: Igor Russkikh <irusskikh@marvell.com>

Devendra, please give it a try when possible.

Regards,
  Igor
  
David Marchand June 2, 2021, 3:46 p.m. UTC | #3
On Wed, Jun 2, 2021 at 1:13 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > +static int
> > +firmware_read(const char *name, void **buf, size_t *bufsz)
> > +{
> > +       const size_t blocksize = 4096;
> > +       int ret = -1;
> > +       int err;
> > +#ifdef RTE_HAS_LIBARCHIVE
>
>
> I think, better to have small inline functions for libarchive variant
> vs normal file accessors
> in the group for open, access, read etc to avoid the ifdef clutter and
> manage with one ifdef.

That may be a bit artificial, since there is no reuse of such helpers for now.
I'll have a try and see how it looks.


> > +int
> > +rte_firmware_read(const char *name, void **buf, size_t *bufsz)
> > +{
> > +       char path[PATH_MAX];
> > +       int ret;
> > +
> > +       ret = firmware_read(name, buf, bufsz);
> > +       if (ret < 0) {
> > +               snprintf(path, sizeof(path), "%s.xz", name);
> > +               path[PATH_MAX - 1] = '\0';
> > +#ifndef RTE_HAS_LIBARCHIVE
>
> See above

There is nothing to abstract here.

If you don't have libarchive, returning the .xz content to a driver is wrong.
I prefer to leave this block as is.



>
> > +               if (access(path, F_OK) == 0) {
> > +                       RTE_LOG(WARNING, EAL, "libarchive not available, %s cannot be decompressed\n",
> > +                               path);
> > +               }
> > +#else
> > +               ret = firmware_read(path, buf, bufsz);
> > +#endif
> >
>
  
Dmitry Kozlyuk June 2, 2021, 9:19 p.m. UTC | #4
2021-06-02 11:58 (UTC+0200), David Marchand:
> Introduce an internal firmware loading helper to remove code duplication
> in our drivers and handle xz compressed firmwares by calling libarchive.
> 
> This helper tries to look for .xz suffixes so that drivers are not aware
> the firmwares have been compressed.
> 
> libarchive is set as an optional dependency: without libarchive, a
> runtime warning is emitted so that users know there is a compressed
> firmware.
> 
> Windows implementation is left as an empty stub.

JFYI, it doesn't seem hard to implement as a follow-up if need be.
libarchive is available for Windows and even provides a .pc file.

[...]
> diff --git a/config/meson.build b/config/meson.build
> index 017bb2efbb..337daa2719 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -166,6 +166,15 @@ if fdt_dep.found() and cc.has_header('fdt.h')
>      dpdk_extra_ldflags += '-lfdt'
>  endif
>  
> +has_libarchive = 0
> +archive_dep = cc.find_library('libarchive', required: false)
> +if archive_dep.found() and cc.has_header('archive.h')
> +    dpdk_conf.set10('RTE_HAS_LIBARCHIVE', true)
> +    has_libarchive = 1
> +    add_project_link_arguments('-larchive', language: 'c')
> +    dpdk_extra_ldflags += '-larchive'
> +endif
> +

Why not use pkg-config?
`has_libarchive` is unused.

>  libexecinfo = cc.find_library('libexecinfo', required: false)
>  if libexecinfo.found() and cc.has_header('execinfo.h')
>      add_project_link_arguments('-lexecinfo', language: 'c')
[...]
> diff --git a/lib/eal/version.map b/lib/eal/version.map
> index fe5c3dac98..020c058e5f 100644
> --- a/lib/eal/version.map
> +++ b/lib/eal/version.map
> @@ -423,11 +423,13 @@ EXPERIMENTAL {
>  	rte_version_release; # WINDOWS_NO_EXPORT
>  	rte_version_suffix; # WINDOWS_NO_EXPORT
>  	rte_version_year; # WINDOWS_NO_EXPORT
> +
>  };

Unnecessary empty line.

>  
>  INTERNAL {
>  	global:
>  
> +	rte_firmware_read;
>  	rte_mem_lock;
>  	rte_mem_map;
>  	rte_mem_page_size;
  
David Marchand June 3, 2021, 7:23 a.m. UTC | #5
On Wed, Jun 2, 2021 at 11:19 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> 2021-06-02 11:58 (UTC+0200), David Marchand:
> > Introduce an internal firmware loading helper to remove code duplication
> > in our drivers and handle xz compressed firmwares by calling libarchive.
> >
> > This helper tries to look for .xz suffixes so that drivers are not aware
> > the firmwares have been compressed.
> >
> > libarchive is set as an optional dependency: without libarchive, a
> > runtime warning is emitted so that users know there is a compressed
> > firmware.
> >
> > Windows implementation is left as an empty stub.
>
> JFYI, it doesn't seem hard to implement as a follow-up if need be.
> libarchive is available for Windows and even provides a .pc file.

I noticed.
But I am not sure the current API is abstracted enough since we pass a
firmware filename.

>
> [...]
> > diff --git a/config/meson.build b/config/meson.build
> > index 017bb2efbb..337daa2719 100644
> > --- a/config/meson.build
> > +++ b/config/meson.build
> > @@ -166,6 +166,15 @@ if fdt_dep.found() and cc.has_header('fdt.h')
> >      dpdk_extra_ldflags += '-lfdt'
> >  endif
> >
> > +has_libarchive = 0
> > +archive_dep = cc.find_library('libarchive', required: false)
> > +if archive_dep.found() and cc.has_header('archive.h')
> > +    dpdk_conf.set10('RTE_HAS_LIBARCHIVE', true)
> > +    has_libarchive = 1
> > +    add_project_link_arguments('-larchive', language: 'c')
> > +    dpdk_extra_ldflags += '-larchive'
> > +endif
> > +
>
> Why not use pkg-config?
> `has_libarchive` is unused.

Frankly, I just copied the logic already present in config/meson.build.
If there is better to do, please advise.


>
> >  libexecinfo = cc.find_library('libexecinfo', required: false)
> >  if libexecinfo.found() and cc.has_header('execinfo.h')
> >      add_project_link_arguments('-lexecinfo', language: 'c')
> [...]
> > diff --git a/lib/eal/version.map b/lib/eal/version.map
> > index fe5c3dac98..020c058e5f 100644
> > --- a/lib/eal/version.map
> > +++ b/lib/eal/version.map
> > @@ -423,11 +423,13 @@ EXPERIMENTAL {
> >       rte_version_release; # WINDOWS_NO_EXPORT
> >       rte_version_suffix; # WINDOWS_NO_EXPORT
> >       rte_version_year; # WINDOWS_NO_EXPORT
> > +
> >  };
>
> Unnecessary empty line.

Yep, already fixed in my local branch.
Thanks.
  
David Marchand June 3, 2021, 7:53 a.m. UTC | #6
On Thu, Jun 3, 2021 at 9:23 AM David Marchand <david.marchand@redhat.com> wrote:
> > > diff --git a/config/meson.build b/config/meson.build
> > > index 017bb2efbb..337daa2719 100644
> > > --- a/config/meson.build
> > > +++ b/config/meson.build
> > > @@ -166,6 +166,15 @@ if fdt_dep.found() and cc.has_header('fdt.h')
> > >      dpdk_extra_ldflags += '-lfdt'
> > >  endif
> > >
> > > +has_libarchive = 0
> > > +archive_dep = cc.find_library('libarchive', required: false)
> > > +if archive_dep.found() and cc.has_header('archive.h')
> > > +    dpdk_conf.set10('RTE_HAS_LIBARCHIVE', true)
> > > +    has_libarchive = 1
> > > +    add_project_link_arguments('-larchive', language: 'c')
> > > +    dpdk_extra_ldflags += '-larchive'
> > > +endif
> > > +
> >
> > Why not use pkg-config?
> > `has_libarchive` is unused.
>
> Frankly, I just copied the logic already present in config/meson.build.
> If there is better to do, please advise.

Ah ok, I think I understand.
Do you mean to align on libbsd?
  
Bruce Richardson June 3, 2021, 8:14 a.m. UTC | #7
On Thu, Jun 03, 2021 at 09:53:44AM +0200, David Marchand wrote:
> On Thu, Jun 3, 2021 at 9:23 AM David Marchand <david.marchand@redhat.com>
> wrote:
> > > > diff --git a/config/meson.build b/config/meson.build index
> > > > 017bb2efbb..337daa2719 100644 --- a/config/meson.build +++
> > > > b/config/meson.build @@ -166,6 +166,15 @@ if fdt_dep.found() and
> > > > cc.has_header('fdt.h') dpdk_extra_ldflags += '-lfdt' endif
> > > >
> > > > +has_libarchive = 0 +archive_dep = cc.find_library('libarchive',
> > > > required: false) +if archive_dep.found() and
> > > > cc.has_header('archive.h') +
> > > > dpdk_conf.set10('RTE_HAS_LIBARCHIVE', true) +    has_libarchive = 1
> > > > +    add_project_link_arguments('-larchive', language: 'c') +
> > > > dpdk_extra_ldflags += '-larchive' +endif +
> > >
> > > Why not use pkg-config?  `has_libarchive` is unused.
> >
> > Frankly, I just copied the logic already present in config/meson.build.
> > If there is better to do, please advise.
> 
> Ah ok, I think I understand.  Do you mean to align on libbsd?
> 
Yep. "dependency()" should be used for libs/packages that have a pkg-config
file, while "find_library()" is a fallback for literally just trying to
find a library that doesn't have a .pc file.


When specifying dependencies, it's best to explicitly state the lookup
method as pkg-config (only), because if not found via pkg-config, meson
will by default then attempt to use cmake (if present) to find the
requested package, and that can cause issues with cmake finding the
incorrect package version e.g. when doing 32-bit builds on 64-bit systems.
There is almost certainly a way to configure cmake not to do this, but by
limiting search to pkg-config it saves us having to document how to
configure proper library search paths for multiple tools. Setting
PKG_CONFIG_LIBDIR should be enough!

/Bruce
  

Patch

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 7c4d6dcdbf..7dac20ddeb 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -93,6 +93,7 @@  jobs:
       run: sudo apt install -y ccache libnuma-dev python3-setuptools
         python3-wheel python3-pip python3-pyelftools ninja-build libbsd-dev
         libpcap-dev libibverbs-dev libcrypto++-dev libfdt-dev libjansson-dev
+        libarchive-dev
     - name: Install libabigail build dependencies if no cache is available
       if: env.ABI_CHECKS == 'true' && steps.libabigail-cache.outputs.cache-hit != 'true'
       run: sudo apt install -y autoconf automake libtool pkg-config libxml2-dev
diff --git a/.travis.yml b/.travis.yml
index 5b702cc9bb..23067d9e3c 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -16,6 +16,7 @@  addons:
     packages: &required_packages
       - [libnuma-dev, python3-setuptools, python3-wheel, python3-pip, python3-pyelftools, ninja-build]
       - [libbsd-dev, libpcap-dev, libibverbs-dev, libcrypto++-dev, libfdt-dev, libjansson-dev]
+      - [libarchive-dev]
 
 _aarch64_packages: &aarch64_packages
   - *required_packages
diff --git a/config/meson.build b/config/meson.build
index 017bb2efbb..337daa2719 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -166,6 +166,15 @@  if fdt_dep.found() and cc.has_header('fdt.h')
     dpdk_extra_ldflags += '-lfdt'
 endif
 
+has_libarchive = 0
+archive_dep = cc.find_library('libarchive', required: false)
+if archive_dep.found() and cc.has_header('archive.h')
+    dpdk_conf.set10('RTE_HAS_LIBARCHIVE', true)
+    has_libarchive = 1
+    add_project_link_arguments('-larchive', language: 'c')
+    dpdk_extra_ldflags += '-larchive'
+endif
+
 libexecinfo = cc.find_library('libexecinfo', required: false)
 if libexecinfo.found() and cc.has_header('execinfo.h')
     add_project_link_arguments('-lexecinfo', language: 'c')
diff --git a/drivers/net/bnx2x/bnx2x.c b/drivers/net/bnx2x/bnx2x.c
index 654878d9de..60292753e2 100644
--- a/drivers/net/bnx2x/bnx2x.c
+++ b/drivers/net/bnx2x/bnx2x.c
@@ -26,7 +26,9 @@ 
 #include <arpa/inet.h>
 #include <fcntl.h>
 #include <zlib.h>
+
 #include <rte_bitops.h>
+#include <rte_firmware.h>
 #include <rte_string_fns.h>
 
 #define BNX2X_PMD_VER_PREFIX "BNX2X PMD"
@@ -9655,44 +9657,33 @@  static void bnx2x_init_rte(struct bnx2x_softc *sc)
 void bnx2x_load_firmware(struct bnx2x_softc *sc)
 {
 	const char *fwname;
-	int f;
-	struct stat st;
+	void *buf;
+	size_t bufsz;
 
 	fwname = sc->devinfo.device_id == CHIP_NUM_57711
 		? FW_NAME_57711 : FW_NAME_57810;
-	f = open(fwname, O_RDONLY);
-	if (f < 0) {
+	if (rte_firmware_read(fwname, &buf, &bufsz) != 0) {
 		PMD_DRV_LOG(NOTICE, sc, "Can't open firmware file");
 		return;
 	}
 
-	if (fstat(f, &st) < 0) {
-		PMD_DRV_LOG(NOTICE, sc, "Can't stat firmware file");
-		close(f);
-		return;
-	}
-
-	sc->firmware = rte_zmalloc("bnx2x_fw", st.st_size, RTE_CACHE_LINE_SIZE);
+	sc->firmware = rte_zmalloc("bnx2x_fw", bufsz, RTE_CACHE_LINE_SIZE);
 	if (!sc->firmware) {
 		PMD_DRV_LOG(NOTICE, sc, "Can't allocate memory for firmware");
-		close(f);
-		return;
+		goto out;
 	}
 
-	if (read(f, sc->firmware, st.st_size) != st.st_size) {
-		PMD_DRV_LOG(NOTICE, sc, "Can't read firmware data");
-		close(f);
-		return;
-	}
-	close(f);
-
-	sc->fw_len = st.st_size;
+	sc->fw_len = bufsz;
 	if (sc->fw_len < FW_HEADER_LEN) {
 		PMD_DRV_LOG(NOTICE, sc,
 			    "Invalid fw size: %" PRIu64, sc->fw_len);
-		return;
+		goto out;
 	}
+
+	memcpy(sc->firmware, buf, sc->fw_len);
 	PMD_DRV_LOG(DEBUG, sc, "fw_len = %" PRIu64, sc->fw_len);
+out:
+	free(buf);
 }
 
 static void
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 05ccc41d95..6bb8c76ab3 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -10,6 +10,7 @@ 
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include <rte_firmware.h>
 #include <rte_tailq.h>
 
 #include "base/ice_sched.h"
@@ -1673,22 +1674,14 @@  ice_load_pkg_type(struct ice_hw *hw)
 	return package_type;
 }
 
-#ifdef RTE_EXEC_ENV_WINDOWS
-#define ice_access _access
-#else
-#define ice_access access
-#endif
-
 int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 {
 	struct ice_hw *hw = &adapter->hw;
 	char pkg_file[ICE_MAX_PKG_FILENAME_SIZE];
 	char opt_ddp_filename[ICE_MAX_PKG_FILENAME_SIZE];
+	void *buf;
+	size_t bufsz;
 	int err;
-	uint8_t *buf = NULL;
-	int buf_len;
-	FILE *file;
-	struct stat fstat;
 
 	if (!use_dsn)
 		goto no_dsn;
@@ -1698,57 +1691,31 @@  int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 		"ice-%016" PRIx64 ".pkg", dsn);
 	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
 		ICE_MAX_PKG_FILENAME_SIZE);
-	if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
+	strcat(pkg_file, opt_ddp_filename);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
 		goto load_fw;
 
 	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_DEFAULT,
 		ICE_MAX_PKG_FILENAME_SIZE);
-	if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
+	strcat(pkg_file, opt_ddp_filename);
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
 		goto load_fw;
 
 no_dsn:
 	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
-	if (!ice_access(pkg_file, 0))
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
 		goto load_fw;
+
 	strncpy(pkg_file, ICE_PKG_FILE_DEFAULT, ICE_MAX_PKG_FILENAME_SIZE);
-	if (ice_access(pkg_file, 0)) {
+	if (rte_firmware_read(pkg_file, &buf, &bufsz) < 0) {
 		PMD_INIT_LOG(ERR, "failed to search file path\n");
 		return -1;
 	}
 
 load_fw:
-	file = fopen(pkg_file, "rb");
-	if (!file)  {
-		PMD_INIT_LOG(ERR, "failed to open file: %s\n", pkg_file);
-		return -1;
-	}
-
 	PMD_INIT_LOG(DEBUG, "DDP package name: %s", pkg_file);
 
-	err = stat(pkg_file, &fstat);
-	if (err) {
-		PMD_INIT_LOG(ERR, "failed to get file stats\n");
-		goto out;
-	}
-
-	buf_len = fstat.st_size;
-	buf = rte_malloc(NULL, buf_len, 0);
-
-	if (!buf) {
-		PMD_INIT_LOG(ERR, "failed to allocate buf of size %d for package\n",
-				buf_len);
-		err = -1;
-		goto out;
-	}
-
-	err = fread(buf, buf_len, 1, file);
-	if (err != 1) {
-		PMD_INIT_LOG(ERR, "failed to read package data\n");
-		err = -1;
-		goto out;
-	}
-
-	err = ice_copy_and_init_pkg(hw, buf, buf_len);
+	err = ice_copy_and_init_pkg(hw, buf, bufsz);
 	if (err) {
 		PMD_INIT_LOG(ERR, "ice_copy_and_init_hw failed: %d\n", err);
 		goto out;
@@ -1758,13 +1725,10 @@  int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
 	adapter->active_pkg_type = ice_load_pkg_type(hw);
 
 out:
-	fclose(file);
-	rte_free(buf);
+	free(buf);
 	return err;
 }
 
-#undef ice_access
-
 static void
 ice_base_queue_get(struct ice_pf *pf)
 {
diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 2ee88fbfc7..879da7ef59 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -29,6 +29,7 @@ 
 #include <rte_alarm.h>
 #include <rte_spinlock.h>
 #include <rte_service_component.h>
+#include <rte_firmware.h>
 
 #include "nfpcore/nfp_cpp.h"
 #include "nfpcore/nfp_nffw.h"
@@ -3366,12 +3367,10 @@  static int
 nfp_fw_upload(struct rte_pci_device *dev, struct nfp_nsp *nsp, char *card)
 {
 	struct nfp_cpp *cpp = nsp->cpp;
-	int fw_f;
-	char *fw_buf;
+	void *fw_buf;
 	char fw_name[125];
 	char serial[40];
-	struct stat file_stat;
-	off_t fsize, bytes;
+	size_t fsize;
 
 	/* Looking for firmware file in order of priority */
 
@@ -3384,66 +3383,34 @@  nfp_fw_upload(struct rte_pci_device *dev, struct nfp_nsp *nsp, char *card)
 
 	snprintf(fw_name, sizeof(fw_name), "%s/%s.nffw", DEFAULT_FW_PATH,
 			serial);
-
 	PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
-	fw_f = open(fw_name, O_RDONLY);
-	if (fw_f >= 0)
-		goto read_fw;
+	if (rte_firmware_read(fw_name, &fw_buf, &fsize) == 0)
+		goto load_fw;
 
 	/* Then try the PCI name */
 	snprintf(fw_name, sizeof(fw_name), "%s/pci-%s.nffw", DEFAULT_FW_PATH,
 			dev->device.name);
-
 	PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
-	fw_f = open(fw_name, O_RDONLY);
-	if (fw_f >= 0)
-		goto read_fw;
+	if (rte_firmware_read(fw_name, &fw_buf, &fsize) == 0)
+		goto load_fw;
 
 	/* Finally try the card type and media */
 	snprintf(fw_name, sizeof(fw_name), "%s/%s", DEFAULT_FW_PATH, card);
 	PMD_DRV_LOG(DEBUG, "Trying with fw file: %s", fw_name);
-	fw_f = open(fw_name, O_RDONLY);
-	if (fw_f < 0) {
+	if (rte_firmware_read(fw_name, &fw_buf, &fsize) < 0) {
 		PMD_DRV_LOG(INFO, "Firmware file %s not found.", fw_name);
 		return -ENOENT;
 	}
 
-read_fw:
-	if (fstat(fw_f, &file_stat) < 0) {
-		PMD_DRV_LOG(INFO, "Firmware file %s size is unknown", fw_name);
-		close(fw_f);
-		return -ENOENT;
-	}
-
-	fsize = file_stat.st_size;
-	PMD_DRV_LOG(INFO, "Firmware file found at %s with size: %" PRIu64 "",
-			    fw_name, (uint64_t)fsize);
-
-	fw_buf = malloc((size_t)fsize);
-	if (!fw_buf) {
-		PMD_DRV_LOG(INFO, "malloc failed for fw buffer");
-		close(fw_f);
-		return -ENOMEM;
-	}
-	memset(fw_buf, 0, fsize);
-
-	bytes = read(fw_f, fw_buf, fsize);
-	if (bytes != fsize) {
-		PMD_DRV_LOG(INFO, "Reading fw to buffer failed."
-				   "Just %" PRIu64 " of %" PRIu64 " bytes read",
-				   (uint64_t)bytes, (uint64_t)fsize);
-		free(fw_buf);
-		close(fw_f);
-		return -EIO;
-	}
+load_fw:
+	PMD_DRV_LOG(INFO, "Firmware file found at %s with size: %zu",
+		fw_name, fsize);
 
 	PMD_DRV_LOG(INFO, "Uploading the firmware ...");
-	nfp_nsp_load_fw(nsp, fw_buf, bytes);
+	nfp_nsp_load_fw(nsp, fw_buf, fsize);
 	PMD_DRV_LOG(INFO, "Done");
 
 	free(fw_buf);
-	close(fw_f);
-
 	return 0;
 }
 
diff --git a/drivers/net/qede/qede_main.c b/drivers/net/qede/qede_main.c
index caa9d1d4f6..347e407b1b 100644
--- a/drivers/net/qede/qede_main.c
+++ b/drivers/net/qede/qede_main.c
@@ -6,6 +6,7 @@ 
 
 #include <limits.h>
 #include <rte_alarm.h>
+#include <rte_firmware.h>
 #include <rte_string_fns.h>
 
 #include "qede_ethdev.h"
@@ -127,51 +128,40 @@  static void qed_free_stream_mem(struct ecore_dev *edev)
 #ifdef CONFIG_ECORE_BINARY_FW
 static int qed_load_firmware_data(struct ecore_dev *edev)
 {
-	int fd;
-	struct stat st;
 	const char *fw = RTE_LIBRTE_QEDE_FW;
+	void *buf;
+	size_t bufsz;
+	int ret;
 
 	if (strcmp(fw, "") == 0)
 		strcpy(qede_fw_file, QEDE_DEFAULT_FIRMWARE);
 	else
 		strcpy(qede_fw_file, fw);
 
-	fd = open(qede_fw_file, O_RDONLY);
-	if (fd < 0) {
-		DP_ERR(edev, "Can't open firmware file\n");
-		return -ENOENT;
-	}
-
-	if (fstat(fd, &st) < 0) {
-		DP_ERR(edev, "Can't stat firmware file\n");
-		close(fd);
+	if (rte_firmware_read(qede_fw_file, &buf, &bufsz) < 0) {
+		DP_ERR(edev, "Can't read firmware data: %s\n", qede_fw_file);
 		return -1;
 	}
 
-	edev->firmware = rte_zmalloc("qede_fw", st.st_size,
-				    RTE_CACHE_LINE_SIZE);
+	edev->firmware = rte_zmalloc("qede_fw", bufsz, RTE_CACHE_LINE_SIZE);
 	if (!edev->firmware) {
 		DP_ERR(edev, "Can't allocate memory for firmware\n");
-		close(fd);
-		return -ENOMEM;
-	}
-
-	if (read(fd, edev->firmware, st.st_size) != st.st_size) {
-		DP_ERR(edev, "Can't read firmware data\n");
-		close(fd);
-		return -1;
+		ret = -ENOMEM;
+		goto out;
 	}
 
-	edev->fw_len = st.st_size;
+	memcpy(edev->firmware, buf, bufsz);
+	edev->fw_len = bufsz;
 	if (edev->fw_len < 104) {
 		DP_ERR(edev, "Invalid fw size: %" PRIu64 "\n",
 			  edev->fw_len);
-		close(fd);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
-
-	close(fd);
-	return 0;
+	ret = 0;
+out:
+	free(buf);
+	return ret;
 }
 #endif
 
diff --git a/lib/eal/include/rte_firmware.h b/lib/eal/include/rte_firmware.h
new file mode 100644
index 0000000000..d2a7bc8083
--- /dev/null
+++ b/lib/eal/include/rte_firmware.h
@@ -0,0 +1,35 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2021 Red Hat, Inc.
+ */
+
+#ifndef __RTE_FIRMWARE_H__
+#define __RTE_FIRMWARE_H__
+
+#include <sys/types.h>
+
+#include <rte_compat.h>
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Load a firmware in a dynamically allocated buffer, dealing with compressed
+ * files if libarchive is available.
+ *
+ * @param name
+ *      Firmware filename to load.
+ * @param buf
+ *      Buffer allocated by this function. If this function succeeds, the
+ *      caller is responsible for freeing the buffer.
+ * @param bufsz
+ *      Size of the data in the buffer.
+ *
+ * @return
+ *      0 if successful.
+ *      Negative otherwise, buf and bufsize contents are invalid.
+ */
+__rte_internal
+int
+rte_firmware_read(const char *name, void **buf, size_t *bufsz);
+
+#endif
diff --git a/lib/eal/unix/eal_firmware.c b/lib/eal/unix/eal_firmware.c
new file mode 100644
index 0000000000..ea66fecfe9
--- /dev/null
+++ b/lib/eal/unix/eal_firmware.c
@@ -0,0 +1,106 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2021 Red Hat, Inc.
+ */
+
+#ifdef RTE_HAS_LIBARCHIVE
+#include <archive.h>
+#endif
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <rte_common.h>
+#include <rte_firmware.h>
+#include <rte_log.h>
+
+static int
+firmware_read(const char *name, void **buf, size_t *bufsz)
+{
+	const size_t blocksize = 4096;
+	int ret = -1;
+	int err;
+#ifdef RTE_HAS_LIBARCHIVE
+	struct archive_entry *entry;
+	struct archive *a;
+#else
+	int fd;
+#endif
+
+	*buf = NULL;
+	*bufsz = 0;
+
+#ifdef RTE_HAS_LIBARCHIVE
+	a = archive_read_new();
+	if (a == NULL || archive_read_support_format_raw(a) != ARCHIVE_OK ||
+			archive_read_support_filter_xz(a) != ARCHIVE_OK ||
+			archive_read_open_filename(a, name, blocksize) != ARCHIVE_OK ||
+			archive_read_next_header(a, &entry) != ARCHIVE_OK)
+		goto out;
+#else
+	fd = open(name, O_RDONLY);
+	if (fd < 0)
+		goto out;
+#endif
+
+	do {
+		void *tmp;
+
+		tmp = realloc(*buf, *bufsz + blocksize);
+		if (tmp == NULL) {
+			free(*buf);
+			*buf = NULL;
+			*bufsz = 0;
+			break;
+		}
+		*buf = tmp;
+
+#ifdef RTE_HAS_LIBARCHIVE
+		err = archive_read_data(a, RTE_PTR_ADD(*buf, *bufsz), blocksize);
+#else
+		err = read(fd, RTE_PTR_ADD(*buf, *bufsz), blocksize);
+#endif
+		if (err < 0) {
+			free(*buf);
+			*buf = NULL;
+			*bufsz = 0;
+			break;
+		}
+		*bufsz += err;
+
+	} while (err != 0);
+
+	if (*buf != NULL)
+		ret = 0;
+out:
+#ifdef RTE_HAS_LIBARCHIVE
+	if (a != NULL)
+		archive_read_free(a);
+#else
+	if (fd >= 0)
+		close(fd);
+#endif
+	return ret;
+}
+
+int
+rte_firmware_read(const char *name, void **buf, size_t *bufsz)
+{
+	char path[PATH_MAX];
+	int ret;
+
+	ret = firmware_read(name, buf, bufsz);
+	if (ret < 0) {
+		snprintf(path, sizeof(path), "%s.xz", name);
+		path[PATH_MAX - 1] = '\0';
+#ifndef RTE_HAS_LIBARCHIVE
+		if (access(path, F_OK) == 0) {
+			RTE_LOG(WARNING, EAL, "libarchive not available, %s cannot be decompressed\n",
+				path);
+		}
+#else
+		ret = firmware_read(path, buf, bufsz);
+#endif
+	}
+	return ret;
+}
diff --git a/lib/eal/unix/meson.build b/lib/eal/unix/meson.build
index dc711b4240..e3ecd3e956 100644
--- a/lib/eal/unix/meson.build
+++ b/lib/eal/unix/meson.build
@@ -5,5 +5,6 @@  sources += files(
         'eal_file.c',
         'eal_unix_memory.c',
         'eal_unix_timer.c',
+        'eal_firmware.c',
         'rte_thread.c',
 )
diff --git a/lib/eal/version.map b/lib/eal/version.map
index fe5c3dac98..020c058e5f 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -423,11 +423,13 @@  EXPERIMENTAL {
 	rte_version_release; # WINDOWS_NO_EXPORT
 	rte_version_suffix; # WINDOWS_NO_EXPORT
 	rte_version_year; # WINDOWS_NO_EXPORT
+
 };
 
 INTERNAL {
 	global:
 
+	rte_firmware_read;
 	rte_mem_lock;
 	rte_mem_map;
 	rte_mem_page_size;
diff --git a/lib/eal/windows/eal.c b/lib/eal/windows/eal.c
index 28c787c0b0..938ace92f5 100644
--- a/lib/eal/windows/eal.c
+++ b/lib/eal/windows/eal.c
@@ -21,6 +21,7 @@ 
 #include <eal_private.h>
 #include <rte_service_component.h>
 #include <rte_vfio.h>
+#include <rte_firmware.h>
 
 #include "eal_hugepages.h"
 #include "eal_trace.h"
@@ -463,3 +464,11 @@  rte_vfio_container_dma_unmap(__rte_unused int container_fd,
 {
 	return -1;
 }
+
+int
+rte_firmware_read(__rte_unused const char *name,
+			__rte_unused void **buf,
+			__rte_unused size_t *bufsz)
+{
+	return -1;
+}