[v1,02/34] ml/cnxk: drop use of RTE API for firmware read

Message ID 20230830155927.3566-3-syalavarthi@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Implemenation of revised ml/cnxk driver |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Srikanth Yalavarthi Aug. 30, 2023, 3:58 p.m. UTC
  Dropped use of rte_firmware_read API to read ML firmware
binary. When DPDK is built with libarchive aaupport, the
the RTE API assumes the binary file as a compressed
archive. This causes the ML firmware binary to be parsed
incorrectly.

Fixes: c29da752ffa8 ("ml/cnxk: support firmware load and device reset")
Cc: syalavarthi@marvell.com

Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
---
 drivers/ml/cnxk/cn10k_ml_dev.c | 64 +++++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 4 deletions(-)
  

Comments

Jerin Jacob Sept. 21, 2023, 12:08 p.m. UTC | #1
On Wed, Aug 30, 2023 at 9:40 PM Srikanth Yalavarthi
<syalavarthi@marvell.com> wrote:
>
> Dropped use of rte_firmware_read API to read ML firmware
> binary. When DPDK is built with libarchive aaupport, the
> the RTE API assumes the binary file as a compressed
> archive. This causes the ML firmware binary to be parsed
> incorrectly.

+ @David Marchand  rte_firmware_read() author for his opinions


