From: Max Stolze <max.stolze@xxxxxx> Sent: Saturday, December 5, 2020 12:32 PM > > On 05/12/2020 19:27, Michael Kelley wrote: > > From: Stefan Eschenbacher <stefan.eschenbacher@xxxxxx> > >> > >> According to the TODO comment in hyperv_vmbus.h the value in macro > >> MAX_NUM_CHANNELS_SUPPORTED should be configurable. The first patch > >> accomplishes that by introducting uint max_num_channels_supported as > >> module parameter. > >> Also macro MAX_NUM_CHANNELS_SUPPORTED_DEFAULT is introduced with > >> value 256, which is the currently used macro value. > >> MAX_NUM_CHANNELS_SUPPORTED was found and replaced in two locations. > >> > >> During module initialization sanity checks are applied which will result > >> in EINVAL or ERANGE if the given value is no multiple of 32 or larger than > >> MAX_NUM_CHANNELS. > >> > >> While testing, we found a misleading typo in the comment for the macro > >> MAX_NUM_CHANNELS which is fixed by the second patch. > >> > >> The third patch makes the added default macro configurable by > >> introduction and use of Kconfig parameter HYPERV_VMBUS_DEFAULT_CHANNELS. > >> Default value remains at 256. > >> > >> Two notes on these patches: > >> 1) With above patches it is valid to configure max_num_channels_supported > >> and MAX_NUM_CHANNELS_SUPPORTED_DEFAULT as 0. We simply don't know if that > >> is a valid value. Doing so while testing still left us with a working > >> Debian VM in Hyper-V on Windows 10. > >> 2) To set the Kconfig parameter the user currently has to divide the > >> desired default number of channels by 32 and enter the result of that > >> calculation. This way both known constraints we got from the comments in > >> the code are enforced. It feels a bit unintuitive though. We haven't found > >> Kconfig options to ensure that the value is a multiple of 32. So if you'd > >> like us to fix that we'd be happy for some input on how to settle it with > >> Kconfig. > >> > >> Signed-off-by: Stefan Eschenbacher <stefan.eschenbacher@xxxxxx> > >> Co-developed-by: Max Stolze <max.stolze@xxxxxx> > >> Signed-off-by: Max Stolze <max.stolze@xxxxxx> > >> > >> Stefan Eschenbacher (3): > >> drivers/hv: make max_num_channels_supported configurable > >> drivers/hv: fix misleading typo in comment > >> drivers/hv: add default number of vmbus channels to Kconfig > >> > >> drivers/hv/Kconfig | 13 +++++++++++++ > >> drivers/hv/hyperv_vmbus.h | 8 ++++---- > >> drivers/hv/vmbus_drv.c | 20 +++++++++++++++++++- > >> 3 files changed, 36 insertions(+), 5 deletions(-) > >> > >> -- > >> 2.20.1 > > > > Stefan -- this cover letter email came through, but it doesn't look like > > the individual patch emails did. So you might want to check your > > patch sending process. > > > > Thanks for your interest in this old "TODO" item. But let me provide some > > additional background. Starting in Windows 8 and Windows Server 2012, > > Hyper-V revised the mechanism by which channel interrupt notifications > > are made. The MAX_NUM_CHANNELS_SUPPORTED value is only used > > with Windows 7 and Windows Server 2008 R2, neither of which is officially > > supported any longer. See the code at the top of vmbus_chan_sched() where > > the VMBus protocol version is checked, and MAX_NUM_CHANNELS_SUPPORTED > > is used only when the protocol version indicates we're running on Windows 7 > > (or the equivalent Windows Server 2008 R2). > > > > Because the old mechanism was superseded, making the value configurable > > doesn't have any benefit. At some point, we will remove this old code path > > entirely, including the #define MAX_NUM_CHANNELS_SUPPORTED. The > > comment with the "TODO" could be removed, but other than that, I don't > > think we want to make these changes. > > > > Michael > > > > Hi Michael, > thanks for the insight and explanation. > It's a pity that the rest of the series did not make it trough. > However, given what you wrote it doesn't seem to be of > utmost importance. > > As irrelevant as it may be we'd still be glad to see the TODO gone. > Given that we found it by looking for things that can be done around > the kernel I don't see why somebody else would not find it while looking > for the same. > > Max The additional patches did finally show up -- about 45 minutes later. The same delay occurred in the patches appearing on lkml.org, so there must be have been some delay on the sending side. No matter. I would be fine if you want to submit a patch that removes the "TODO" and fixes the 16348 vs. 16384 typo in the comment for MAX_NUM_CHANNELS. Michael
