Message-ID: <Z5f94J7rzSC-TyIB@hm-sls2>
Date: Mon, 27 Jan 2025 16:42:56 -0500
From: Hamza Mahfooz <hamzamahfooz@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.com>
Cc: "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"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

On Mon, Jan 27, 2025 at 09:02:22PM +0000, Michael Kelley wrote:
> 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.)

Ya, we ran into an issue where CONFIG_NVME_CORE=y and
CONFIG_PCI_HYPERV=m caused the passed-through SSDs not to show up (i.e.
they aren't visible to userspace). I guess it's cause PCI_HYPERV has
to load in before the nvme stuff for that workload. So, I thought it was
reasonable to select PCI_HYPERV here to prevent someone else from
shooting themselves in the foot. Though, I guess it really it on the
distro guys to get that right.

> 
> 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
> > 
> 

Powered by blists - more mailing lists