Details

Reviewers

kevans
whu
schakrabarti_microsoft.com

Commits

rG62f9bcf2b427: Switch to contigmalloc in the Hyper-V code

Summary

In the Hyper-V drivers we need to allocate buffers shared between the
host and guest. This memory has been allocated with bus_dma, however
it doesn't use this correctly, e.g. it is missing calls to
bus_dmamap_sync. Along with this on arm64 we need this memory to be
mapped with the correct memory type that bus_dma may not use.

Switch to contigmalloc to allocate this memory as this will correctly
allocate cacheable memory.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 51630
Build 48521: arc lint + arc unit

Event Timeline

andrew created this revision.May 23 2023, 8:56 AM

Herald added a subscriber: imp. · View Herald TranscriptMay 23 2023, 8:56 AM

andrew requested review of this revision.May 23 2023, 8:56 AM

Harbormaster completed remote builds in B51630: Diff 122272.May 23 2023, 8:56 AM

andrew added reviewers: kevans, whu, schakrabarti_microsoft.com.May 23 2023, 8:57 AM

Tested under Hyper-V on a Windows Dev Kit 2023 with the patch in [1], still needs testing elsewhere.

[1] https://lists.freebsd.org/archives/dev-commits-src-all/2023-April/025804.html

schakrabarti_microsoft.com added a comment.May 23 2023, 9:18 AM

This comment was removed by schakrabarti_microsoft.com.

schakrabarti_microsoft.com added inline comments.May 23 2023, 9:36 AM

sys/dev/hyperv/netvsc/if_hn.c
5489	where is check for contigmalloc() failure? Missing in all the places
sys/dev/hyperv/vmbus/vmbus.c
749	Please use a Macro here instead 12 bit shift. Same for the code below.

schakrabarti_microsoft.com added inline comments.May 23 2023, 9:38 AM

sys/dev/hyperv/vmbus/vmbus_chan.c
1633	Please handle null return scenarios.

Interesting, M_WAITOK has a slightly different meaning in contigmalloc to the main kernel malloc. In malloc with M_WAITOK the thread will sleep until it can allocate the requested memory. With contigmalloc it will try harder to allocate the memory, but can still fail after a few attempts.

Also if hyperv_dmamem_alloc() and hyperv_dmamem_free() is not in use, please remove them from vmbus. As they are dead code.

hyperv_dmamem_alloc is used in amd64 code. I wasn't planning on touching that as I am unable to easily test the change.

In D40227#915806, @andrew wrote:

hyperv_dmamem_alloc is used in amd64 code. I wasn't planning on touching that as I am unable to easily test the change.

Then it will create confusion in future, as having two different functions in same code base.
The current change not only scoped for arm64 only, all these are also part of amd64 code.
So either we should take care of the whole code, or only arm64 / amd64 specific area.

Restore NULL checks
Don't include hyperv_busdma.h when it's unneeded
Add a MSR_HV_SIEFP_PGMASK and MSR_HV_SIMP_PGMASK macro
Remove hyperv_dmamem_free as it's unused

Harbormaster completed remote builds in B51632: Diff 122275.May 23 2023, 11:32 AM

hyperv_dmamem_free() is removed, but hyperv_dmamem_alloc() stays, so what about the memory allocated by hyperv_dmamem_alloc()? How they will be freed?
Also it is not very consistent.
We need to do a thorough testing on Azure amd64 gen1 gen2 and arm64 with this patch.
Have you tested the change on Hyper-V x86 and arm64?

I will try to test the patch on Azure x86 gen1 and gen2 and arm64.
I guess by this weekend I can complete it.

The only remaining caller of hyperv_dmamem_alloc is to allocate hyperv_ref_tsc.tsc_ref. It doesn't have a call to hyperv_dmamem_free.

If possible please get the change thoroughly tested in Azure x86 platform or Hyper-V x86.

Move the last x86 caller of hyperv_dmamem_alloc

Harbormaster completed remote builds in B51679: Diff 122413.May 25 2023, 2:41 PM

lwhsu added a subscriber: lwhsu.May 25 2023, 2:46 PM

In D40227#916382, @schakrabarti_microsoft.com wrote:

If possible please get the change thoroughly tested in Azure x86 platform or Hyper-V x86.

Getting panic after installation :
cd0 at storvsc1 bus 0 scbus1 target 0 lun 0
cd0: <Msft Virtual DVD-ROM 1.0> Removable CD-ROM SPC-3 SCSI device
cd0: 300.000MB/s transfers
cd0: 819MB (419479 2048 byte sectors)
GEOM: new disk cd0
hvkvp0: chan8 revoked
hvkvp0: chan8 detached
hvkvp0: chan8 closed
panic: Assertion bt->bt_size == vmem_roundup_size(vm, size) || bt->bt_size - vmem_roundup_size(vm, size) <= vm->vm_quantum_mask failed at /datadrive/sandbox_30_05/src/sys/kern/subr_vmem.c:1492
cpuid = 3
time = 1685480882
KDB: stack backtrace:
db_trace_self() at db_trace_self
db_trace_self_wrapper() at db_trace_self_wrapper+0x30
vpanic() at vpanic+0x13c
panic() at panic+0x44
vmem_xfree() at vmem_xfree+0x350
contigfree() at contigfree+0x14
vmbus_chan_close_internal() at vmbus_chan_close_internal+0x204
vmbus_ic_detach() at vmbus_ic_detach+0x20
device_detach() at device_detach+0x1a8
device_delete_child() at device_delete_child+0x18
vmbus_delete_child() at vmbus_delete_child+0x28
vmbus_prichan_detach_task() at vmbus_prichan_detach_task+0x1c
taskqueue_run_locked() at taskqueue_run_locked+0x17c
taskqueue_thread_loop() at taskqueue_thread_loop+0xc8
fork_exit() at fork_exit+0x74
fork_trampoline() at fork_trampoline+0x14
KDB: enter: panic
[ thread pid 0 tid 100116 ]
Stopped at kdb_enter+0x44: str xzr, [x19, #3328]
db>