Re: git: 9729f076e4d9 - main - arm64: Hyper-V: enablement for ARM64 in Hyper-V (Part 3, final)

On Thu, Oct 27, 2022 at 8:54 AM Wei Hu <whu@freebsd.org> wrote:
>
> The branch main has been updated by whu:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=9729f076e4d93c5a37e78d427bfe0f1ab99bbcc6
>
> commit 9729f076e4d93c5a37e78d427bfe0f1ab99bbcc6
> Author:     Souradeep Chakrabarti <schakrabarti@microsoft.com>
> AuthorDate: 2022-10-27 13:46:08 +0000
> Commit:     Wei Hu <whu@FreeBSD.org>
> CommitDate: 2022-10-27 13:53:22 +0000
>
>     arm64: Hyper-V: enablement for ARM64 in Hyper-V (Part 3, final)
>
>     This is the last part for ARM64 Hyper-V enablement. This includes
>     commone files and make file changes to enable the ARM64 FreeBSD
>     guest on Hyper-V. With this patch, it should be able to build
>     the ARM64 image and install it on Hyper-V.
>

Hi,

First off- thanks for doing this work! I can't seem to boot a -CURRENT
image under Hyper-V on a Volterra machine, seemingly due to vmbus. It
stalls right after "vmbus: the irq 18" but before emitting a version
number, I have some other comments here...

> [... snip ...]
> diff --git a/sys/dev/hyperv/vmbus/vmbus.c b/sys/dev/hyperv/vmbus/vmbus.c
> index b0cd750b26c8..f370f2a75b99 100644
> --- a/sys/dev/hyperv/vmbus/vmbus.c
> +++ b/sys/dev/hyperv/vmbus/vmbus.c
> [... snip ...]
> @@ -107,7 +113,7 @@ static uint32_t                     vmbus_get_vcpu_id_method(device_t bus,
>                                     device_t dev, int cpu);
>  static struct taskqueue                *vmbus_get_eventtq_method(device_t, device_t,
>                                     int);
> -#ifdef EARLY_AP_STARTUP
> +#if defined(EARLY_AP_STARTUP) || defined(__aarch64__)
>  static void                    vmbus_intrhook(void *);
>  #endif
>

My gut reaction to this is that this is a red flag. EARLY_AP_STARTUP
implies characteristics that aarch64 doesn't exhibit; it's hard to see
why this conditional is OK, or whether it's testing EARLY_AP_STARTUP
as a bad way to write __i386__ || __amd64__.

> [... snip ...]
> @@ -760,7 +736,7 @@ vmbus_synic_setup(void *xsc)
>
>         if (hyperv_features & CPUID_HV_MSR_VP_INDEX) {
>                 /* Save virtual processor id. */
> -               VMBUS_PCPU_GET(sc, vcpuid, cpu) = rdmsr(MSR_HV_VP_INDEX);
> +               VMBUS_PCPU_GET(sc, vcpuid, cpu) = RDMSR(MSR_HV_VP_INDEX);
>         } else {
>                 /* Set virtual processor id to 0 for compatibility. */
>                 VMBUS_PCPU_GET(sc, vcpuid, cpu) = 0;

This one, vmbus_synic_setup(), is invoked via vmbus_intrhook ->
vmbus_doattach -> smp_rendezvous. On !EARLY_AP_STARTUP (e.g.,
aarch64), SMP isn't functional in intrhooks and smp_rendezvous() will
just call vmbus_synic_setup() on the boot AP. There's nothing that
will initialize the pcpu data on every other AP, AFAICT.

That said, the !EARLY_AP_STARTUP path is also wrong. Quoting lines
that weren't in this e-mail from vmbus_attach():

1527 #else   /* !EARLY_AP_STARTUP */
1528         /*
1529          * If the system has already booted and thread
1530          * scheduling is possible indicated by the global
1531          * cold set to zero, we just call the driver
1532          * initialization directly.
1533          */
1534         if (!cold)
1535                 vmbus_doattach(vmbus_sc);
1536 #endif /* EARLY_AP_STARTUP  and aarch64 */

The two immediate issues I see is that in a device_attach, SMP won't
be started. It's also going to be before SI_SUB_CONFIGURE, so `cold`
will never be 0 here -- this is effectively dead code, and if it
weren't dead code it would suffer the same problem as above where
other APs aren't started yet so we won't set their pcpu data. This is
OK, though, because then we have this sysinit later on that does the
same thing for !EARLY_AP_STARTUP && !__aarch64__`, but that should
really just be `!EARLY_AP_STARTUP` as aarch64 also needs to invoke
vmbus_doattach() at SI_SUB_SMP (much later than SI_SUB_DRIVERS).

Thanks,

Kyle Evans