This patch fixes efi serial console hang issue in ARM64 Hyper-V.
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| share/mk/src.opts.mk | ||
|---|---|---|
| 334 ↗ | (On Diff #108915) | We'll want to update the comment also, or more likely just delete it as it's clearly limiting HyperV to some subset of architectures. |
| stand/efi/loader/conf.c | ||
| 91 | This seems odd, paging @imp | |
| sys/Makefile | ||
| 22 ↗ | (On Diff #108915) | this is an extraneous diff I think |
| sys/arm/conf/GENERIC | ||
| 285 ↗ | (On Diff #108915) | Is 32-bit ARM supported? |
| sys/dev/hyperv/vmbus/aarch64/hyperv_aarch64.c | ||
| 2 ↗ | (On Diff #108915) | May want to add 2022 here? |
Comment Actions
This change is kinda large to review... Please see if you can break it up into smaller pieces with better explanations for each of the steps.
I've only just started reviewing things... I'm not super familiar with arm64 hypervisor interfaces...
This is super cool...
| stand/efi/loader/conf.c | ||
|---|---|---|
| 91 | This seems wrong, and the #define is weird too... | |
| sys/conf/files.x86 | ||
| 136 ↗ | (On Diff #108915) | This split out suggests a good way to make this smaller... |
| sys/dev/hyperv/storvsc/hv_storvsc_drv_freebsd.c | ||
| 1842 ↗ | (On Diff #108915) | why's that? |
| 1861 ↗ | (On Diff #108915) | I don't think this #ifdef is needed. This will be a nop on x86, but is technically required by the KPI |
| 2133 ↗ | (On Diff #108915) | ditto |
| sys/dev/hyperv/vmbus/aarch64/hyperv_aarch64.c | ||
| 82 ↗ | (On Diff #108915) | Not sure that we need this comment. |
| sys/dev/hyperv/vmbus/aarch64/hyperv_machdep.h | ||
| 3 ↗ | (On Diff #108915) | Likely need to update copyright here. Suggest dropping All Rights reserved, though that's a question for your legal department. |
| stand/efi/loader/conf.c | ||
|---|---|---|
| 91 |
Device "ttyAMA" refers to a UART with hardware part # PL011. | |
Comment Actions
I was about to test the match to amd64 Hyper-V but a simple "patch -C < .....diff" wasn't successful.
-------------------------- |Index: sys/dev/hyperv/vmbus/amd64/hyperv_machdep.h |=================================================================== |--- sys/dev/hyperv/vmbus/amd64/hyperv_machdep.h |+++ sys/dev/hyperv/vmbus/amd64/hyperv_machdep.h -------------------------- File to patch:
Is there anything special to get this patch applied?
| stand/efi/loader/conf.c | ||
|---|---|---|
| 91 | The trouble is that we can't control for hypervisor vs non-hypervisor in our build. We build one loader.efi for aarch64, and the non-hypervisor code needs comconsole. It just uses the UEFI Serial interfaces, so in theory should support any device that provides a UEFI driver. It's not the same as the x86 comconsole which talks to hardware directly (yes, this is confusing) | |
| stand/efi/loader/conf.c | ||
|---|---|---|
| 91 | That is why I kept this aarch64_hyperv to avoid comconsole in consoles list, till the time we are getting a ttyAMA support for comconsole | |
| sys/arm/conf/GENERIC | ||
| 285 ↗ | (On Diff #108915) | No, will remove it. Thanks for pointing. |
| sys/conf/files.arm64 | ||
|---|---|---|
| 610 ↗ | (On Diff #108915) | Please add a space after the '#'. |
| sys/dev/hyperv/vmbus/aarch64/hyperv_aarch64.c | ||
| 5 ↗ | (On Diff #108915) | This can be dropped. |
| sys/dev/hyperv/vmbus/aarch64/hyperv_machdep.c | ||
| 3 ↗ | (On Diff #108915) | This can be dropped. |
| sys/dev/hyperv/vmbus/aarch64/hyperv_reg.h | ||
| 3 ↗ | (On Diff #108915) | This can be dropped. |
| sys/dev/hyperv/vmbus/i386/hyperv_machdep.c | ||
|---|---|---|
| 31 ↗ | (On Diff #108915) | Does it mean to be amd64/hyperv_machdep.h or we need a x86/hyperv_machdep.h ? |
Comment Actions
I have tried patch < /mnt/resource/sandbox1/src/patch_arm.diff , and it has worked for me.
I have cloned the repo from github and applied the patch in main branch.
Comment Actions
I have removed storvsc changes from here, as it will be reviewed through a separate bug. Also addressed the comments.
Comment Actions
Stripping older large diff to smaller chunks. That way it will be easier to review.
This patch disables the comconsole from supported consoles list for ARM64 Hyper-V gen2 VM. The implementation
of comconsole in ARM64 loader.efi is not supported in Hyper-V gen2 VM at this moment. As Hyper-V only supports
ttyAMA0 for serial console in ARM64. Device "ttyAMA" refers to a UART with hardware part # PL011.
It's a UART that is specific to ARM processors.
In a Hyper-V VM on ARM64, "COM1" refers to a virtual PL011 UART that Hyper-V provides to the guest.
Set-VMFirmware -VMName <vm> -console COM1, adds a ACPI table for SPCR. After which using Set-VMComPort
we can access virtual serial console using putty.
In case of FreeBSD 13, enabling COM1 and setting VMComPort, make FreeBSD boot stuck during comconsole initialization itself
and no logs come in both vmconnect.exe and putty. So disabling the comconsole from the consoles list, till arm64 hyper-v is supported
allows the boot to progress and boot_serial/ boot_multicons to work for kernel log in putty and vmconnect.exe.
Comment Actions
It seems the latest version of the patch only contains the loader modification part. Does it needs to be submitted in another change? Currently all the vmbus codes are gone, maybe that's the main part of this review?
Comment Actions
Updated loader.mk for the LOADER_AARCH64_HYPERV_SUPPORT.
Yes, all the vmbus related changes will be submitted as part of another review, as this is not related with vmbus but with ARM64 enablement of FreeBSD on Hyper-V, so put it as a separate review.
Once this and D36256 are approved, will put vmbus changes on review.
Comment Actions
Please just create multiple reviews. It's a lot easier to review several smallish reviews at once rather than doing them serially. We'll likely get it into the tree faster if you do this.
| stand/loader.mk | ||
|---|---|---|
| 105 | Is there some way we could block the UEFI Firmware versions / names / etc that hang instead? | |
| stand/efi/loader/conf.c | ||
|---|---|---|
| 91 | Where does it hung, while in loader or in kernel? | |
Comment Actions
Bug 266248 - efi comconsole does not work with ARM64 Hyper-V
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=266248
Comment Actions
Setting other values than default values cause hang during comc_setup. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=266248
Comment Actions
small nits still, but good otherwise, I assume, this code does work as expected now?
| stand/efi/loader/efiserialio.c | ||
|---|---|---|
| 264 ↗ | (On Diff #114192) | As COMSPEED is replaced by 0, it is not used any more and can be deleted from beginning of the file. (in the future, please set up phabricator with more context, git arc can do it for you - see ./tools/tools/git). |
| 265 ↗ | (On Diff #114192) | I think, the comments is a bit misleading here; perhaps use like /* default XXX from firmware */ -- we are not setting default port to 0, but we are asking to use value from firmware. Same for lines below. |
Comment Actions
Yes it is not hanging at loader prompt when we are using virtual serial console, and also set console="comconsole"
is working.
Comment Actions
Getting rid of default 9600 baud serial console is a good thing, we'll need to make sure this is widely documented in release notes etc.
See also D36295
Comment Actions
Please note, this code is specific to non-x86 UEFI code
| stand/efi/loader/efiserialio.c | ||
|---|---|---|
| 263 ↗ | (On Diff #114194) | Actually, I have just found an qemu-system-aarch64 varaint, which did choke on these values... I did add a change to fight with it -- after we get handle to sio, we can set the values from sio->Mode, also add defaults for timeout and receivefifodepth, so we can use those values in comc_setup() (instead of 0's). |
| stand/efi/loader/efiserialio.c | ||
|---|---|---|
| 263 ↗ | (On Diff #114194) | Can you please share me the patch for the same, I can then test it on Hyper-V ARM64. |
| stand/efi/loader/efiserialio.c | ||
|---|---|---|
| 263 ↗ | (On Diff #114194) | This will take a bit of time - I'll try to pull it out tomorrow or so. |
Comment Actions
@tsoome Can you please share the patch, would like to test it on Hyper-V arm64 and x86. Thanks.