ix/ixv: Add support for new Intel Ethernet E610 family devices

[Yogesh.Bhosale_intel.com]

Summary

This is part 1 of the support for the new Intel Ethernet E610 family of devices.

Introduce new PCI device IDs:
• 57AE: Intel(R) E610 (Backplane)
• 57AF: Intel(R) E610 (SFP)
• 57B0: Intel(R) E610 (10 GbE)
• 57B1: Intel(R) E610 (2.5 GbE)
• 57B2: Intel(R) E610 (SGMII)

Key updates for E610 family:
• Firmware manages Link and PHY
• Implement new CSR-based Admin Command Interface (ACI) for SW-FW interaction
• Tested exclusively for x64 operating systems on E610-XT2/XT4 (10G) and E610-IT4 (2.5G)
• Enable link speeds above 1G: 2.5G, 5G and 10G
• NVM Recovery Mode and Rollback support

Signed-off-by: Yogesh Bhosale [e-mail]
Co-developed-by: Krzysztof Galazka [e-mail]

[kgalazka]

@erj, @kbowling: We're still awaiting for feedback from validation team, but would you mind taking a look?

[erj]

In the description:

Support exclusively for x64 operating systems; no 32-bit OS support

Doesn't the in-kernel ixgbe driver still build for i386? Does this just mean no "official" support when it's running on non amd64 kernels?

Enable link speeds above 1G: 2.5G, 5G and 10G

These are already enabled -- do you mean just beyond the initial 1G E610 part? It's a little confusing to add this when support for those link speeds already exists.

Otherwise,
I think it mostly looks good -- but what's going to be included in part 2?

sys/dev/ixgbe/if_ix.c

1589

This should say "Link", right?

3054

It feels like you could combine this call with the one below for ixgbe_shutdown_aci() to reduce the number of if-statements here; mta isn't needed in these E610-only cleanup calls?

sys/dev/ixgbe/ixgbe_type_e610.h

805

Any 25G Base-T parts on the horizon for Linkville? :p

Or, are those only coming in maybe a future follow-on...

[kbowling]

To add to what @erj said, multi-arch build is a requirement the same as on other OS projects. We have to provide some advance notice to drop things and this should be MFC'd back to 13 and 14. See build(7), 'make universe' or 'make kernels' is enough. I don't see anything off hand that is problematic, and there is no expectation on Intel for validation for odd platforms. But it has to build everywhere it currently does.

[kgalazka]

In D50067#1149007, @erj wrote:

In the description:

Support exclusively for x64 operating systems; no 32-bit OS support

Doesn't the in-kernel ixgbe driver still build for i386? Does this just mean no "official" support when it's running on non amd64 kernels?

It does build on i386 and other architectures. The intention was only to make it clear that it was tested only on x86. I should have notice that the choice of words is not very fortunate. We'll fix that.

To be sure I run make kernels as suggested by Kevin and there was only one error on arm.TEGRA124, but unrelated to ixgbe:

src/sys/dev/drm2/ttm/ttm_bo_vm.c:238:2: error: call to undeclared function 'pctrie_iter_reset'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
  238 |         pctrie_iter_reset(&pages);

Enable link speeds above 1G: 2.5G, 5G and 10G

These are already enabled -- do you mean just beyond the initial 1G E610 part? It's a little confusing to add this when support for those link speeds already exists.

Yeah, that also needs better wording.

Otherwise,
I think it mostly looks good -- but what's going to be included in part 2?

In part 2 we will add support for debug dump and in part 3 FW logging. I thought it may be easier to review that way.

[kbowling]

Please reupload with context (git diff -U9999). It also seems to not be based on FreeBSD main.

sys/dev/ixgbe/if_ix.c

1414

I'm a little confused by this, is this diff against git main? @erj already pointed out we have support for this.

2441

this seems unnecessary if you rebase on main

sys/dev/ixgbe/ixgbe.h

200

this should not be necessary

sys/dev/ixgbe/ixgbe_vf.c

659

I wonder if this should all just be folded into the top level case, and the mac type and NON_STD chacks dropped.. isn't link speed just informational for a VF?

[kbowling]

sys/dev/ixgbe/ixgbe_osdep.h

203

is the aci accessed by multiple threads? the iflib admin task, media status, init, etc is protected internally by the CTX_LOCK

I wonder if you might be able to change ixgbe_acquire_lock to
#ifdef INVARIANTS
sx_assert(iflib_ctx_lock_get(((struct e1000_osdep *)hw->back)->ctx), SX_XLOCKED)
#endif

and make the other lock functions no-op and see what a WITNESS kernel says.

240

It seems this is main usage of this beyond the small HW buffer limits is to get the shadow ROM in a contiguous buffer which the checksum is then computed incrementally in word size chunks anyways. It could go away with a small refactoring to just move the 4k reads to the right place?

246

it seems like this is used for 4k IXGBE_ACI_MAX_BUFFER_SIZE operations, is there any reason not to do that on the stack?

[kbowling]

sys/dev/ixgbe/ixgbe_osdep.h

246

Otherwise since the use is all serialized, malloc one or more buffers you need on init and access them as needed. I don't think it makes sense to expose the runtime path to malloc and the associated error handling.

[Yogesh.Bhosale_intel.com]

Combining if's as suggested by Eric and fixing a kernel panic issue during unload

[Yogesh.Bhosale_intel.com]

Combining if's as suggested by Eric and fixing a kernel panic issue during unload

Also fixing a broken update of the patch

[Yogesh.Bhosale_intel.com]

sys/dev/ixgbe/if_ix.c

1414

This is intentional. The X550 relies on mac type for speed detection, while the E610 driver can determine speeds using layer flags.

1589

I will fix that.

3054

Done.

sys/dev/ixgbe/ixgbe_osdep.h

