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

[David Marchand]

Hello,

On Fri, Sep 22, 2023 at 5:59 AM Srikanth Yalavarthi
<syalavarthi at marvell.com> wrote:
> > From: David Marchand <david.marchand at redhat.com>
> > On Thu, Sep 21, 2023 at 3:06 PM Srikanth Yalavarthi
> > <syalavarthi at 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/


-- 
David Marchand