In D23808#523738, @ae wrote:

It seems some of these macros should be fixed to respect style(9) and use tab instead of space after #define.

With this patch both riscv64 and riscv64sf TARGET_ARCH build result in something that can build ports. They both report the expected hw.machine_arch, and both have the correct MACHINE_ARCH set in bmake.

I've also had to apply this patch, but that's an issue that was already present:

In D23813#523305, @imp wrote:

I can't test-boot this change, however... I'm not sure about the lp64->lp64d for mabi.

In D23813#523305, @imp wrote:

I can't test-boot this change, however... I'm not sure about the lp64->lp64d for mabi.

There's certainly still something missing. When I build with make TARGET_ARCH=riscv64sf it builds with MACHINE_ARCH = riscv64, and that breaks ports builds (because we set the wrong flags).

In D23741#522425, @mhorne wrote:

I found the following excerpt in bsd.cpu.mk:

.if ${MACHINE_CPUARCH} == "riscv"
.if ${MACHINE_ARCH:Mriscv*sf}
CFLAGS += -march=rv64imac -mabi=lp64
.else
CFLAGS += -march=rv64imafdc -mabi=lp64d
.endif
.endif

So we could instead check for the presence of hardware floating extensions using the following check in param.h:
#if defined(__riscv_flen) && __riscv_flen != 0

https://github.com/riscv/riscv-toolchain-conventions defines __riscv_float_abi_soft as well, so I don't know if that helps us.
It's also not explicit about what the value will set to if neither F nor D extensions are present (although yes, I'd expect it to be unset or 0).

The previous version was an incomple fix.
It did ensure that the hw.machine_arch was correct, but bmake uses the
MACHINE_ARCH define if it exists. As it includes sys/params.h this was the
case. In other words: we need to ensure that MACHINE_ARCH is set correctly for
userspace builds as well.

I'm not sure why we care about CTLFLAG_MPSAFE on nodes with no handler (pf, pfsync), but I have no objections.

• sort test list in Makefile
• rename test case

Hmm. I wonder if it's not better the actually add the cleanup call to the atf_test_case definition.
It's not needed if the test passes, but perhaps it makes sense to try to restore the volume if the test should fail.

This patch series is happy on my SiFive FU540 board, and I'm happy with the patches.

In D23405#513359, @mhorne wrote:

I'm inferring that this driver was written using this device tree, as this is what is passed by the FSBL. If there is a different one that was used please point me to it.

In D23404#513373, @mhorne wrote:

In D23404#513349, @kp wrote:

At first glance this looks good.
I have a SiFive FU540 I can test with tomorrow.
I only implemented the core clock because it feeds the SPI / UART / I2C clocks. The DDR and gigabit ethernet clocks were not relevant.

Thanks. They will still be unused/irrelevant at present, but my thinking is that it is useful to have so we can eventually set the cgem's reference clock.

At first glance this looks good.
I have a SiFive FU540 I can test with tomorrow.

In D23316#512773, @melifaro wrote:

However, I'm not sure what is the best way of telling the framework/wrapper that ipfw/ipdivert has to be loaded in advance, so we actually run these tests :-)

In D23316#512620, @lwhsu wrote:

While other test cases haven't done this yet, but I'd like to see the new tests can restore the system as much as they can, if it doesn't bring too much complexity:

1. Can ipfw and ipdivert be unloaded safely after test?

Unloading the IPFW module seems like a bad idea. The netpfil/common tests rely on the module being loaded to run IPFW tests.
I believe the module is loaded because the test/build scripts explicitly load it, just like pf.

Thanks for the review.

In D23073#505713, @cem wrote:

This seems fine to me, although I'm not super familiar with vtnet / mbufs / uma. I think it might also be reasonable for uma_zone_reserve / uma_prealloc to work on zones that have already been allocated from? Maybe I misunderstand how they work.

uma_zone_reserve() asserts that the keg is cold (no allocations done from it). I can only assume there's a good reason for that, but I can't say that I know why.

In D22920#502031, @ian wrote:

Does anyone know why riscv needs writable text segments? Or if it really does?

With all three of these patches I now see this panic running the pf tests:

There's another call to if_vmove() in vnet_if_return(). We probably can't handle errors there (and I expect that D22691 will ensure it can't fail), but we should at least assert that it succeeded.

Still haven't looked at it, but it seems to fix PR 238870. No more panics (in a couple of runs of) pf:names / pf:synproxy tests, so as far as I'm concerned this cannot get merged quickly enough.

This seems to break the build:

Improve error messages

This affects the number of items (not just tables) that can be processed in a single ioctl() operation, not the number of entries we allow in a table.
pfctl by and large only works on a single table at a time, but might try to add/delete/modify/... multiple IP addresses at a time. It's generally possible to do this in multiple requests, so having more items in a table than the request_maxcount isn't a critical problem.

In D22733#497518, @lutz_donnerhacke.de wrote:

Reporting an "error too many tables" is useful for adding a new table.
It's wrong for deleting and misleading for obtaining statistics.

In D20468#495864, @kevans wrote:

In D20468#495861, @eugen_grosbein.net wrote:

Could you please supply use case? Why do we need another virtual ethernet interface? We already have many kinds of them.

As it says in the review body -- bridge(4) regression testing was/is the first candidate for use. My initial plan for testing was to spawn up a bridge and stick N of these in it in different configurations. This review has been sitting long enough now that I don't recall if I decided that wasn't feasible in the meantime (or if @kp had already had other plans), but if it is it'd be more convenient than epair -- with this, you have the absolute bare minimum needed to get a packet from bridge to userspace in 329 total lines of file without also pair management in between.

I have plans (and minor in-progress work) to do bridge tests with if_epair. I've not really found if_epair to be burdensome to write test withs (aside from its tendency to expose locking issues in the network stack).

Minor nit: Mention where this is mentioned (i.e. the comments to the function).
Other than that this looks like a good idea to me.

This happens on a riscv machine, running from an mdroot (although I'm not sure if that's relevant to trigger it), not running devd during the aio_test:md_waitcomplete regression test.
That test opens /dev/mdX and tries to perform an aio write on it, which ends up failing. I'm not sure I fully understand the intent behind the test, but it revealed that the aio code considered that to be an unsafe. The safety check code thinks that mp->mnt_stat.f_iosize is relevant, and because of the lack of VFS_STATFS call that was still set to 0 for devfs.

Other than the typo this looks good to me.

This probably also wants this:

And this is wrong, or at least very confusing, in firewall_init():

I seem to run into issues running the ipfw_basic test:

I think I'm happy with this.
I'll give Tom a bit of time to add any more remarks he might have, but I think we can commit this soon.

Full context.

In D21086#457948, @ae wrote:

Does this mean that you concluded in the IETF mailing list to drop this support?

Remove more bits, as suggested by thj