RE: [EXTERNAL] Re: git: 9729f076e4d9 - main - arm64: Hyper-V: enablement for ARM64 in Hyper-V (Part 3, final)
- Reply: Souradeep Chakrabarti : "RE: [EXTERNAL] Re: git: 9729f076e4d9 - main - arm64: Hyper-V: enablement for ARM64 in Hyper-V (Part 3, final)"
- In reply to: Kyle Evans : "Re: git: 9729f076e4d9 - main - arm64: Hyper-V: enablement for ARM64 in Hyper-V (Part 3, final)"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Mon, 24 Apr 2023 12:54:23 UTC
>-----Original Message-----
>From: Kyle Evans <kevans@freebsd.org>
>Sent: Wednesday, April 19, 2023 11:00 AM
>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: [EXTERNAL] 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit
>>
>.freebsd.org%2Fsrc%2Fcommit%2F%3Fid%3D9729f076e4d93c5a37e78d427bfe0
>f1a
>>
>b99bbcc6&data=05%7C01%7Cschakrabarti%40microsoft.com%7C393e855f13c6
>49a
>>
>8883708db4097211e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C
>6381747
>>
>90106786449%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
>V2luMz
>>
>IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=huiaOBSB2Z
>QDn5Q
>> cw%2FLWnmI%2FqbbriQd7HkYtpxnGLGE%3D&reserved=0
>>
>> 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__.
[Souradeep] We had or'd the aarch64 here, as we wanted to defer the vmbus attachment
after gic and arm generic timer is attached. As we need PAUSE and DELAY for vmbus and hyper-v
drivers. For that we wanted to use vmbus_intrhook().
>
>> [... 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():
[Souradeep] Thanks! This is a really good catch, and yes, we are missing the smp initialization
during vmbus synic setup.
>
>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).
[Souradeep] I have tried to use the suggestion but in multi processor system
the boot is getting stuck at the end and "vmbus0: device scan, probe and attach done"
is not happening.
I am trying to figure out what is going wrong here, as the same change is working with single
cpu system. Any help or pointer will be really helpful.
For your reference I am attaching the diff here:
diff --git a/sys/dev/hyperv/vmbus/vmbus.c b/sys/dev/hyperv/vmbus/vmbus.c
index f370f2a75b99..254529764763 100644
--- a/sys/dev/hyperv/vmbus/vmbus.c
+++ b/sys/dev/hyperv/vmbus/vmbus.c
@@ -113,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);
-#if defined(EARLY_AP_STARTUP) || defined(__aarch64__)
+#if defined(EARLY_AP_STARTUP) //|| defined(__aarch64__)
static void vmbus_intrhook(void *);
#endif
@@ -1490,7 +1490,7 @@ vmbus_event_proc_dummy(struct vmbus_softc *sc __unused, int cpu __unused)
{
}
-#if defined(EARLY_AP_STARTUP) || defined(__aarch64__)
+#if defined(EARLY_AP_STARTUP) //|| defined(__aarch64__)
static void
vmbus_intrhook(void *xsc)
@@ -1519,7 +1519,7 @@ vmbus_attach(device_t dev)
*/
vmbus_sc->vmbus_event_proc = vmbus_event_proc_dummy;
-#if defined(EARLY_AP_STARTUP) || defined(__aarch64__)
+#if defined(EARLY_AP_STARTUP) //|| defined(__aarch64__)
/*
* Defer the real attach until the pause(9) works as expected.
*/
@@ -1533,8 +1533,8 @@ vmbus_attach(device_t dev)
* cold set to zero, we just call the driver
* initialization directly.
*/
- if (!cold)
- vmbus_doattach(vmbus_sc);
+ /*if (!cold)
+ vmbus_doattach(vmbus_sc);*/
#endif /* EARLY_AP_STARTUP and aarch64 */
return (0);
@@ -1580,7 +1580,7 @@ vmbus_detach(device_t dev)
return (0);
}
-#if !defined(EARLY_AP_STARTUP) && !defined(__aarch64__)
+#if !defined(EARLY_AP_STARTUP) //&& !defined(__aarch64__)
static void
vmbus_sysinit(void *arg __unused)
>
>Thanks,
>
>Kyle Evans