203

We plan to add debug dump and firmware logging features that will access this interface using ioctl and sysctl, which are not protected by the iflib context lock. I will submit a patch for the debug dump shortly to clarify the use case and simplify this review.

[Yogesh.Bhosale_intel.com]

Fixing a typo

[kbowling]

Generally ok with this. I would like to know if the malloc thing can be addressed during init or if that is required by your upcoming changes.

[kgalazka]

sys/dev/ixgbe/if_ix.c

2441

I'm not sure what are you referring to. It was rebased on main. I don't see anything missing here:
https://cgit.freebsd.org/src/tree/sys/dev/ixgbe/if_ix.c#n2373.

sys/dev/ixgbe/ixgbe.h

200

As above - I'm not sure what do you mean. I don't see anything like that on main.

sys/dev/ixgbe/ixgbe_osdep.h

246

it seems like this is used for 4k IXGBE_ACI_MAX_BUFFER_SIZE operations, is there any reason not to do that on the stack?

I think it was based on ICE code, where those buffers are used for DMA. Anyway it is used by the part of the code shared between OSes and some of them have more explicit requirements regarding stack usage.

Otherwise since the use is all serialized, malloc one or more buffers you need on init and access them as needed.

Both debug dump and FW logging use ACI interface, and only ixgbe_aci_send_cmd_execute call is serialized with ACI lock. One buffer won't be enough. With multiple buffers we would have to track which are in use. It's not a trivial change I guess.

I think it would be useful if we can stay in sync with the out-of-tree version for a while to leverage work of our teammates working on other OSes.

[Yogesh.Bhosale_intel.com]

Fix ixv driver loading issue

[Yogesh.Bhosale_intel.com]

sys/dev/ixgbe/ixgbe_osdep.h

203

@kbowling:

Here is the patch I referred to earlier: https://reviews.freebsd.org/D50934

Please take a look and let me know if you still have any concerns.

[kgalazka]

sys/dev/ixgbe/ixgbe_type_e610.h

805

It looks like we missed that 25G PHY types were removed from the final version of a E610 datasheet, so I guess not in a nearest future. We should probably remove them from the code too.

sys/dev/ixgbe/ixgbe_vf.c

659

I'm not sure about the use case but at least on Linux folks like to have accurate information about the link speed, so unless you think it may cause some issues I would prefer to keep it.

[Yogesh.Bhosale_intel.com]

Review comment and a fix for firmware event

[kgalazka]

@erj @kbowling - We've got a green light from our validation team. Could you please take a look at latest updates?

[kbowling]

@kgalazka I'm ok with this, go ahead. Please think about budgeting some time for tinderbox, lint, and early user feedback issues that may come up, and by extension when you want to send it in, i.e. a Monday would be good.

[erj]

I think it's ok as-is, but there are some future changes to look at.

sys/dev/ixgbe/if_ix.c

1414

I don't remember -- why couldn't the X550T use these physical layer flags, too? I'm looking at ixgbe_x550.c, and it appears someone could've created an ixgbe_get_supported_physical_layer_X550T or similar that specified the 2500BASE_T and 5000BASE_T speeds.

Though, the X540 version reads an MDIO register for the capabilities; were those new 2500BASE_T and 5000BASE_T speeds not reported there for X550T devices?

Regardless, your explanation is good enough for me for E610 -- I just noticed this because @kbowling called it out and I don't think I ever looked into why there's a difference here, other than X550T didn't "officially" support 2.5G and 5G speeds.

5158

No SR-IOV support? Someone is going to complain.

sys/dev/ixgbe/ixgbe_common.c

3703

Isn't this sort of a codename leak? Someone should rename it from LKV -> E610.

sys/dev/ixgbe/ixgbe_vf.c

649

Doesn't this check need to be updated for E610 VFs as well?

[kgalazka]

Fixing Eric's comment about codename seemed as a trivial change so I tried to do it without engaging Yogesh and updating the review. I'm sorry, it looks like it was a poor idea. Commit wasn't automatically detected by Phabricator and this review is still open. Is there anything else I should do except for adding the commit with 'Edit Related Objects', and promising to not try to cut the corners in the future?

[kbowling]

In D50067#1188547, @kgalazka wrote:

Fixing Eric's comment about codename seemed as a trivial change so I tried to do it without engaging Yogesh and updating the review. I'm sorry, it looks like it was a poor idea. Commit wasn't automatically detected by Phabricator and this review is still open. Is there anything else I should do except for adding the commit with 'Edit Related Objects', and promising to not try to cut the corners in the future?

Trivial edits are a normal thing to do when committing, especially if there is implicit approval like this. The phab integration is based solely on the 'Differential Revision:' tag, and it does seem like the commit was picked up. Usually it would automagically update the review and close it so I don't know what is different here. You can simply close the review out in the action drop down, the commit link is enough context back and forth IMO.

[erj]

I think the automatic close fails if the diff is too big or has too many files? It's happened a lot to me, but I never figured out why. So as @kbowling says, it's not cutting corners, it's just an imperfect integration between git and Phabricator; it's normal to make small edits between the review being approved and pushing it upstream.

[kgalazka]

Thanks again!

[markj]

sys/dev/ixgbe/ixgbe_e610.c

1412

I see a compiler warning that this function is unused. Is that expected?

[kgalazka]

sys/dev/ixgbe/ixgbe_e610.c

1412

Most probably not. I'm sorry for a late response, I've been AFK due to family reasons. I'll take care of that early next week.

[Yogesh.Bhosale_intel.com]

sys/dev/ixgbe/ixgbe_e610.c

1412

I have raised a review for the fix: https://reviews.freebsd.org/D52467

[markj]

sys/dev/ixgbe/ixgbe_e610.c

1412

Perfect, thank you!