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 <[email protected]>
Co-developed-by: Max Stolze <[email protected]>
Signed-off-by: Max Stolze <[email protected]>
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
From: Stefan Eschenbacher <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Max Stolze <[email protected]>
> Signed-off-by: Max Stolze <[email protected]>
>
> 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
Fixes a misleading typo in the comment for the macro MAX_NUM_CHANNELS,
where two digits have been twisted.
Signed-off-by: Stefan Eschenbacher <[email protected]>
Co-developed-by: Max Stolze <[email protected]>
Signed-off-by: Max Stolze <[email protected]>
---
drivers/hv/hyperv_vmbus.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index edeee1c0497d..52dcfa1c17ef 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -187,7 +187,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
u64 *requestid, bool raw);
/*
- * The Maximum number of channels (16348) is determined by the size of the
+ * The Maximum number of channels (16384) is determined by the size of the
* interrupt page, which is HV_HYP_PAGE_SIZE. 1/2 of HV_HYP_PAGE_SIZE is to
* send endpoint interrupts, and the other is to receive endpoint interrupts.
*/
--
2.20.1
The default number of vmbus channels (macro
MAX_NUM_CHANNELS_SUPPORTED_DEFAULT) is made configurable through the new
Kconfig option HYPERV_VMBUS_DEFAULT_CHANNELS.
Signed-off-by: Stefan Eschenbacher <[email protected]>
Co-developed-by: Max Stolze <[email protected]>
Signed-off-by: Max Stolze <[email protected]>
---
drivers/hv/Kconfig | 13 +++++++++++++
drivers/hv/hyperv_vmbus.h | 2 +-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 79e5356a737a..40bee5b05ccd 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -26,4 +26,17 @@ config HYPERV_BALLOON
help
Select this option to enable Hyper-V Balloon driver.
+config HYPERV_VMBUS_DEFAULT_CHANNELS
+ int "Default number of VMBus channels (as multiple of 32)"
+ range 0 512
+ default 8
+ depends on HYPERV
+ help
+ Specify the default number of VMBus channels here as nth multiple of
+ 32. The value must be equal to or less than 512.
+ The default is 8 meaning the number of channels is 8 * 32 = 256.
+ A different value can be set when loading the hv_vmbus module by
+ setting the parameter max_num_channels_supported directly to the
+ number of channels and not as a multiple of 32.
+
endmenu
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 52dcfa1c17ef..47af2c76ce57 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -194,7 +194,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
#define MAX_NUM_CHANNELS ((HV_HYP_PAGE_SIZE >> 1) << 3)
/* The value here must be in multiple of 32 */
-#define MAX_NUM_CHANNELS_SUPPORTED_DEFAULT 256
+#define MAX_NUM_CHANNELS_SUPPORTED_DEFAULT (CONFIG_HYPERV_VMBUS_DEFAULT_CHANNELS * 32)
extern uint max_num_channels_supported;
#define MAX_CHANNEL_RELIDS \
--
2.20.1
To make the number of supported channels configurable, this patch
introduces 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.
Signed-off-by: Stefan Eschenbacher <[email protected]>
Co-developed-by: Max Stolze <[email protected]>
Signed-off-by: Max Stolze <[email protected]>
---
drivers/hv/hyperv_vmbus.h | 6 +++---
drivers/hv/vmbus_drv.c | 20 +++++++++++++++++++-
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 02f3e8988836..edeee1c0497d 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -194,11 +194,11 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
#define MAX_NUM_CHANNELS ((HV_HYP_PAGE_SIZE >> 1) << 3)
/* The value here must be in multiple of 32 */
-/* TODO: Need to make this configurable */
-#define MAX_NUM_CHANNELS_SUPPORTED 256
+#define MAX_NUM_CHANNELS_SUPPORTED_DEFAULT 256
+extern uint max_num_channels_supported;
#define MAX_CHANNEL_RELIDS \
- max(MAX_NUM_CHANNELS_SUPPORTED, HV_EVENT_FLAGS_COUNT)
+ max((ulong) max_num_channels_supported, (ulong) HV_EVENT_FLAGS_COUNT)
enum vmbus_connect_state {
DISCONNECTED,
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 502f8cd95f6d..8f1c8a606b4a 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -48,6 +48,11 @@ static int hyperv_cpuhp_online;
static void *hv_panic_page;
+uint max_num_channels_supported = MAX_NUM_CHANNELS_SUPPORTED_DEFAULT;
+module_param(max_num_channels_supported, uint, 0);
+MODULE_PARM_DESC(max_num_channels_supported,
+ "Number of supported vmbus channels. Must be a multiple of 32 and equal to or less than 16348");
+
/* Values parsed from ACPI DSDT */
static int vmbus_irq;
int vmbus_interrupt;
@@ -1220,7 +1225,7 @@ static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu)
u32 maxbits, relid;
if (vmbus_proto_version < VERSION_WIN8) {
- maxbits = MAX_NUM_CHANNELS_SUPPORTED;
+ maxbits = max_num_channels_supported;
recv_int_page = vmbus_connection.recv_int_page;
} else {
/*
@@ -2620,6 +2625,17 @@ static int __init hv_acpi_init(void)
if (!hv_is_hyperv_initialized())
return -ENODEV;
+ // Check if max_num_channels_supported is a multiple of 32 and smaller MAX_NUM_CHANNELS
+ if (max_num_channels_supported % 32 != 0) {
+ pr_err("max_num_channels_supported is %u, but must be a multiple of 32\n",
+ max_num_channels_supported);
+ return -EINVAL;
+ } else if (max_num_channels_supported > MAX_NUM_CHANNELS) {
+ pr_err("max_num_channels_supported is %u which exceeds maximum of %lu\n",
+ max_num_channels_supported, MAX_NUM_CHANNELS);
+ return -ERANGE;
+ }
+
init_completion(&probe_event);
/*
@@ -2641,6 +2657,8 @@ static int __init hv_acpi_init(void)
if (ret)
goto cleanup;
+ pr_info("Supporting up to %u channels\n", max_num_channels_supported);
+
hv_setup_kexec_handler(hv_kexec_handler);
hv_setup_crash_handler(hv_crash_handler);
--
2.20.1
On 05/12/2020 19:27, Michael Kelley wrote:
> From: Stefan Eschenbacher <[email protected]>
>>
>> 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 <[email protected]>
>> Co-developed-by: Max Stolze <[email protected]>
>> Signed-off-by: Max Stolze <[email protected]>
>>
>> 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
From: Max Stolze <[email protected]> Sent: Saturday, December 5, 2020 12:32 PM
>
> On 05/12/2020 19:27, Michael Kelley wrote:
> > From: Stefan Eschenbacher <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Co-developed-by: Max Stolze <[email protected]>
> >> Signed-off-by: Max Stolze <[email protected]>
> >>
> >> 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
Removes an obsolete TODO in the VMBus module and fixes a misleading typo
in the comment for the macro MAX_NUM_CHANNELS, where two digits have been
twisted.
Signed-off-by: Stefan Eschenbacher <[email protected]>
Co-developed-by: Max Stolze <[email protected]>
Signed-off-by: Max Stolze <[email protected]>
---
drivers/hv/hyperv_vmbus.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 02f3e8988836..9416e09ebd58 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -187,14 +187,13 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
u64 *requestid, bool raw);
/*
- * The Maximum number of channels (16348) is determined by the size of the
+ * The Maximum number of channels (16384) is determined by the size of the
* interrupt page, which is HV_HYP_PAGE_SIZE. 1/2 of HV_HYP_PAGE_SIZE is to
* send endpoint interrupts, and the other is to receive endpoint interrupts.
*/
#define MAX_NUM_CHANNELS ((HV_HYP_PAGE_SIZE >> 1) << 3)
/* The value here must be in multiple of 32 */
-/* TODO: Need to make this configurable */
#define MAX_NUM_CHANNELS_SUPPORTED 256
#define MAX_CHANNEL_RELIDS \
--
2.20.1
On Sun, Dec 06, 2020 at 11:48:50AM +0100, Stefan Eschenbacher wrote:
> Removes an obsolete TODO in the VMBus module and fixes a misleading typo
> in the comment for the macro MAX_NUM_CHANNELS, where two digits have been
> twisted.
>
> Signed-off-by: Stefan Eschenbacher <[email protected]>
> Co-developed-by: Max Stolze <[email protected]>
> Signed-off-by: Max Stolze <[email protected]>
Applied to hyperv-next. Thanks.
Wei.