Allow net_cleanup for loader.efi

[sjg]

Summary

While netbooting with loader.efi on at least one arm64 platform
which uses u-boot emulating UEFI, the kernel gets corrupted, we
suspected the u-boot ethernet driver was still running.

Use netdev.dv_cleanup for efinet_dev to address this.

This in turn requires calling dev_cleanup() before bi_load() to avoid
a loader crash.

Sponsored by: Juniper Networks, Inc.

[imp]

So what happens when bi_load() returns an error? Most devices don't have the 'dv_cleanup()' routines, so it doesn't matter that much... But wouldn't this preclude network working if it does cleanup? I think it might be OK.

So looking more generally, we should likely do this before building the metadata on all the loaders... But i386 this would be bad since we'd need to re-call pxe_enable() if we did it this way. Similar issues crop up with userdisk and udev storage: they both assume that if you call cleanup, no further interaction is possible.

So it kinda works for EFI, but I'm hesitant to break symmetry since we've not documented things very well... But exit boot services is done in a MI way, so maybe that needs to move instead to after dev_cleanup()?

[sjg]

In D51186#1168883, @imp wrote:

So what happens when bi_load() returns an error? Most devices don't have the 'dv_cleanup()' routines, so it doesn't matter that much... But wouldn't this preclude network working if it does cleanup? I think it might be OK.

I don't know actually. I found that after bi_load() is called we can no longer use printf to get any debug output.
You can see a comment that was in the arm and riscv versions of exec to the effect that not much is expected to work after we call bi_load() which probably explains why we exploded when trying to call net_cleanup() after the call to bi_load()

So looking more generally, we should likely do this before building the metadata on all the loaders... But i386 this would be bad since we'd need to re-call pxe_enable() if we did it this way. Similar issues crop up with userdisk and udev storage: they both assume that if you call cleanup, no further interaction is possible.

AFAICT once you call bi_load() no further interaction is possible. Both netboot and normal boot work with this re-ordering - though we are only able to test on amd64 and arm64

So it kinda works for EFI, but I'm hesitant to break symmetry since we've not documented things very well... But exit boot services is done in a MI way, so maybe that needs to move instead to after dev_cleanup()?

[sjg]

In D51186#1168883, @imp wrote:

So what happens when bi_load() returns an error? Most devices don't have the 'dv_cleanup()' routines, so it doesn't matter that much... But wouldn't this preclude network working if it does cleanup? I think it might be OK.

It looks like the only case for which bi_load() returns an error - vs panic is failure to find rootdev which seems the sort of thing that could have been done earlier?
If that were extracted to some earlier call, then bi_load() looks like a one way trip to boot or panic

[imp]

In D51186#1169123, @sjg wrote:

In D51186#1168883, @imp wrote:

So what happens when bi_load() returns an error? Most devices don't have the 'dv_cleanup()' routines, so it doesn't matter that much... But wouldn't this preclude network working if it does cleanup? I think it might be OK.

It looks like the only case for which bi_load() returns an error - vs panic is failure to find rootdev which seems the sort of thing that could have been done earlier?
If that were extracted to some earlier call, then bi_load() looks like a one way trip to boot or panic

It may be a redunant check. But I'm not sure that bi_load is the right place to do exit boot services since I don't want its callers to know that only panics are possible because that changes over time.

But I think your analysis is flawed, since we have

if (retry == 0) {
        BS->FreePages(addr, pages);
        printf("ExitBootServices error %lu\n", EFI_ERROR_CODE(status));
        return (EINVAL);
}

late in the bi_load. But maybe that should be a panic. But there's several other places where we allocate memory and return an error... but once bi_load_efi_data returns, we won't return an error. and the order inside of bi_load() is such that we have to exit boot services there since we have to exit boot services to get the memory map... which kinda sucks... we want to run dev_cleanup() before we exit boot services, so maybe we need to move it into before this loop and turn memory allocation errors here into panics since we can't really recover from them, I don't think.

Then again, we (bogusly?) ignore the bi_load_efi_data return value, so you are right the only place we get an error is where you say. So maybe that needs to move it into the exec() routines just before we call dev_cleanup(). I'd add a comment to each of them about why we need to run dev_cleaniup() before bi_load(). That's likely the best path forward. So returning errors from bi_load_efi_data() should be converted to panics.

Does that make snese?

[sjg]

In D51186#1169361, @imp wrote:

In D51186#1169123, @sjg wrote:

In D51186#1168883, @imp wrote:

So what happens when bi_load() returns an error? Most devices don't have the 'dv_cleanup()' routines, so it doesn't matter that much... But wouldn't this preclude network working if it does cleanup? I think it might be OK.

It looks like the only case for which bi_load() returns an error - vs panic is failure to find rootdev which seems the sort of thing that could have been done earlier?
If that were extracted to some earlier call, then bi_load() looks like a one way trip to boot or panic

It may be a redunant check. But I'm not sure that bi_load is the right place to do exit boot services since I don't want its callers to know that only panics are possible because that changes over time.

But I think your analysis is flawed, since we have

if (retry == 0) {
        BS->FreePages(addr, pages);
        printf("ExitBootServices error %lu\n", EFI_ERROR_CODE(status));
        return (EINVAL);
}

late in the bi_load.

I was only looking at bi_load() itself.

But maybe that should be a panic. But there's several other places where we allocate memory and return an error... but once bi_load_efi_data returns, we won't return an error. and the order inside of bi_load() is such that we have to exit boot services there since we have to exit boot services to get the memory map... which kinda sucks... we want to run dev_cleanup() before we exit boot services, so maybe we need to move it into before this loop and turn memory allocation errors here into panics since we can't really recover from them, I don't think.

Then again, we (bogusly?) ignore the bi_load_efi_data return value, so you are right the only place we get an error is where you say. So maybe that needs to move it into the exec() routines just before we call dev_cleanup(). I'd add a comment to each of them about why we need to run dev_cleaniup() before bi_load(). That's likely the best path forward. So returning errors from bi_load_efi_data() should be converted to panics.

Does that make snese?

I think so - got a suggestion for the comment?

[sjg]

Add a comment about why dev_cleanup needs to be called before bi_load

[imp]

suggestion for a more complete comment that's still relatively short.

stand/efi/loader/arch/amd64/elf64_freebsd.c

212

/*
 * we have to cleanup here because net_cleanup() doesn't work after
 * we call ExitBootServices
*/

I think is a better comment. Without it, there's a huge effort need to understand WHY net_cleanup() won't work. With it, it's clear and obvious.

[sjg]

Tweak comment as suggested

[imp]

OK. I'm happy now!