linux-kernel - RE: [PATCH] drivers/hv: select PCI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <SN6PR02MB4157BAAEB938E7E4E849DC7CD4EC2@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Mon, 27 Jan 2025 21:02:22 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Hamza Mahfooz <hamzamahfooz@...ux.microsoft.com>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>
CC: "stable@...r.kernel.org" <stable@...r.kernel.org>, Wei Liu
	<wei.liu@...nel.org>, "K. Y. Srinivasan" <kys@...rosoft.com>, Haiyang Zhang
	<haiyangz@...rosoft.com>, Dexuan Cui <decui@...rosoft.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] drivers/hv: select PCI_HYPERV if PCI is enabled

From: Hamza Mahfooz <hamzamahfooz@...ux.microsoft.com> Sent: Monday, January 27, 2025 10:10 AM
> 
> We should select PCI_HYPERV here, otherwise it's possible for devices to
> not show up as expected, at least not in an orderly manner.

The commit message needs more precision:  What does "not show up"
mean, and what does "not in an orderly manner" mean?  And "it's possible"
is vague -- can you be more specific about the conditions?  Also, avoid
the use of personal pronouns like "we".

But the commit message notwithstanding, I don't think this is change
that should be made. CONFIG_PCI_HYPERV refers to the VMBus device
driver for handling vPCI (a.k.a PCI pass-thru) devices. It's perfectly
possible and normal for a VM on Hyper-V to not have any such devices,
in which case the driver isn't needed and should not be forced to be
included. (See Documentation/virt/hyperv/vpci.rst for more on vPCI
devices.)

There are other VMBus device drivers:  storvsc, netvsc, the Hyper-V
frame buffer driver, the "util" drivers for shutdown, KVP, etc., and more.
These each have their own CONFIG_* entry, and current practice
doesn't select them when CONFIG_HYPERV is set. I don't see a reason
that the vPCI driver should be handled differently.

Also, different distro vendors take different approaches as to whether
these drivers are built as modules, or as built-in to their kernel images.
I'm not sure what the Kconfig tool does when a SELECT statement identifies
a tri-state setting. Since CONFIG_HYPERV is tri-state, does the target of
the SELECT get the same tri-state value as CONFIG_HYPERV? Again,
that may not be what distro vendors want. They may choose to have
some of the VMBus drivers built-in and others built as modules. Distro
vendors (and anyone doing a custom kernel build) should be allowed
to make their choices just like for any other drivers.

If you've come across a situation these considerations don't apply
or are problematic, provide more details. That's what a good commit
message should do -- be convincing as to *why* the change should
be made! :-)

Michael

> 
> Cc: stable@...r.kernel.org
> Cc: Wei Liu <wei.liu@...nel.org>
> Signed-off-by: Hamza Mahfooz <hamzamahfooz@...ux.microsoft.com>
> ---
>  drivers/hv/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> index 862c47b191af..6ee75b3f0fa6 100644
> --- a/drivers/hv/Kconfig
> +++ b/drivers/hv/Kconfig
> @@ -9,6 +9,7 @@ config HYPERV
>  	select PARAVIRT
>  	select X86_HV_CALLBACK_VECTOR if X86
>  	select OF_EARLY_FLATTREE if OF
> +	select PCI_HYPERV if PCI
>  	help
>  	  Select this option to run Linux as a Hyper-V client operating
>  	  system.
> --
> 2.47.1
>