drivers/network: add ixgbe driver - #682
Conversation
|
@terryzbai What's the current state on this PR? Is this just waiting on having a PCIe driver to be merged? |
That's one of the reasons. Another reason is that I haven't got time to look at the event logics, which seem different from other ethernet drivers that we have. Btw, it would be great for development if we have a machine with less booting time. |
6b14384 to
3e27cd9
Compare
omeh-a
left a comment
There was a problem hiding this comment.
Just a quick pass mostly looking at style / maintainability. I haven't had time to consult the ixgbe manual (and it's been way too long since I last touched it). There's a huge amount of dead code in ethernet.h that should probably be pruned. I also am not convinced that the changes to board.py and the echo server are prudent.
| eth_regs->eims = 0xFF; | ||
| } | ||
|
|
||
| void print_mac_addr() |
| * This defines whether we actually try to benchmark, setup the PMU etc. | ||
| */ | ||
| #if defined(CONFIG_ENABLE_BENCHMARKS) && (defined(CONFIG_ARCH_ARM) || defined(CONFIG_ARCH_RISCV)) | ||
| #if defined(CONFIG_ENABLE_BENCHMARKS) && (defined(CONFIG_ARCH_ARM) || defined(CONFIG_ARCH_RISCV) || defined(CONFIG_ARCH_X86_64)) |
There was a problem hiding this comment.
isn't this allowing all of our architectures now (except 32 bit x86)? can probably reduce this check
There was a problem hiding this comment.
I think what @omeh-a means is that you can just do
#if defined(CONFIG_ENABLE_BENCHMARKS)
since we now support all CPU architectures that the Microkit support.
There was a problem hiding this comment.
Feel free to resolve the comment if I misunderstood.
There was a problem hiding this comment.
yea, I did that but haven't uploaded it.
| timer=0xFED00000, | ||
| ethernet=0xFE000000, | ||
| ), | ||
| Board( |
There was a problem hiding this comment.
IMO we really shouldn't be adding board.py listings for individual x86 machines that are in our server room. The sDDF is a project that's used by people outside the lab and I think we should avoid adding code that's dead for everybody outside of TS.
There was a problem hiding this comment.
This should be fixed once the ACPI/PCI driver is merged. But for now, there seems no better solutions.
There was a problem hiding this comment.
I think we should probably add some indirection to indicate what this is ... e.g. make a ts_x86_machines.py which we import from for this instead of board.py or something (and that doesn't necessarily need to be on main)... I just think it doesn't really make sense to upload this since it will be useless to all externals
There was a problem hiding this comment.
...long in-person discussion...
Given that uploading configurations of TS internal x86 machines has no harm but gives the external users the examples to run on their own machines, so I will just leave some instructions in the board.py file instead.
There was a problem hiding this comment.
I still think this really is not a good idea. Going to look extremely silly when we need to open a pull request to update these magic numbers one day... having internal machines explicitly doesn't give external users the option to run locally, it just gives them an example for how they can try match their local machine. It literally only benefits us. It's also worth observing that we have maintained the principle of not doing this for the entire history of the sDDF, under this very reasoning. I don't see why we should give it up now when the project is mature when it was too hacky for the past
There was a problem hiding this comment.
We should instead have a guide for how users can collect their own magic info and we should give them a way to pass those numbers in instead. E.g. using argparse in the metaprogram
| uint32_t lo, hi, unused; | ||
| __asm__ __volatile__("rdtscp" : "=a"(lo), "=d"(hi), "=c"(unused)); | ||
| __asm__ __volatile__("lfence" ::: "memory"); | ||
| cycle_count = ((uint64_t)hi << 32) | lo; |
There was a problem hiding this comment.
While this works for just measuring CPU utilisation, it is kind of sus because the TSC frequency is != to the CPU frequency. You should use the PMU counter for this kind of job. Given that the ARM and RISC-V already uses the proper cycle counter.
| paddr_top=0x70000000, | ||
| serial=0x3F8, | ||
| timer=0xFED00000, | ||
| ethernet=0x90200000, |
There was a problem hiding this comment.
Please add detailed instruction on how these addresses are derived on from a working Linux boot.
There was a problem hiding this comment.
hmm I just realised that the values read on Linux could be wrong, probably because Linux re-configures the BARs at its boot time. I got the BAR addresses via BAR registers on ECAM.
There was a problem hiding this comment.
Does it? or does it rely on the platform firmware configured value? You could look at the PCI code in Linux and see what it does. Or write a small Microkit based program that walks the PCI bus and read out the value via the 0xCF8 and 0xCFC ports.
| #define UTILIZATION_PORT 1237 | ||
|
|
||
| #define TCP_ECHO_MAX_CONNS 4 | ||
| #define TCP_ECHO_MAX_CONNS 10 |
There was a problem hiding this comment.
I'd prefer that changes like this are isolated in its own commit, so that you can justify why the change is necessary in the commit message. Rather than reviewers having to ask you later
|
From your PR body:
Yet I don't see the driver using MSI at all, does this need to be edited then? On the same note, is it hard to support MSI, that would be more performant and less brittle compared to INTx. |
| if (ch == timer_config.driver_id) { | ||
| if (device.init_stage == 0) { | ||
| init_1(); | ||
| } else if (device.init_stage == 1) { | ||
| init_2(); | ||
| } else if (device.init_stage == 2) { | ||
| init_3(); | ||
| } |
There was a problem hiding this comment.
Might be a horrible idea, but now that #707 is merged, can we just spin wait / seL4_Yield() for the required amount of time instead of needing to use a timer driver?
There was a problem hiding this comment.
Same applies for the imx mmc driver
There was a problem hiding this comment.
We shouldn't spin wait if we can help it... A timeout is not that bad.
There was a problem hiding this comment.
yea, and ixgbe driver needs 10s for stabilising the hardware.
There was a problem hiding this comment.
10 seconds seems a long time. Really?
There was a problem hiding this comment.
It would be best if you include a X550 manual reference for the timeout.
There was a problem hiding this comment.
well, this isn't documented in the manual. The initial version of this driver is ported from atmosphereOS.
https://github.com/mars-research/atmosphere/blob/bd485f22f1d5e4d1623e133700dc233086059603/ixgbe_driver/src/device.rs#L258-L262
| paddr_top=0x70000000, | ||
| serial=0x3F8, | ||
| timer=0xFED00000, | ||
| ethernet=0xFE000000, |
There was a problem hiding this comment.
How is this value derived? You need to document it, feel free to copy what I did in libvmm: https://github.com/au-ts/libvmm/tree/main/examples/virtio_pci#x86-64-hardware-requirements
There was a problem hiding this comment.
For QEMU, I think it would be better to move your instructions to sDDF repo and link it in LibVMM example.
|
Slightly off-topic, I wonder if making this change to clang.mk will improve the echo server performance with this driver: sddf/tools/make/toolchain/clang.mk Line 30 in da05d66 - CFLAGS_ARCH := -mtune=$(CPU)
+ CFLAGS_ARCH := -mtune=$(CPU) -march=x86-64-v2With this change the compiler will emit more modern and faster instructions without requiring AVX vector instructions, which the Microkit doesn't enable on x86 currently. |
| # Legacy I/O APIC | ||
| eth_irq = SystemDescription.IrqIoapic(ioapic_id=0, pin=16, vector=8, trigger=IrqIoapic.Trigger.LEVEL, | ||
| polarity=IrqIoapic.Polarity.ACTIVELOW, id=16) |
There was a problem hiding this comment.
This is incorrect for sdfgen 0.33.0 no? You fixed the corresponding bug in sdfgen but have not triggered a release: au-ts/microkit_sdf_gen@608ef02
You will need to make a 0.34.0 release and pin it in your PR.
There was a problem hiding this comment.
@Courtney3141 Can you please trigger a release for this?
|
Would this driver work on Microkit 2.3.0? I don't think so because sdfgen doesn't support generation of the IOMMU SDF elements. You will need to document how users can get it working on 2.3.0 (i.e. disable the IOMMU and rebuild the SDK from source), or support the 2.2.0 SDK. |
| ethernet_driver.add_map(Map(hw_tx_ring_buffer, vaddr=0x2404000, perms="rw")) | ||
|
|
||
| # Legacy I/O APIC | ||
| eth_irq = SystemDescription.IrqIoapic(ioapic_id=0, pin=16, vector=8, trigger=IrqIoapic.Trigger.LEVEL, |
There was a problem hiding this comment.
vector=8 is low priority. I reckon the ethernet IRQ should be a higher priority, make it one less than the HPET vector
There was a problem hiding this comment.
It seems not matter too much in this example, as there are only HPET and ethernet driver having user IRQs. Note: virtio-net driver also has vector=1.
Does the priority order of IRQ always match the priority order of the corresponding driver PDs? If so, this might be worth being handled by sdf_gen/Acacia.
Note it's possible to pass |
yes, I decided to remove MSI/MSI-x support for this PR because users would need to manually uncomment and comment code snippet, which is very ugly. Have updated the PR description. |
Another method which is super easy is to change the IOMMU boolean from True to False in build_sdk.py |
|
Oops sorry |
To fix: au-ts/sddf#682 (comment) Signed-off-by: Bill Nguyen <bill.nguyen@unsw.edu.au>
To f i x this issue: au-ts/sddf#682 (comment) Signed-off-by: Bill Nguyen <bill.nguyen@unsw.edu.au>
889d272 to
de3c4e0
Compare
This adds the initial ixgbe driver, which has been tested on X550/X540 NICs. The device reigsters and IRQ numbers are hard-coded before the PCIe driver is properly implemented. Signed-off-by: Terry Bai <tianyi.bai@unsw.edu.au>
This adds the physical TSC reading for CPU utilisation measurement. Signed-off-by: Terry Bai <tianyi.bai@unsw.edu.au>
Signed-off-by: Terry Bai <tianyi.bai@unsw.edu.au>
Signed-off-by: Terry Bai <tianyi.bai@unsw.edu.au>
de3c4e0 to
5bea2f7
Compare
| SEL4_64B = $(MICROKIT_SDK)/board/$(MICROKIT_BOARD)/$(MICROKIT_CONFIG)/elf/sel4.elf | ||
| SEL4_32B := $(MICROKIT_SDK)/board/$(MICROKIT_BOARD)/$(MICROKIT_CONFIG)/elf/sel4_32.elf |
There was a problem hiding this comment.
Could be refactored, same with vtx.mk
| if test_config.board.startswith("vb_105"): | ||
| # This x86 machine takes around 3 mintues to boot | ||
| timeout = 200 | ||
|
|
There was a problem hiding this comment.
The boot timeout is separate from the test timeout, this shouldn't be here, see https://github.com/au-ts/systems-ci/blob/f1127dfb4c47d0c5bc3512d3678b3434c3835c34/ts_ci/backends/machine_queue.py#L19
So this block should be removed
To f i x this issue: au-ts/sddf#682 (comment) Signed-off-by: Bill Nguyen <bill.nguyen@unsw.edu.au>
This driver is for Intel X540/X550 NICs and works with IO-APIC
and MSI/MSI-X interrupts.(remove MSI/MSI-X support for now to avoid hacky code)Before a proper PCI driver (#622) gets implemented, this experimental driver needs the access to PCI configuration space and set things up by itself. Interrupt types need to be manually switched by (un)commenting code snippets in
meta.pyandethernet.c.