Date: Wed, 19 Apr 2023 00:29:51 -0500 From: Kyle Evans <kevans@freebsd.org> To: Wei Hu <whu@freebsd.org>, Souradeep Chakrabarti <schakrabarti@microsoft.com> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 9729f076e4d9 - main - arm64: Hyper-V: enablement for ARM64 in Hyper-V (Part 3, final) Message-ID: <CACNAnaEwghH9T0xpLpDhjZng50RFggHabgzgJoGczMUSL-KeLQ@mail.gmail.com> In-Reply-To: <202210271354.29RDsUoH077155@gitrepo.freebsd.org> References: <202210271354.29RDsUoH077155@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Oct 27, 2022 at 8:54=E2=80=AFAM Wei Hu <whu@freebsd.org> wrote: > > The branch main has been updated by whu: > > URL: https://cgit.FreeBSD.org/src/commit/?id=3D9729f076e4d93c5a37e78d427b= fe0f1ab99bbcc6 > > 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) =3D rdmsr(MSR_HV_VP_INDEX= ); > + VMBUS_PCPU_GET(sc, vcpuid, cpu) =3D RDMSR(MSR_HV_VP_INDEX= ); > } else { > /* Set virtual processor id to 0 for compatibility. */ > VMBUS_PCPU_GET(sc, vcpuid, cpu) =3D 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACNAnaEwghH9T0xpLpDhjZng50RFggHabgzgJoGczMUSL-KeLQ>