Update ena-com HAL to newest version and update driver accordingly

[mk_semihalf.com]

Summary

The newest ena-com HAL supports LLQ and introduces few API changes.
The driver had to be updated to follow up those changes.

List of changes:

• Change version of the driver to 0.8.0
• Update ENA Makefile to make usage of LinuxKPI which is now used in ena-com platform code
• Add reset reasons when triggering reset of the device
• Reset device after attach fails
• In the reset task. free management irq after calling ena_down. Admin queue can still be used before ena_down is called, or when it is being handled.
• Do not reset device if ena_reset_task fails
• Move call of the ena_com_dev_reset to the ena_down() routine - it should be called only if interface was up
• Use different function for checking empty space on the sq ring (ena-com API change)
• Fix typo on ENA_TX_CLEANUP_THRESHOLD
• Change checking for EPERM with EOPNOTSUPP - change in the ena-com API

[rlibby]

I'm not familiar enough with this code to give it a proper review, but I suppose I am on the list for gcc compatibility? I checked and a GENERIC kernel does build with this patch applied to head r322969 and

make CROSS_TOOLCHAIN=amd64-gcc TARGET=amd64 TARGET_ARCH=amd64 buildkernel

with the amd64-xtoolchain-gcc-0.2 pkg installed. It produces the following warnings, but they do not appear to be new (KERNFAST log here):

$ grep warning gcc-buildkernel-ena.log 
/usr/src/freebsd-branches/D12134/sys/dev/ena/ena.c:1378:28: warning: comparison between 'enum ena_eth_io_l3_proto_index' and 'enum ena_eth_io_l4_proto_index' [-Wenum-compare]
/usr/src/freebsd-branches/D12134/sys/dev/ena/ena.c:1434:22: warning: variable 'adapter' set but not used [-Wunused-but-set-variable]
/usr/src/freebsd-branches/D12134/sys/dev/ena/ena.c:2677:11: warning: variable 'push_len' set but not used [-Wunused-but-set-variable]
/usr/src/freebsd-branches/D12134/sys/dev/ena/ena.c:2670:22: warning: variable 'ena_dev' set but not used [-Wunused-but-set-variable]

That said I did notice a couple of other issues:

• ena_efa_com.c does not appear to be hooked up to the build at all. It doesn't look like it would build if it were (it seems to include missing headers).
• This patch sets the executable bit on the file permissions for several headers.

sys/contrib/ena-com/ena_efa_com.c

34–36 ↗

(On Diff #32441)

The ena_netdev.h and ena_efa_com.h headers are not present in this patch.

[mk_semihalf.com]

Thanks Ryan for checking build with gcc and review.

I will upload compilation warnings fixes in next patch, which will be responsible for code cleanup.

sys/contrib/ena-com/ena_efa_com.c

34–36 ↗

(On Diff #32441)

Yes, this file probably shouldn't be uploaded with newest version of the ena_com. It looks like it is Linux specific.
We will remove this when committing new version into vendor-sys.

[rlibby]

sys/contrib/ena-com/ena_efa_com.c

34–36 ↗

(On Diff #32441)

Okay, does that go for ena_efa_admin_defs.h and ena_efa_io_defs.h too? If ena_efa_com.c is deleted then they also seem to be unused, after removing the #includes from ena_includes.h. A GENERIC kernel builds with those three files deleted and removed from ena_includes.h.

With ena_efa cut out, the diffstat is much smaller.

[mk_semihalf.com]

• The ena_efa files were removed
• Removed usage of LinuxKPI
• Implement macro ENA_MEMCPY_TO_DEVICE_64 in ena_plat.h file

[rlibby]

sys/contrib/ena-com/ena_plat.h

260 ↗

(On Diff #32513)

Why?

285–291 ↗

(On Diff #32513)

Getting this right may be tricky, so LinuxKPI may end up simpler. But assuming you really do want to avoid it, then...

Doesn't this need:

• cld, to ensure you are counting up?
• "memory", to tell the compiler about pointer dereference?

Is this particularly sensitive code that the generic path below doesn't perform well enough?

Should this maybe be bus_space_write_multi?

292–296 ↗

(On Diff #32513)

Declare as volatile uint64_t *to and const uint64_t *from?

299–300 ↗

(On Diff #32513)

You're assigning to to but incrementing dst.

[mk_semihalf.com]

• Remove non-generic ENA_MEMCPY_TO_DEVICE_64
• Fix ENA_MEMCPY_TO_DEVICE_64

sys/contrib/ena-com/ena_plat.h

260 ↗

(On Diff #32513)

uint64_t is defined as unsigned long on amd64 FreeBSD. However, u64 on Linux is unsigned long long.

There is one printout, where the value is casted to (u64) and it has flag %llu. We are getting compilation error, because our u64 was unsigned long.

We cannot change it in ena_com to %ju. The Linux cannot handle it, I think that it was introduced in C99.

285–291 ↗

(On Diff #32513)

Unfortunately, we cannot give you exact numbers if there is any performance boost because for now we cannot test it with WC enabled due to device limitations. We will stay with generic part for now, I will fix it in the next diff.

I will try to gather some test results when we will be able to test it with WC enabled.

[rlibby]

By the way the files in this review still have the wrong mode bits (100755 instead of 100644, need chmod a-x).

sys/contrib/ena-com/ena_plat.h

260 ↗

(On Diff #32513)

It's better to fix the formatter than to use an ambiguous-size type for a type alias called "u64". You should be able to address that with the macros from inttypes.h, e.g.:

	ena_trc_dbg("AENQ! Group[%x] Syndrom[%x] timestamp: [%"PRIu64"s]\n",
	                    aenq_common->group,
	                    aenq_common->syndrom,
	                    (u64)aenq_common->timestamp_low +
	                    ((u64)aenq_common->timestamp_high << 32));

At least on FreeBSD. I'm not sure whether the inttypes.h macros are available in the linux kernel. If not then you could cast to unsigned long long just here for the format argument.

[mk_semihalf.com]

We were waiting for Linux community to accept ena_com patch (common code for both FreeBSD and Linux), but to do not waste more time, we decided to upload fix to issue with uint64_t being defined as unsigned long on amd64 on FreeBSD, and on Linux it is being defined as unsigned long long.

[mk_semihalf.com]

In D12143#252641, @rlibby wrote:

By the way the files in this review still have the wrong mode bits (100755 instead of 100644, need chmod a-x).

We will change the mode bits when we will be preparing patch for vendor-sys, for now it shouldn't matter as this diff won't be merged directly (ena-com must go through vendor-sys branch)