⚙ D36052 Enabling FreeBSD on ARM64 Hyper-V

schakrabarti_microsoft.com created this revision.Aug 5 2022, 10:46 PM

Herald added a reviewer: manu. · View Herald TranscriptAug 5 2022, 10:46 PM

Herald added a reviewer: manu. · View Herald Transcript

Herald added subscribers: dab, bcran, emaste and 2 others. · View Herald Transcript

schakrabarti_microsoft.com requested review of this revision.Aug 5 2022, 10:46 PM

emaste added a subscriber: imp.Aug 6 2022, 1:27 AM

emaste added inline comments.

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 ↗	(On Diff #108915)	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?

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 ↗	(On Diff #108915)	This seems wrong, and the #define is weird too... So I'd like more details... Because we don't build stand with that define, as far as I know, normally.
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.

schakrabarti_microsoft.com added inline comments.Aug 6 2022, 6:31 AM

stand/efi/loader/conf.c
91 ↗	(On Diff #108915)	This seems wrong, and the #define is weird too... So I'd like more details... Because we don't build stand with that define, as far as I know, normally. Device "ttyAMA" refers to a UART with hardware part # PL011. It's a UART that is specific to ARM processors. Linux has a driver for this UART at drivers/tty/serial/amba-pl011.c. 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. Current EFI boot loader comconsole implementation is not supporting ttyAMA but causing a hung during probe. Until the time FreeBSD boot loader comconsole is supporting ttyAMA, we need to avoid using comconsole for Hyper-V in ARM64.

kpraveen.lkml_gmail.com added a subscriber: kpraveen.lkml_gmail.com.Aug 6 2022, 7:34 AM

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?

imp added inline comments.Aug 6 2022, 10:40 PM

stand/efi/loader/conf.c
91 ↗	(On Diff #108915)	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)

schakrabarti_microsoft.com edited the summary of this revision. (Show Details)Aug 7 2022, 6:42 AM

schakrabarti_microsoft.com added inline comments.

stand/efi/loader/conf.c
91 ↗	(On Diff #108915)	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 in loader.efi. Once that support is there, we can simply remove it. If you have some other suggestions, please let me know.
sys/arm/conf/GENERIC
285 ↗	(On Diff #108915)	No, will remove it. Thanks for pointing.

gbe added inline comments.Aug 7 2022, 10:15 AM

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.

lwhsu added a subscriber: lwhsu.Aug 7 2022, 5:28 PM

lwhsu added inline comments.Aug 7 2022, 5:37 PM

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` ?

In D36052#818681, @gbe wrote:
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?

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.

I have removed storvsc changes from here, as it will be reviewed through a separate bug. Also addressed the comments.

schakrabarti_microsoft.com marked 7 inline comments as done.Aug 8 2022, 9:29 AM

Fixed one comment and header inclusion.

fixed the diff

schakrabarti_microsoft.com marked an inline comment as done.Aug 8 2022, 11:34 AM

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.

Herald added a reviewer: manu. · View Herald TranscriptAug 18 2022, 11:47 AM

Updated loader.mk for the LOADER_AARCH64_HYPERV_SUPPORT.

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?

Updated loader.mk for the LOADER_AARCH64_HYPERV_SUPPORT.

In D36052#825458, @lwhsu wrote:

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?

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.

In D36052#825525, @schakrabarti_microsoft.com wrote:

Updated loader.mk for the LOADER_AARCH64_HYPERV_SUPPORT.

In D36052#825458, @lwhsu wrote:

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?

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.

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 ↗	(On Diff #109540)	Is there some way we could block the UEFI Firmware versions / names / etc that hang instead? Otherwise this will be a noop except for people that build their own boot loaders. People that try 'stock' FreeBSD on Hyper V and hit the hang will have a bad experience.

tsoome added a subscriber: tsoome.Aug 31 2022, 12:01 PM

tsoome added inline comments.

stand/efi/loader/conf.c
91 ↗	(On Diff #108915)	Where does it hung, while in loader or in kernel?

Bug 266248 - efi comconsole does not work with ARM64 Hyper-V
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=266248

Setting other values than default values cause hang during comc_setup. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=266248

schakrabarti_microsoft.com added a reviewer: tsoome.Dec 15 2022, 7:47 PM

schakrabarti_microsoft.com edited the summary of this revision. (Show Details)

In D36052#857682, @schakrabarti_microsoft.com wrote:

Setting other values than default values cause hang during comc_setup. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=266248

It seems, you got some extra files included?

schakrabarti_microsoft.com updated this revision to Diff 114192.Dec 16 2022, 5:50 AM

In D36052#857698, @tsoome wrote:

In D36052#857682, @schakrabarti_microsoft.com wrote:

Setting other values than default values cause hang during comc_setup. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=266248

It seems, you got some extra files included?

Fixed it. Thanks for pointing.

small nits still, but good otherwise, I assume, this code does work as expected now?

stand/efi/loader/efiserialio.c
264	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	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.

schakrabarti_microsoft.com updated this revision to Diff 114194.Dec 16 2022, 9:12 AM

In D36052#857738, @tsoome wrote:

small nits still, but good otherwise, I assume, this code does work as expected now?

Yes it is not hanging at loader prompt when we are using virtual serial console, and also set console="comconsole"
is working.

thanks!

This revision is now accepted and ready to land.Dec 16 2022, 9:37 AM

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

In D36052#857841, @emaste wrote:

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

Please note, this code is specific to non-x86 UEFI code

stand/efi/loader/efiserialio.c
263	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).

schakrabarti_microsoft.com added inline comments.Jan 24 2023, 10:13 AM

stand/efi/loader/efiserialio.c
263	Can you please share me the patch for the same, I can then test it on Hyper-V ARM64.

tsoome added inline comments.Jan 24 2023, 4:57 PM

stand/efi/loader/efiserialio.c
263	This will take a bit of time - I'll try to pull it out tomorrow or so.

@tsoome Can you please share the patch, would like to test it on Hyper-V arm64 and x86. Thanks.

Committed as rGa1f8a0c793c67ab5854035e017f34d3d016b6d0d

Enabling FreeBSD on ARM64 Hyper-V
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 114194

stand/efi/loader/efiserialio.c

Enabling FreeBSD on ARM64 Hyper-VClosedPublicActions