>
> Fixes: c29da752ffa8 ("ml/cnxk: support firmware load and device reset")
> Cc: syalavarthi@marvell.com
>
> Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
> ---
>  drivers/ml/cnxk/cn10k_ml_dev.c | 64 +++++++++++++++++++++++++++++++---
>  1 file changed, 60 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/ml/cnxk/cn10k_ml_dev.c b/drivers/ml/cnxk/cn10k_ml_dev.c
> index e3c2badcef5..b7e6ed9a00e 100644
> --- a/drivers/ml/cnxk/cn10k_ml_dev.c
> +++ b/drivers/ml/cnxk/cn10k_ml_dev.c
> @@ -2,6 +2,11 @@
>   * Copyright (c) 2022 Marvell.
>   */
>
> +#include <fcntl.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +
>  #include <rte_common.h>
>  #include <rte_dev.h>
>  #include <rte_devargs.h>
> @@ -61,6 +66,57 @@ static const int valid_ocm_page_size[] = {1024, 2048, 4096, 8192, 16384};
>  /* Dummy operations for ML device */
>  struct rte_ml_dev_ops ml_dev_dummy_ops = {0};
>
> +static int
> +ml_read_file(const char *file, size_t *size, char **buffer)
> +{
> +       char *file_buffer = NULL;
> +       struct stat file_stat;
> +       char *file_map;
> +       int ret;
> +       int fd;
> +
> +       fd = open(file, O_RDONLY);
> +       if (fd == -1) {
> +               plt_err("Failed to open file: %s\n", file);
> +               return -errno;
> +       }
> +
> +       if (fstat(fd, &file_stat) != 0) {
> +               plt_err("fstat failed for file: %s\n", file);
> +               close(fd);
> +               return -errno;
> +       }
> +
> +       file_buffer = rte_malloc("ml_firmware", file_stat.st_size, PLT_CACHE_LINE_SIZE);
> +       if (file_buffer == NULL) {
> +               plt_err("Failed to allocate memory: %s\n", file);
> +               ret = -ENOMEM;
> +               goto error;
> +       }
> +
> +       file_map = mmap(0, file_stat.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
> +       if (file_map == MAP_FAILED) {
> +               plt_err("Failed to map file: %s\n", file);
> +               ret = -errno;
> +               goto error;
> +       }
> +
> +       rte_memcpy(file_buffer, file_map, file_stat.st_size);
> +       munmap(file_map, file_stat.st_size);
> +       close(fd);
> +
> +       *size = file_stat.st_size;
> +       *buffer = file_buffer;
> +
> +       return 0;
> +
> +error:
> +       free(file_buffer);
> +       close(fd);
> +
> +       return ret;
> +}
> +
>  static int
>  parse_string_arg(const char *key __rte_unused, const char *value, void *extra_args)
>  {
> @@ -736,7 +792,7 @@ cn10k_ml_fw_load(struct cn10k_ml_dev *mldev)
>  {
>         const struct plt_memzone *mz;
>         struct cn10k_ml_fw *fw;
> -       void *fw_buffer = NULL;
> +       char *fw_buffer = NULL;
>         uint64_t mz_size = 0;
>         uint64_t fw_size = 0;
>         int ret = 0;
> @@ -746,7 +802,7 @@ cn10k_ml_fw_load(struct cn10k_ml_dev *mldev)
>
>         if (roc_env_is_emulator() || roc_env_is_hw()) {
>                 /* Read firmware image to a buffer */
> -               ret = rte_firmware_read(fw->path, &fw_buffer, &fw_size);
> +               ret = ml_read_file(fw->path, &fw_size, &fw_buffer);
>                 if ((ret < 0) || (fw_buffer == NULL)) {
>                         plt_err("Unable to read firmware data: %s\n", fw->path);
>                         return ret;
> @@ -763,7 +819,7 @@ cn10k_ml_fw_load(struct cn10k_ml_dev *mldev)
>         mz = plt_memzone_reserve_aligned(FW_MEMZONE_NAME, mz_size, 0, ML_CN10K_ALIGN_SIZE);
>         if (mz == NULL) {
>                 plt_err("plt_memzone_reserve failed : %s", FW_MEMZONE_NAME);
> -               free(fw_buffer);
> +               rte_free(fw_buffer);
>                 return -ENOMEM;
>         }
>         fw->req = mz->addr;
> @@ -780,7 +836,7 @@ cn10k_ml_fw_load(struct cn10k_ml_dev *mldev)
>         if (roc_env_is_emulator() || roc_env_is_hw()) {
>                 fw->data = PLT_PTR_ADD(mz->addr, sizeof(struct cn10k_ml_req));
>                 ret = cn10k_ml_fw_load_cn10ka(fw, fw_buffer, fw_size);
> -               free(fw_buffer);
> +               rte_free(fw_buffer);
>         } else if (roc_env_is_asim()) {
>                 fw->data = NULL;
>                 ret = cn10k_ml_fw_load_asim(fw);
> --
> 2.41.0
>
  
David Marchand Sept. 21, 2023, 12:52 p.m. UTC | #2
On Thu, Sep 21, 2023 at 2:08 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Wed, Aug 30, 2023 at 9:40 PM Srikanth Yalavarthi
> <syalavarthi@marvell.com> wrote:
> >
> > Dropped use of rte_firmware_read API to read ML firmware
> > binary. When DPDK is built with libarchive aaupport, the
> > the RTE API assumes the binary file as a compressed

The rte_firmware API supports both xz-compressed  and uncompressed files.
Otherwise, it would break loading net/ice on systems where
/lib/firmware content is uncompressed (which is still the case in some
Linux distributions).


To convince myself, I wrote a quick tool ("./archive" below) that
outputs in hexa the content of a file, with the same libarchive calls.

With a xz-compressed file:
$ hexdump -C /lib/firmware/intel/ice/ddp/ice.pkg.xz | head -1
00000000  fd 37 7a 58 5a 00 00 01  69 22 de 36 02 00 21 01  |.7zXZ...i".6..!.|
$ ./archive /lib/firmware/intel/ice/ddp/ice.pkg.xz | head -1
00000000: 01 00 00 00 05 00 00 00 1C 00 00 00 70 00 00 00 | ............p...

Uncompressing this file, and passing it to the same tool:
$ hexdump -C ice.pkg | head
00000000  01 00 00 00 05 00 00 00  1c 00 00 00 70 00 00 00  |............p...|
$ ./archive ice.pkg | head
00000000: 01 00 00 00 05 00 00 00 1C 00 00 00 70 00 00 00 | ............p...


For the record, I am using:
$ rpm -q libarchive
libarchive-3.6.1-3.fc37.x86_64


> > archive. This causes the ML firmware binary to be parsed
> > incorrectly.
>
> + @David Marchand  rte_firmware_read() author for his opinions

/lib/firmware/mlip-fw.bin does not seem to be something packaged in
Fedora, and I found no trace in linux-firmware repo, so I can't
reproduce your issue.

Please add some debug and give more details about the issue you are facing.
  
Srikanth Yalavarthi Sept. 21, 2023, 1:06 p.m. UTC | #3
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: 21 September 2023 18:23
> To: Jerin Jacob <jerinjacobk@gmail.com>; Srikanth Yalavarthi
> <syalavarthi@marvell.com>
> Cc: Prince Takkar <ptakkar@marvell.com>; dev@dpdk.org; Shivah Shankar
> Shankar Narayan Rao <sshankarnara@marvell.com>; Anup Prabhu
> <aprabhu@marvell.com>
> Subject: [EXT] Re: [PATCH v1 02/34] ml/cnxk: drop use of RTE API for
> firmware read
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Thu, Sep 21, 2023 at 2:08 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Wed, Aug 30, 2023 at 9:40 PM Srikanth Yalavarthi
> > <syalavarthi@marvell.com> wrote:
> > >
> > > Dropped use of rte_firmware_read API to read ML firmware binary.
> > > When DPDK is built with libarchive aaupport, the the RTE API assumes
> > > the binary file as a compressed
> 
> The rte_firmware API supports both xz-compressed  and uncompressed files.
> Otherwise, it would break loading net/ice on systems where /lib/firmware
> content is uncompressed (which is still the case in some Linux distributions).
> 
> 
> To convince myself, I wrote a quick tool ("./archive" below) that outputs in
> hexa the content of a file, with the same libarchive calls.
> 
> With a xz-compressed file:
> $ hexdump -C /lib/firmware/intel/ice/ddp/ice.pkg.xz | head -1
> 00000000  fd 37 7a 58 5a 00 00 01  69 22 de 36 02 00 21 01  |.7zXZ...i".6..!.| $
> ./archive /lib/firmware/intel/ice/ddp/ice.pkg.xz | head -1
> 00000000: 01 00 00 00 05 00 00 00 1C 00 00 00 70 00 00 00 | ............p...
> 
> Uncompressing this file, and passing it to the same tool:
> $ hexdump -C ice.pkg | head
> 00000000  01 00 00 00 05 00 00 00  1c 00 00 00 70 00 00 00  |............p...| $
> ./archive ice.pkg | head
> 00000000: 01 00 00 00 05 00 00 00 1C 00 00 00 70 00 00 00 | ............p...
> 
> 
> For the record, I am using:
> $ rpm -q libarchive
> libarchive-3.6.1-3.fc37.x86_64
> 
> 
> > > archive. This causes the ML firmware binary to be parsed
> > > incorrectly.
> >
> > + @David Marchand  rte_firmware_read() author for his opinions
> 
> /lib/firmware/mlip-fw.bin does not seem to be something packaged in
> Fedora, and I found no trace in linux-firmware repo, so I can't reproduce
> your issue.
> 
> Please add some debug and give more details about the issue you are facing.

The "/lib/firmware/mlip-fw.bin" is Marvell's ML firmware binary. This file is in un-compressed form.

When DPDK is built without libarchive support, No issues are observed with using  rte_firmware_read to load the firmware file as open and read system calls are used.

When libarchive support is enabled, rte_firmware_read tries to parse the firmware binary as an xz archive. Since the file is not an archive, this step is failing.

Hence, added new ML driver function to read the firmware binary.

> 
> 
> --
> David Marchand
  
David Marchand Sept. 21, 2023, 1:26 p.m. UTC | #4
On Thu, Sep 21, 2023 at 3:06 PM Srikanth Yalavarthi
<syalavarthi@marvell.com> wrote:
> > > > archive. This causes the ML firmware binary to be parsed
> > > > incorrectly.
> > >
> > > + @David Marchand  rte_firmware_read() author for his opinions
> >
> > /lib/firmware/mlip-fw.bin does not seem to be something packaged in
> > Fedora, and I found no trace in linux-firmware repo, so I can't reproduce
> > your issue.
> >
> > Please add some debug and give more details about the issue you are facing.
>
> The "/lib/firmware/mlip-fw.bin" is Marvell's ML firmware binary. This file is in un-compressed form.
>
> When DPDK is built without libarchive support, No issues are observed with using  rte_firmware_read to load the firmware file as open and read system calls are used.
>
> When libarchive support is enabled, rte_firmware_read tries to parse the firmware binary as an xz archive. Since the file is not an archive, this step is failing.

Please debug this part and point at the exact place where it fails.

>
> Hence, added new ML driver function to read the firmware binary.

This is just avoiding the issue without understanding it...
  
Srikanth Yalavarthi Sept. 22, 2023, 3:59 a.m. UTC | #5
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: 21 September 2023 18:57
> To: Srikanth Yalavarthi <syalavarthi@marvell.com>
> Cc: Jerin Jacob <jerinjacobk@gmail.com>; Prince Takkar
> <ptakkar@marvell.com>; dev@dpdk.org; Shivah Shankar Shankar Narayan
> Rao <sshankarnara@marvell.com>; Anup Prabhu <aprabhu@marvell.com>
> Subject: Re: [EXT] Re: [PATCH v1 02/34] ml/cnxk: drop use of RTE API for
> firmware read
> 
> On Thu, Sep 21, 2023 at 3:06 PM Srikanth Yalavarthi
> <syalavarthi@marvell.com> wrote:
> > > > > archive. This causes the ML firmware binary to be parsed
> > > > > incorrectly.
> > > >
> > > > + @David Marchand  rte_firmware_read() author for his opinions
> > >
> > > /lib/firmware/mlip-fw.bin does not seem to be something packaged in
> > > Fedora, and I found no trace in linux-firmware repo, so I can't
> > > reproduce your issue.
> > >
> > > Please add some debug and give more details about the issue you are
> facing.
> >
> > The "/lib/firmware/mlip-fw.bin" is Marvell's ML firmware binary. This file is
> in un-compressed form.
> >
> > When DPDK is built without libarchive support, No issues are observed with
> using  rte_firmware_read to load the firmware file as open and read system
> calls are used.
> >
> > When libarchive support is enabled, rte_firmware_read tries to parse the
> firmware binary as an xz archive. Since the file is not an archive, this step is
> failing.
> 
> Please debug this part and point at the exact place where it fails.

When compiled with libarchive support, the code fails in firmware_open (lib/eal/unix/eal_firmware.c:24) function

	if (archive_read_support_format_raw(ctx->a) != ARCHIVE_OK ||
			archive_read_support_filter_xz(ctx->a) != ARCHIVE_OK ||
			archive_read_open_filename(ctx->a, name, blocksize) != ARCHIVE_OK ||
			archive_read_next_header(ctx->a, &e) != ARCHIVE_OK) {
		archive_read_free(ctx->a);
		ctx->a = NULL;
		return -1;
	}

I understand that all of the 4 checks in the if condition assume that the file is a compressed archive. i.e, they look for relevant metadata of a compressed archive.
All 4 checks were failing when the file being read is a single uncompressed file (as in our case).

And, when compiled without libarchive enabled, alternate firmware_open (lib/eal/unix/eal_firmware.c:63) is called, which works for the file that we are trying to read.

Please correct me if my understanding is not correct.

> 
> >
> > Hence, added new ML driver function to read the firmware binary.
> 
> This is just avoiding the issue without understanding it...
> 
> 
> --
> David Marchand
  
David Marchand Sept. 22, 2023, 8:07 a.m. UTC | #6
Hello,

On Fri, Sep 22, 2023 at 5:59 AM Srikanth Yalavarthi
<syalavarthi@marvell.com> wrote:
> > From: David Marchand <david.marchand@redhat.com>
> > On Thu, Sep 21, 2023 at 3:06 PM Srikanth Yalavarthi
> > <syalavarthi@marvell.com> wrote:
> > > > > > archive. This causes the ML firmware binary to be parsed
> > > > > > incorrectly.
> > > > >
> > > > > + @David Marchand  rte_firmware_read() author for his opinions
> > > >
> > > > /lib/firmware/mlip-fw.bin does not seem to be something packaged in
> > > > Fedora, and I found no trace in linux-firmware repo, so I can't
> > > > reproduce your issue.
> > > >
> > > > Please add some debug and give more details about the issue you are
> > facing.
> > >
> > > The "/lib/firmware/mlip-fw.bin" is Marvell's ML firmware binary. This file is
> > in un-compressed form.
> > >
> > > When DPDK is built without libarchive support, No issues are observed with
> > using  rte_firmware_read to load the firmware file as open and read system
> > calls are used.
> > >
> > > When libarchive support is enabled, rte_firmware_read tries to parse the
> > firmware binary as an xz archive. Since the file is not an archive, this step is
> > failing.
> >
> > Please debug this part and point at the exact place where it fails.
>
> When compiled with libarchive support, the code fails in firmware_open (lib/eal/unix/eal_firmware.c:24) function
>
>         if (archive_read_support_format_raw(ctx->a) != ARCHIVE_OK ||

"""
     archive_read_support_format_raw()
             The “raw” format handler allows libarchive to be used to
read arbitrary data.  It treats any data stream as an archive with a
single entry.  The pathname of this entry is “data”; all other entry
fields are unset.  This
             is not enabled by archive_read_support_format_all() in
order to avoid erroneous handling of damaged archives.
"""

Which means that this instance of libarchive accepts "raw" archive.

>                         archive_read_support_filter_xz(ctx->a) != ARCHIVE_OK ||

"""
     archive_read_support_filter_bzip2(),
archive_read_support_filter_compress(),
archive_read_support_filter_grzip(),
archive_read_support_filter_gzip(),
archive_read_support_filter_lrzip(),
archive_read_support_filter_lz4(),
             archive_read_support_filter_lzma(),
archive_read_support_filter_lzop(),
archive_read_support_filter_none(), archive_read_support_filter_rpm(),
archive_read_support_filter_uu(), archive_read_support_filter_xz(),
             archive_read_support_filter_zstd(),
             Enables auto-detection code and decompression support for
the specified compression.  These functions may fall back on external
programs if an appropriate library was not available at build time.
Decompression using an
             external program is usually slower than decompression
through built-in libraries.  Note that “none” is always enabled by
default.
"""

Which means that this instance of libarchive accepts xz compressed
files, and uncompressed files too because the "none" filter is enabled
by default.


>                         archive_read_open_filename(ctx->a, name, blocksize) != ARCHIVE_OK ||
>                         archive_read_next_header(ctx->a, &e) != ARCHIVE_OK) {
>                 archive_read_free(ctx->a);
>                 ctx->a = NULL;
>                 return -1;
>         }
>
> I understand that all of the 4 checks in the if condition assume that the file is a compressed archive. i.e, they look for relevant metadata of a compressed archive.

I had double checked before replying last time, it works as I
described with my fedora libarchive.


> All 4 checks were failing when the file being read is a single uncompressed file (as in our case).

o_O

Did you check that all 4 checks are failing individually or are you
saying this 4 tests fail as a whole?

I have one suspicion on archive_read_support_filter_xz, which may
return ARCHIVE_WARN.
But that's my only serious hint so far.

I have put up some debug patch, please have a try with it.
https://patchwork.dpdk.org/project/dpdk/patch/20230922080606.905222-1-david.marchand@redhat.com/
  
Srikanth Yalavarthi Sept. 22, 2023, 4:59 p.m. UTC | #7
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: 22 September 2023 13:38
> To: Srikanth Yalavarthi <syalavarthi@marvell.com>
> Cc: Jerin Jacob <jerinjacobk@gmail.com>; Prince Takkar
> <ptakkar@marvell.com>; dev@dpdk.org; Shivah Shankar Shankar Narayan
> Rao <sshankarnara@marvell.com>; Anup Prabhu <aprabhu@marvell.com>
> Subject: Re: [EXT] Re: [PATCH v1 02/34] ml/cnxk: drop use of RTE API for
> firmware read
> 
> Hello,
> 
> On Fri, Sep 22, 2023 at 5:59 AM Srikanth Yalavarthi
> <syalavarthi@marvell.com> wrote:
> > > From: David Marchand <david.marchand@redhat.com> On Thu, Sep 21,
> > > 2023 at 3:06 PM Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
> > > > > > > archive. This causes the ML firmware binary to be parsed
> > > > > > > incorrectly.
> > > > > >
> > > > > > + @David Marchand  rte_firmware_read() author for his opinions
> > > > >
> Did you check that all 4 checks are failing individually or are you saying this 4
> tests fail as a whole?
> 
> I have one suspicion on archive_read_support_filter_xz, which may return
> ARCHIVE_WARN.

Yes, archive_read_support_xz is returning ARCHIVE_WARN (-20). This is causing the firmware_open function to fail.

I guess we can ignore the ARCHIVE_WARN, since this means about compression support, not decompression.

""These functions return ARCHIVE_OK if the compression is fully supported, ARCHIVE_WARN if the compression is supported only through an external program.""

I have submitted a patch, which I have tested with compressed (xz) and uncompressed files. Please share your comments.

http://patches.dpdk.org/project/dpdk/patch/20230922165356.31567-1-syalavarthi@marvell.com/


> But that's my only serious hint so far.
> 
> I have put up some debug patch, please have a try with it.
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__patchwork.dpdk.org_project_dpdk_patch_20230922080606.905222-
> 2D1-2Ddavid.marchand-
> 40redhat.com_&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=SNPqUkGl0n_
> Ms1iJa_6wD6LBwX8efL_NOyXvAX-
> iCMI&m=gXmofsgJJekf5Jw2JIv1eESMDt3J_NyXqmHn9Gpk80XXWBsn7DBPjYsb
> eghxAWQr&s=iwNgfFNxL60sMbI-OP-k78p45eUPFaWTN1kUsO4nguQ&e=

Thanks for the debug patch and support.
> 
> 
> --
> David Marchand
  
David Marchand Sept. 27, 2023, 9:38 a.m. UTC | #8
On Thu, Sep 21, 2023 at 2:08 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Wed, Aug 30, 2023 at 9:40 PM Srikanth Yalavarthi
> <syalavarthi@marvell.com> wrote:
> >
> > Dropped use of rte_firmware_read API to read ML firmware
> > binary. When DPDK is built with libarchive aaupport, the
> > the RTE API assumes the binary file as a compressed
> > archive. This causes the ML firmware binary to be parsed
> > incorrectly.
>
> + @David Marchand  rte_firmware_read() author for his opinions

I am not sure if this series was applied, but this patch can be
discarded as a fix on rte_firmware_read has been merged in the main
repository.
Thanks for the heads up Jerin.
  
Srikanth Yalavarthi Sept. 27, 2023, 10 a.m. UTC | #9
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: 27 September 2023 15:08
> To: Jerin Jacob <jerinjacobk@gmail.com>
> Cc: Srikanth Yalavarthi <syalavarthi@marvell.com>; Prince Takkar
> <ptakkar@marvell.com>; dev@dpdk.org; Shivah Shankar Shankar Narayan
> Rao <sshankarnara@marvell.com>; Anup Prabhu <aprabhu@marvell.com>
> Subject: [EXT] Re: [PATCH v1 02/34] ml/cnxk: drop use of RTE API for
> firmware read
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Thu, Sep 21, 2023 at 2:08 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Wed, Aug 30, 2023 at 9:40 PM Srikanth Yalavarthi
> > <syalavarthi@marvell.com> wrote:
> > >
> > > Dropped use of rte_firmware_read API to read ML firmware binary.
> > > When DPDK is built with libarchive aaupport, the the RTE API assumes
> > > the binary file as a compressed archive. This causes the ML firmware
> > > binary to be parsed incorrectly.
> >
> > + @David Marchand  rte_firmware_read() author for his opinions
> 
> I am not sure if this series was applied, but this patch can be discarded as a fix
> on rte_firmware_read has been merged in the main repository.
> Thanks for the heads up Jerin.

This series is not yet applied.
I will push a revised series with this commit removed.

> 
> 
> --
> David Marchand
  
Srikanth Yalavarthi Sept. 27, 2023, 6:37 p.m. UTC | #10
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: 21 September 2023 17:38
> To: Srikanth Yalavarthi <syalavarthi@marvell.com>; David Marchand
> <david.marchand@redhat.com>
> Cc: Prince Takkar <ptakkar@marvell.com>; dev@dpdk.org; Shivah Shankar
> Shankar Narayan Rao <sshankarnara@marvell.com>; Anup Prabhu
> <aprabhu@marvell.com>; Srikanth Yalavarthi <syalavarthi@marvell.com>
> Subject: [EXT] Re: [PATCH v1 02/34] ml/cnxk: drop use of RTE API for
> firmware read
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Wed, Aug 30, 2023 at 9:40 PM Srikanth Yalavarthi
> <syalavarthi@marvell.com> wrote:
> >
> > Dropped use of rte_firmware_read API to read ML firmware binary. When
> > DPDK is built with libarchive aaupport, the the RTE API assumes the
> > binary file as a compressed archive. This causes the ML firmware
> > binary to be parsed incorrectly.
> 
> + @David Marchand  rte_firmware_read() author for his opinions
> 

Dropped this patch in v3 series. Required fix is implemented as part of
http://patches.dpdk.org/project/dpdk/patch/20230926144454.13419-1-syalavarthi@marvell.com/
  

Patch

diff --git a/drivers/ml/cnxk/cn10k_ml_dev.c b/drivers/ml/cnxk/cn10k_ml_dev.c
index e3c2badcef5..b7e6ed9a00e 100644
--- a/drivers/ml/cnxk/cn10k_ml_dev.c
+++ b/drivers/ml/cnxk/cn10k_ml_dev.c
@@ -2,6 +2,11 @@ 
  * Copyright (c) 2022 Marvell.
  */
 
+#include <fcntl.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
 #include <rte_common.h>
 #include <rte_dev.h>
 #include <rte_devargs.h>
@@ -61,6 +66,57 @@  static const int valid_ocm_page_size[] = {1024, 2048, 4096, 8192, 16384};
 /* Dummy operations for ML device */
 struct rte_ml_dev_ops ml_dev_dummy_ops = {0};
 
+static int
+ml_read_file(const char *file, size_t *size, char **buffer)
+{
+	char *file_buffer = NULL;
+	struct stat file_stat;
+	char *file_map;
+	int ret;
+	int fd;
+
+	fd = open(file, O_RDONLY);
+	if (fd == -1) {
+		plt_err("Failed to open file: %s\n", file);
+		return -errno;
+	}
+
+	if (fstat(fd, &file_stat) != 0) {
+		plt_err("fstat failed for file: %s\n", file);
+		close(fd);
+		return -errno;
+	}
+
+	file_buffer = rte_malloc("ml_firmware", file_stat.st_size, PLT_CACHE_LINE_SIZE);
+	if (file_buffer == NULL) {
+		plt_err("Failed to allocate memory: %s\n", file);
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	file_map = mmap(0, file_stat.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (file_map == MAP_FAILED) {
+		plt_err("Failed to map file: %s\n", file);
+		ret = -errno;
+		goto error;
+	}
+
+	rte_memcpy(file_buffer, file_map, file_stat.st_size);
+	munmap(file_map, file_stat.st_size);
+	close(fd);
+
+	*size = file_stat.st_size;
+	*buffer = file_buffer;
+
+	return 0;
+
+error:
+	free(file_buffer);
+	close(fd);
+
+	return ret;
+}
+
 static int
 parse_string_arg(const char *key __rte_unused, const char *value, void *extra_args)
 {
@@ -736,7 +792,7 @@  cn10k_ml_fw_load(struct cn10k_ml_dev *mldev)
 {
 	const struct plt_memzone *mz;
 	struct cn10k_ml_fw *fw;
-	void *fw_buffer = NULL;
+	char *fw_buffer = NULL;
 	uint64_t mz_size = 0;
 	uint64_t fw_size = 0;
 	int ret = 0;
@@ -746,7 +802,7 @@  cn10k_ml_fw_load(struct cn10k_ml_dev *mldev)
 
 	if (roc_env_is_emulator() || roc_env_is_hw()) {
 		/* Read firmware image to a buffer */
-		ret = rte_firmware_read(fw->path, &fw_buffer, &fw_size);
+		ret = ml_read_file(fw->path, &fw_size, &fw_buffer);
 		if ((ret < 0) || (fw_buffer == NULL)) {
 			plt_err("Unable to read firmware data: %s\n", fw->path);
 			return ret;
@@ -763,7 +819,7 @@  cn10k_ml_fw_load(struct cn10k_ml_dev *mldev)
 	mz = plt_memzone_reserve_aligned(FW_MEMZONE_NAME, mz_size, 0, ML_CN10K_ALIGN_SIZE);
 	if (mz == NULL) {
 		plt_err("plt_memzone_reserve failed : %s", FW_MEMZONE_NAME);
-		free(fw_buffer);
+		rte_free(fw_buffer);
 		return -ENOMEM;
 	}
 	fw->req = mz->addr;
@@ -780,7 +836,7 @@  cn10k_ml_fw_load(struct cn10k_ml_dev *mldev)
 	if (roc_env_is_emulator() || roc_env_is_hw()) {
 		fw->data = PLT_PTR_ADD(mz->addr, sizeof(struct cn10k_ml_req));
 		ret = cn10k_ml_fw_load_cn10ka(fw, fw_buffer, fw_size);
-		free(fw_buffer);
+		rte_free(fw_buffer);
 	} else if (roc_env_is_asim()) {
 		fw->data = NULL;
 		ret = cn10k_ml_fw_load_asim(fw);