xen/efi: introduce a PV interface for EFI run time services for dom0
ClosedPublic
Actions

Authored by royger on Feb 12 2021, 4:03 PM.

Details

Reviewers

royger
kib

Commits

rG952667da986c: xen/efi: introduce a PV interface for EFI run time services for dom0

Summary

FreeBSD when running as a dom0 under Xen is not supposed to access the
run time services directly, and instead should proxy the calls through
Xen using an hypercall interface that exposes access to selected run
time services.

Implement the efirt interface on top of the Xen provided hypercalls.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 36936
Build 33825: arc lint + arc unit

Event Timeline

royger created this revision.Feb 12 2021, 4:03 PM

Herald added a subscriber: imp. · View Herald TranscriptFeb 12 2021, 4:03 PM

Harbormaster completed remote builds in B36936: Diff 83772.Feb 12 2021, 4:03 PM

royger requested review of this revision.Feb 12 2021, 4:03 PM

royger mentioned this in D28620: efirt: add hooks for diverging EFI implementations.Feb 12 2021, 4:04 PM

kevans added a subscriber: kevans.Feb 12 2021, 4:53 PM

kevans added inline comments.

sys/dev/xen/efi/pvefi.c
236	If pvefi isn't being used (disabled or not dom0), this would seem to break stock efirt upon load/unload as we set efi_ops to the null-filled prev.

Looks reasonable, modulo kevan's comments

Thanks for the comment!

sys/dev/xen/efi/pvefi.c
236	I originally had the check above returning ENXIO, but changed it as I didn't know whether that would be reported as an error. I guess returning ENXIO from load will prevent unload attempts?

kevans added inline comments.Feb 12 2021, 6:51 PM

sys/dev/xen/efi/pvefi.c
236	Annoyingly enough, if you return ENXIO it will immediately be issued a MOD_UNLOAD anyways -- it's kind of a no-win situation. :-(

kib added a subscriber: kib.Feb 12 2021, 11:51 PM

kib added inline comments.

sys/dev/xen/efi/pvefi.c
236	Can you please explain more how it breaks bare-metal efirt? That said, I am not sure the prev business is useful at all.

kevans added inline comments.Feb 13 2021, 12:25 AM

sys/dev/xen/efi/pvefi.c
236	Noting how it's implemented from the other review, we end up with an empty ops vector if prev is still .bss (early return in MOD_LOAD path)

royger added inline comments.Feb 15 2021, 9:40 AM

sys/dev/xen/efi/pvefi.c
236	It's likely not of much usage restoring active_efi_ops to prev on module unload, since you either use one of the two interfaces, but not both. I'm just doing it to prevent ending up with active_efi_ops == NULL which will trigger a page-fault.

Fix module unload.
Make module depend on efirt.

Harbormaster completed remote builds in B37013: Diff 83917.Feb 15 2021, 9:52 AM

kib accepted this revision.Feb 15 2021, 9:56 AM

kib added inline comments.

sys/dev/xen/efi/pvefi.c
236	But if prev is NULL, it is still the situation with dandling pointers. I would just drop the prev stuff, but I do not insist.

This revision is now accepted and ready to land.Feb 15 2021, 9:56 AM

royger added inline comments.Feb 15 2021, 10:00 AM

sys/dev/xen/efi/pvefi.c
236	I just added a prev != NULL check in this last version before attempting to set active_efi_ops = prev in the unload case. I'm not sure what's the best way to deal with this, should we have a nop_efi_ops that has all pointers set to NULL in order to set it here on module unload if the module was in-use?

kib added inline comments.Feb 15 2021, 11:25 AM

sys/dev/xen/efi/pvefi.c
236	nop_efi_ops is perhaps fine if the native vector is also in module with the same prev handling. But it is probably too much for useless scenarios.

royger added inline comments.Feb 15 2021, 3:47 PM

sys/dev/xen/efi/pvefi.c
236	efirt module is also where active_efi_ops is defined, so if the module is unloaded so is the symbol itself, hence not sure it makes much sense to set it to nop_efi_ops anyway.