Page MenuHomeFreeBSD

Enabling FreeBSD on ARM64 Hyper-V
ClosedPublic

Authored by schakrabarti_microsoft.com on Aug 5 2022, 10:46 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 11, 3:31 PM
Unknown Object (File)
Thu, Apr 11, 2:52 PM
Unknown Object (File)
Thu, Apr 11, 11:41 AM
Unknown Object (File)
Tue, Apr 9, 4:16 PM
Unknown Object (File)
Tue, Apr 9, 5:56 AM
Unknown Object (File)
Mon, Apr 8, 5:01 PM
Unknown Object (File)
Mon, Apr 1, 5:17 PM
Unknown Object (File)
Mon, Apr 1, 2:47 PM

Details

Reviewers
whu
imp
manu
tsoome
Summary

This patch fixes efi serial console hang issue in ARM64 Hyper-V.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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.

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.

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 ↗(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 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.

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 ?

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.

Fixed one comment and header inclusion.

schakrabarti_microsoft.com edited the test plan for this revision. (Show Details)
schakrabarti_microsoft.com edited reviewers, added: imp; removed: manu.

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.

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.

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.

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?

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

It seems, you got some extra files included?

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.

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.

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

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).

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.

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.