linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] virtio-blk: Fix kconfig option
@ 2012-09-03  4:41 Kent Overstreet
  2012-09-03  4:46 ` Kent Overstreet
  2012-09-04  6:23 ` Rusty Russell
  0 siblings, 2 replies; 21+ messages in thread
From: Kent Overstreet @ 2012-09-03  4:41 UTC (permalink / raw)
  To: rusty, mst; +Cc: virtualization, linux-kernel

CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
instead.

Signed-off-by: Kent Overstreet <koverstreet@google.com>
---
 drivers/block/Kconfig |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index a796407..d4e1d12 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -521,7 +521,8 @@ config XEN_BLKDEV_BACKEND
 
 config VIRTIO_BLK
 	tristate "Virtio block driver (EXPERIMENTAL)"
-	depends on EXPERIMENTAL && VIRTIO
+	select VIRTIO
+	depends on EXPERIMENTAL
 	---help---
 	  This is the virtual block driver for virtio.  It can be used with
           lguest or QEMU based VMMs (like KVM or Xen).  Say Y or M.
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-03  4:41 [PATCH] virtio-blk: Fix kconfig option Kent Overstreet
@ 2012-09-03  4:46 ` Kent Overstreet
  2012-09-04  6:23 ` Rusty Russell
  1 sibling, 0 replies; 21+ messages in thread
From: Kent Overstreet @ 2012-09-03  4:46 UTC (permalink / raw)
  To: rusty, mst; +Cc: virtualization, linux-kernel

Whoops, need VIRTIO_RING too

>From 79dc9ae40e40cefd6079e4197cac858a1d59e1d8 Mon Sep 17 00:00:00 2001
From: Kent Overstreet <koverstreet@google.com>
Date: Sun, 2 Sep 2012 21:44:37 -0700
Subject: [PATCH] virtio-blk: Fix kconfig option

CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
instead.

Signed-off-by: Kent Overstreet <koverstreet@google.com>
---
 drivers/block/Kconfig |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index a796407..40532b8 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -521,7 +521,9 @@ config XEN_BLKDEV_BACKEND
 
 config VIRTIO_BLK
 	tristate "Virtio block driver (EXPERIMENTAL)"
-	depends on EXPERIMENTAL && VIRTIO
+	select VIRTIO
+	select VIRTIO_RING
+	depends on EXPERIMENTAL
 	---help---
 	  This is the virtual block driver for virtio.  It can be used with
           lguest or QEMU based VMMs (like KVM or Xen).  Say Y or M.
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-03  4:41 [PATCH] virtio-blk: Fix kconfig option Kent Overstreet
  2012-09-03  4:46 ` Kent Overstreet
@ 2012-09-04  6:23 ` Rusty Russell
  2012-09-04 23:12   ` Kent Overstreet
                     ` (3 more replies)
  1 sibling, 4 replies; 21+ messages in thread
From: Rusty Russell @ 2012-09-04  6:23 UTC (permalink / raw)
  To: Kent Overstreet, mst; +Cc: virtualization, linux-kernel

Kent Overstreet <koverstreet@google.com> writes:

> CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> instead.

This is a slight mis-understanding.  It's supposed to be selected by
the particular driver, probably virtio_pci in your case.

Cheers,
Rusty.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-04  6:23 ` Rusty Russell
@ 2012-09-04 23:12   ` Kent Overstreet
  2012-09-05  4:22   ` Asias He
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 21+ messages in thread
From: Kent Overstreet @ 2012-09-04 23:12 UTC (permalink / raw)
  To: Rusty Russell; +Cc: mst, virtualization, linux-kernel

On Mon, Sep 3, 2012 at 11:23 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Kent Overstreet <koverstreet@google.com> writes:
>
>> CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
>> instead.
>
> This is a slight mis-understanding.  It's supposed to be selected by
> the particular driver, probably virtio_pci in your case.

Yes... VIRT_BLOCK isn't selecting it.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-04  6:23 ` Rusty Russell
  2012-09-04 23:12   ` Kent Overstreet
@ 2012-09-05  4:22   ` Asias He
  2012-09-05  5:48     ` Michael S. Tsirkin
  2012-09-05  6:12   ` Michael S. Tsirkin
  2012-09-06  7:41   ` Kent Overstreet
  3 siblings, 1 reply; 21+ messages in thread
From: Asias He @ 2012-09-05  4:22 UTC (permalink / raw)
  To: Rusty Russell; +Cc: Kent Overstreet, mst, linux-kernel, virtualization

On 09/04/2012 02:23 PM, Rusty Russell wrote:
> Kent Overstreet <koverstreet@google.com> writes:
> 
>> CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
>> instead.
> 
> This is a slight mis-understanding.  It's supposed to be selected by
> the particular driver, probably virtio_pci in your case.

virtio_mmio selects virtio as well.

drivers/virtio/Kconfig
config VIRTIO_MMIO
       select VIRTIO
config VIRTIO_PCI
       select VIRTIO

How about this:
config VIRTIO_BLK
       depends on EXPERIMENTAL && (VIRTIO_PCI || VIRTIO_MMIO)

-- 
Asias

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-05  4:22   ` Asias He
@ 2012-09-05  5:48     ` Michael S. Tsirkin
  2012-09-05  5:54       ` Asias He
  0 siblings, 1 reply; 21+ messages in thread
From: Michael S. Tsirkin @ 2012-09-05  5:48 UTC (permalink / raw)
  To: Asias He; +Cc: Rusty Russell, Kent Overstreet, linux-kernel, virtualization

On Wed, Sep 05, 2012 at 12:22:58PM +0800, Asias He wrote:
> On 09/04/2012 02:23 PM, Rusty Russell wrote:
> > Kent Overstreet <koverstreet@google.com> writes:
> > 
> >> CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> >> instead.
> > 
> > This is a slight mis-understanding.  It's supposed to be selected by
> > the particular driver, probably virtio_pci in your case.
> 
> virtio_mmio selects virtio as well.
> 
> drivers/virtio/Kconfig
> config VIRTIO_MMIO
>        select VIRTIO
> config VIRTIO_PCI
>        select VIRTIO
> 
> How about this:
> config VIRTIO_BLK
>        depends on EXPERIMENTAL && (VIRTIO_PCI || VIRTIO_MMIO)

Isn't this exactly what depends on VIRTIO achieves?

> -- 
> Asias

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-05  5:48     ` Michael S. Tsirkin
@ 2012-09-05  5:54       ` Asias He
  2012-09-05  6:04         ` Michael S. Tsirkin
  0 siblings, 1 reply; 21+ messages in thread
From: Asias He @ 2012-09-05  5:54 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Rusty Russell, Kent Overstreet, linux-kernel, virtualization

On 09/05/2012 01:48 PM, Michael S. Tsirkin wrote:
> On Wed, Sep 05, 2012 at 12:22:58PM +0800, Asias He wrote:
>> On 09/04/2012 02:23 PM, Rusty Russell wrote:
>>> Kent Overstreet <koverstreet@google.com> writes:
>>>
>>>> CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
>>>> instead.
>>>
>>> This is a slight mis-understanding.  It's supposed to be selected by
>>> the particular driver, probably virtio_pci in your case.
>>
>> virtio_mmio selects virtio as well.
>>
>> drivers/virtio/Kconfig
>> config VIRTIO_MMIO
>>        select VIRTIO
>> config VIRTIO_PCI
>>        select VIRTIO
>>
>> How about this:
>> config VIRTIO_BLK
>>        depends on EXPERIMENTAL && (VIRTIO_PCI || VIRTIO_MMIO)
> 
> Isn't this exactly what depends on VIRTIO achieves?

Yes. But this gives the information that virtio_blk depends on one of
the virtio transport: virito_pci or virtio_mmio.

-- 
Asias

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-05  5:54       ` Asias He
@ 2012-09-05  6:04         ` Michael S. Tsirkin
  0 siblings, 0 replies; 21+ messages in thread
From: Michael S. Tsirkin @ 2012-09-05  6:04 UTC (permalink / raw)
  To: Asias He; +Cc: Rusty Russell, Kent Overstreet, linux-kernel, virtualization

On Wed, Sep 05, 2012 at 01:54:07PM +0800, Asias He wrote:
> On 09/05/2012 01:48 PM, Michael S. Tsirkin wrote:
> > On Wed, Sep 05, 2012 at 12:22:58PM +0800, Asias He wrote:
> >> On 09/04/2012 02:23 PM, Rusty Russell wrote:
> >>> Kent Overstreet <koverstreet@google.com> writes:
> >>>
> >>>> CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> >>>> instead.
> >>>
> >>> This is a slight mis-understanding.  It's supposed to be selected by
> >>> the particular driver, probably virtio_pci in your case.
> >>
> >> virtio_mmio selects virtio as well.
> >>
> >> drivers/virtio/Kconfig
> >> config VIRTIO_MMIO
> >>        select VIRTIO
> >> config VIRTIO_PCI
> >>        select VIRTIO
> >>
> >> How about this:
> >> config VIRTIO_BLK
> >>        depends on EXPERIMENTAL && (VIRTIO_PCI || VIRTIO_MMIO)
> > 
> > Isn't this exactly what depends on VIRTIO achieves?
> 
> Yes. But this gives the information that virtio_blk depends on one of
> the virtio transport: virito_pci or virtio_mmio.

It can work with any transport.

> -- 
> Asias

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-04  6:23 ` Rusty Russell
  2012-09-04 23:12   ` Kent Overstreet
  2012-09-05  4:22   ` Asias He
@ 2012-09-05  6:12   ` Michael S. Tsirkin
  2012-09-06  1:46     ` Rusty Russell
  2012-09-06  7:41   ` Kent Overstreet
  3 siblings, 1 reply; 21+ messages in thread
From: Michael S. Tsirkin @ 2012-09-05  6:12 UTC (permalink / raw)
  To: Rusty Russell; +Cc: Kent Overstreet, virtualization, linux-kernel

On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> Kent Overstreet <koverstreet@google.com> writes:
> 
> > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > instead.
> 
> This is a slight mis-understanding.  It's supposed to be selected by
> the particular driver, probably virtio_pci in your case.
> 
> Cheers,
> Rusty.

Actually balloon selects VIRTIO, I think it's a bug.
Also isn't it time we dropped the experimental tag?
Leaving it in for now.

-->

virtio-balloon: dependency fix

Devices should depend on virtio, not select it.  It's supposed to be
selected by the particular driver, e.g. VIRTIO_PCI.
Make balloon depend on VIRTIO and EXPERIMENTAL
(to match description).

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

--

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index f38b17a..564bb64 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -27,8 +27,7 @@ config VIRTIO_PCI
 
 config VIRTIO_BALLOON
 	tristate "Virtio balloon driver (EXPERIMENTAL)"
-	select VIRTIO
-	select VIRTIO_RING
+	depends on EXPERIMENTAL && VIRTIO
 	---help---
 	 This driver supports increasing and decreasing the amount
 	 of memory within a KVM guest.


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-05  6:12   ` Michael S. Tsirkin
@ 2012-09-06  1:46     ` Rusty Russell
  0 siblings, 0 replies; 21+ messages in thread
From: Rusty Russell @ 2012-09-06  1:46 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Kent Overstreet, virtualization, linux-kernel

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
>> Kent Overstreet <koverstreet@google.com> writes:
>> 
>> > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
>> > instead.
>> 
>> This is a slight mis-understanding.  It's supposed to be selected by
>> the particular driver, probably virtio_pci in your case.
>> 
>> Cheers,
>> Rusty.
>
> Actually balloon selects VIRTIO, I think it's a bug.
> Also isn't it time we dropped the experimental tag?
> Leaving it in for now.

Thanks, I applied this, and killed the EXPERIMENTAL in a separate patch.

Cheers,
Rusty.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-04  6:23 ` Rusty Russell
                     ` (2 preceding siblings ...)
  2012-09-05  6:12   ` Michael S. Tsirkin
@ 2012-09-06  7:41   ` Kent Overstreet
  2012-09-06  8:44     ` Michael S. Tsirkin
  3 siblings, 1 reply; 21+ messages in thread
From: Kent Overstreet @ 2012-09-06  7:41 UTC (permalink / raw)
  To: Rusty Russell; +Cc: mst, virtualization, linux-kernel

On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> Kent Overstreet <koverstreet@google.com> writes:
> 
> > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > instead.
> 
> This is a slight mis-understanding.  It's supposed to be selected by
> the particular driver, probably virtio_pci in your case.

So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
should have that.

As is, VIRTIO_BLK just has:
	depends on EXPERIMENTAL && VIRTIO

which is flat out broken.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-06  7:41   ` Kent Overstreet
@ 2012-09-06  8:44     ` Michael S. Tsirkin
  2012-09-06  9:25       ` Kent Overstreet
  0 siblings, 1 reply; 21+ messages in thread
From: Michael S. Tsirkin @ 2012-09-06  8:44 UTC (permalink / raw)
  To: Kent Overstreet; +Cc: Rusty Russell, virtualization, linux-kernel

On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > Kent Overstreet <koverstreet@google.com> writes:
> > 
> > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > instead.
> > 
> > This is a slight mis-understanding.  It's supposed to be selected by
> > the particular driver, probably virtio_pci in your case.
> 
> So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> should have that.
> 
> As is, VIRTIO_BLK just has:
> 	depends on EXPERIMENTAL && VIRTIO
> 
> which is flat out broken.

I don't think anything is broken.
Can you show an example of a broken configuration?

-- 
MST

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-06  8:44     ` Michael S. Tsirkin
@ 2012-09-06  9:25       ` Kent Overstreet
  2012-09-06  9:49         ` Michael S. Tsirkin
  0 siblings, 1 reply; 21+ messages in thread
From: Kent Overstreet @ 2012-09-06  9:25 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Rusty Russell, virtualization, linux-kernel

On Thu, Sep 06, 2012 at 11:44:03AM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> > On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > > Kent Overstreet <koverstreet@google.com> writes:
> > > 
> > > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > > instead.
> > > 
> > > This is a slight mis-understanding.  It's supposed to be selected by
> > > the particular driver, probably virtio_pci in your case.
> > 
> > So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> > should have that.
> > 
> > As is, VIRTIO_BLK just has:
> > 	depends on EXPERIMENTAL && VIRTIO
> > 
> > which is flat out broken.
> 
> I don't think anything is broken.
> Can you show an example of a broken configuration?

Do you not understand the difference between depends an selects? Or did
you not read my original mail?

Flip off everything in drivers -> virtio

Now go to drivers -> block and try to turn on virtio-blk.

It's not listed!

Now go back to drivers -> virtio and turn on (randomly) balloon.

Go back to drivers -> block, and now you can turn on virtio-blk!

Do you see what's wrong with this picture?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-06  9:25       ` Kent Overstreet
@ 2012-09-06  9:49         ` Michael S. Tsirkin
  2012-09-06 10:02           ` Kent Overstreet
  0 siblings, 1 reply; 21+ messages in thread
From: Michael S. Tsirkin @ 2012-09-06  9:49 UTC (permalink / raw)
  To: Kent Overstreet; +Cc: Rusty Russell, virtualization, linux-kernel

On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
> On Thu, Sep 06, 2012 at 11:44:03AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> > > On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > > > Kent Overstreet <koverstreet@google.com> writes:
> > > > 
> > > > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > > > instead.
> > > > 
> > > > This is a slight mis-understanding.  It's supposed to be selected by
> > > > the particular driver, probably virtio_pci in your case.
> > > 
> > > So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> > > should have that.
> > > 
> > > As is, VIRTIO_BLK just has:
> > > 	depends on EXPERIMENTAL && VIRTIO
> > > 
> > > which is flat out broken.
> > 
> > I don't think anything is broken.
> > Can you show an example of a broken configuration?
> 
> Do you not understand the difference between depends an selects?
> Or did you not read my original mail?
> Flip off everything in drivers -> virtio
> 
> Now go to drivers -> block and try to turn on virtio-blk.
> 
> It's not listed!

Yes. Because you disabled all virtio backends.
It does not make sense to have any frontends.

> Now go back to drivers -> virtio and turn on (randomly) balloon.
> 
> Go back to drivers -> block, and now you can turn on virtio-blk!
> 
> Do you see what's wrong with this picture?

Yes. You got unlucky with your random guess.
It's a bug in balloon kconfig: it should not
select virtio.
I sent a patch to fix that yesterday.

-- 
MST

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-06  9:49         ` Michael S. Tsirkin
@ 2012-09-06 10:02           ` Kent Overstreet
  2012-09-06 10:18             ` Michael S. Tsirkin
  2012-09-06 23:40             ` Rusty Russell
  0 siblings, 2 replies; 21+ messages in thread
From: Kent Overstreet @ 2012-09-06 10:02 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Rusty Russell, virtualization, linux-kernel

On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
> > On Thu, Sep 06, 2012 at 11:44:03AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> > > > On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > > > > Kent Overstreet <koverstreet@google.com> writes:
> > > > > 
> > > > > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > > > > instead.
> > > > > 
> > > > > This is a slight mis-understanding.  It's supposed to be selected by
> > > > > the particular driver, probably virtio_pci in your case.
> > > > 
> > > > So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> > > > should have that.
> > > > 
> > > > As is, VIRTIO_BLK just has:
> > > > 	depends on EXPERIMENTAL && VIRTIO
> > > > 
> > > > which is flat out broken.
> > > 
> > > I don't think anything is broken.
> > > Can you show an example of a broken configuration?
> > 
> > Do you not understand the difference between depends an selects?
> > Or did you not read my original mail?
> > Flip off everything in drivers -> virtio
> > 
> > Now go to drivers -> block and try to turn on virtio-blk.
> > 
> > It's not listed!
> 
> Yes. Because you disabled all virtio backends.
> It does not make sense to have any frontends.

How's a user - or even another kernel developer who isn't familiar with
virtio - supposed to know that?

I still don't know what exactly a virtio backend is - the term isn't
even mentioned anywhere that I've seen.

Whatever it is though virtio-blk should be depending on _that_, not a
config option that _isn't exposed in the menu_!

> > Now go back to drivers -> virtio and turn on (randomly) balloon.
> > 
> > Go back to drivers -> block, and now you can turn on virtio-blk!
> > 
> > Do you see what's wrong with this picture?
> 
> Yes. You got unlucky with your random guess.
> It's a bug in balloon kconfig: it should not
> select virtio.
> I sent a patch to fix that yesterday.

Then it's also a bug in the comments at the top of
drivers/virtio/Kconfig.

And besides that, how the _hell_ is a user supposed to know to turn on
VIRTIO_PCI before VIRTIO_BLK? It's not documented anywhere (if that is
what's supposed to happen! I still don't know) and even if it was
documented, having one kconfig option depend on something that's exposed
in a _completely different menu_ is just made of fail.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-06 10:02           ` Kent Overstreet
@ 2012-09-06 10:18             ` Michael S. Tsirkin
  2012-09-06 10:31               ` Kent Overstreet
  2012-09-06 23:40             ` Rusty Russell
  1 sibling, 1 reply; 21+ messages in thread
From: Michael S. Tsirkin @ 2012-09-06 10:18 UTC (permalink / raw)
  To: Kent Overstreet; +Cc: Rusty Russell, virtualization, linux-kernel

On Thu, Sep 06, 2012 at 03:02:48AM -0700, Kent Overstreet wrote:
> On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
> > > On Thu, Sep 06, 2012 at 11:44:03AM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> > > > > On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > > > > > Kent Overstreet <koverstreet@google.com> writes:
> > > > > > 
> > > > > > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > > > > > instead.
> > > > > > 
> > > > > > This is a slight mis-understanding.  It's supposed to be selected by
> > > > > > the particular driver, probably virtio_pci in your case.
> > > > > 
> > > > > So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> > > > > should have that.
> > > > > 
> > > > > As is, VIRTIO_BLK just has:
> > > > > 	depends on EXPERIMENTAL && VIRTIO
> > > > > 
> > > > > which is flat out broken.
> > > > 
> > > > I don't think anything is broken.
> > > > Can you show an example of a broken configuration?
> > > 
> > > Do you not understand the difference between depends an selects?
> > > Or did you not read my original mail?
> > > Flip off everything in drivers -> virtio
> > > 
> > > Now go to drivers -> block and try to turn on virtio-blk.
> > > 
> > > It's not listed!
> > 
> > Yes. Because you disabled all virtio backends.
> > It does not make sense to have any frontends.
> 
> How's a user - or even another kernel developer who isn't familiar with
> virtio - supposed to know that?
> 
> I still don't know what exactly a virtio backend is - the term isn't
> even mentioned anywhere that I've seen.
> 
> Whatever it is though virtio-blk should be depending on _that_, not a
> config option that _isn't exposed in the menu_!
> 
> > > Now go back to drivers -> virtio and turn on (randomly) balloon.
> > > 
> > > Go back to drivers -> block, and now you can turn on virtio-blk!
> > > 
> > > Do you see what's wrong with this picture?
> > 
> > Yes. You got unlucky with your random guess.
> > It's a bug in balloon kconfig: it should not
> > select virtio.
> > I sent a patch to fix that yesterday.
> 
> Then it's also a bug in the comments at the top of
> drivers/virtio/Kconfig.
> 
> And besides that, how the _hell_ is a user supposed to know to turn on
> VIRTIO_PCI before VIRTIO_BLK? It's not documented anywhere (if that is
> what's supposed to happen! I still don't know)

Well, what kind of device do you have? Tell us :)
If it's a virtio pci device,
you need to enable virtio-pci and virtio-blk.

> and even if it was
> documented, having one kconfig option depend on something that's exposed
> in a _completely different menu_ is just made of fail.

Fine, but why pick on virtio?
This is extremely common in kconfig.
For example, a ton of network drivers depend
on PCI, it's exactly the same thing.


-- 
MST

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-06 10:18             ` Michael S. Tsirkin
@ 2012-09-06 10:31               ` Kent Overstreet
  2012-09-06 11:09                 ` Michael S. Tsirkin
  0 siblings, 1 reply; 21+ messages in thread
From: Kent Overstreet @ 2012-09-06 10:31 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Rusty Russell, virtualization, linux-kernel

On Thu, Sep 06, 2012 at 01:18:43PM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 06, 2012 at 03:02:48AM -0700, Kent Overstreet wrote:
> > On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
> > > > On Thu, Sep 06, 2012 at 11:44:03AM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> > > > > > On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > > > > > > Kent Overstreet <koverstreet@google.com> writes:
> > > > > > > 
> > > > > > > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > > > > > > instead.
> > > > > > > 
> > > > > > > This is a slight mis-understanding.  It's supposed to be selected by
> > > > > > > the particular driver, probably virtio_pci in your case.
> > > > > > 
> > > > > > So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> > > > > > should have that.
> > > > > > 
> > > > > > As is, VIRTIO_BLK just has:
> > > > > > 	depends on EXPERIMENTAL && VIRTIO
> > > > > > 
> > > > > > which is flat out broken.
> > > > > 
> > > > > I don't think anything is broken.
> > > > > Can you show an example of a broken configuration?
> > > > 
> > > > Do you not understand the difference between depends an selects?
> > > > Or did you not read my original mail?
> > > > Flip off everything in drivers -> virtio
> > > > 
> > > > Now go to drivers -> block and try to turn on virtio-blk.
> > > > 
> > > > It's not listed!
> > > 
> > > Yes. Because you disabled all virtio backends.
> > > It does not make sense to have any frontends.
> > 
> > How's a user - or even another kernel developer who isn't familiar with
> > virtio - supposed to know that?
> > 
> > I still don't know what exactly a virtio backend is - the term isn't
> > even mentioned anywhere that I've seen.
> > 
> > Whatever it is though virtio-blk should be depending on _that_, not a
> > config option that _isn't exposed in the menu_!
> > 
> > > > Now go back to drivers -> virtio and turn on (randomly) balloon.
> > > > 
> > > > Go back to drivers -> block, and now you can turn on virtio-blk!
> > > > 
> > > > Do you see what's wrong with this picture?
> > > 
> > > Yes. You got unlucky with your random guess.
> > > It's a bug in balloon kconfig: it should not
> > > select virtio.
> > > I sent a patch to fix that yesterday.
> > 
> > Then it's also a bug in the comments at the top of
> > drivers/virtio/Kconfig.
> > 
> > And besides that, how the _hell_ is a user supposed to know to turn on
> > VIRTIO_PCI before VIRTIO_BLK? It's not documented anywhere (if that is
> > what's supposed to happen! I still don't know)
> 
> Well, what kind of device do you have? Tell us :)
> If it's a virtio pci device,
> you need to enable virtio-pci and virtio-blk.

I run qemu with -drive if=virtio. You tell me!

Better yet, tell me how the user is supposed to figure it out!

> 
> > and even if it was
> > documented, having one kconfig option depend on something that's exposed
> > in a _completely different menu_ is just made of fail.
> 
> Fine, but why pick on virtio?
> This is extremely common in kconfig.
> For example, a ton of network drivers depend
> on PCI, it's exactly the same thing.

Never noticed where CONFIG_PCI is exposed in bus options?

Nope, not the same thing.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-06 10:31               ` Kent Overstreet
@ 2012-09-06 11:09                 ` Michael S. Tsirkin
  0 siblings, 0 replies; 21+ messages in thread
From: Michael S. Tsirkin @ 2012-09-06 11:09 UTC (permalink / raw)
  To: Kent Overstreet; +Cc: Rusty Russell, virtualization, linux-kernel

On Thu, Sep 06, 2012 at 03:31:44AM -0700, Kent Overstreet wrote:
> On Thu, Sep 06, 2012 at 01:18:43PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Sep 06, 2012 at 03:02:48AM -0700, Kent Overstreet wrote:
> > > On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
> > > > > On Thu, Sep 06, 2012 at 11:44:03AM +0300, Michael S. Tsirkin wrote:
> > > > > > On Thu, Sep 06, 2012 at 12:41:13AM -0700, Kent Overstreet wrote:
> > > > > > > On Tue, Sep 04, 2012 at 03:53:53PM +0930, Rusty Russell wrote:
> > > > > > > > Kent Overstreet <koverstreet@google.com> writes:
> > > > > > > > 
> > > > > > > > > CONFIG_VIRTIO isn't exposed, everything else is supposed to select it
> > > > > > > > > instead.
> > > > > > > > 
> > > > > > > > This is a slight mis-understanding.  It's supposed to be selected by
> > > > > > > > the particular driver, probably virtio_pci in your case.
> > > > > > > 
> > > > > > > So are you saying virtio-blk depends on virtio-pci? If so, the kconfig
> > > > > > > should have that.
> > > > > > > 
> > > > > > > As is, VIRTIO_BLK just has:
> > > > > > > 	depends on EXPERIMENTAL && VIRTIO
> > > > > > > 
> > > > > > > which is flat out broken.
> > > > > > 
> > > > > > I don't think anything is broken.
> > > > > > Can you show an example of a broken configuration?
> > > > > 
> > > > > Do you not understand the difference between depends an selects?
> > > > > Or did you not read my original mail?
> > > > > Flip off everything in drivers -> virtio
> > > > > 
> > > > > Now go to drivers -> block and try to turn on virtio-blk.
> > > > > 
> > > > > It's not listed!
> > > > 
> > > > Yes. Because you disabled all virtio backends.
> > > > It does not make sense to have any frontends.
> > > 
> > > How's a user - or even another kernel developer who isn't familiar with
> > > virtio - supposed to know that?
> > > 
> > > I still don't know what exactly a virtio backend is - the term isn't
> > > even mentioned anywhere that I've seen.
> > > 
> > > Whatever it is though virtio-blk should be depending on _that_, not a
> > > config option that _isn't exposed in the menu_!
> > > 
> > > > > Now go back to drivers -> virtio and turn on (randomly) balloon.
> > > > > 
> > > > > Go back to drivers -> block, and now you can turn on virtio-blk!
> > > > > 
> > > > > Do you see what's wrong with this picture?
> > > > 
> > > > Yes. You got unlucky with your random guess.
> > > > It's a bug in balloon kconfig: it should not
> > > > select virtio.
> > > > I sent a patch to fix that yesterday.
> > > 
> > > Then it's also a bug in the comments at the top of
> > > drivers/virtio/Kconfig.
> > > 
> > > And besides that, how the _hell_ is a user supposed to know to turn on
> > > VIRTIO_PCI before VIRTIO_BLK? It's not documented anywhere (if that is
> > > what's supposed to happen! I still don't know)
> > 
> > Well, what kind of device do you have? Tell us :)
> > If it's a virtio pci device,
> > you need to enable virtio-pci and virtio-blk.
> 
> I run qemu with -drive if=virtio. You tell me!

-drive if= is a compatibility option. qemu makes
an effort to guess what it is you want to do.
Result is usually correct but it means people building
their own kernels get confused.

For x86 kvm the modern equivalent is:

-device virtio-blk-pci,drive=foobar -drive if=no,...

If you use this you get what you asked for :).

Yes this usage is not documented anywhere, but this is
not guest driver's problem.

> Better yet, tell me how the user is supposed to figure it out!

As usual when you do not know which driver to select.
Boot a distro kernel and look around.
Where is your virtio device? On a pci bus?
There you are.

> > 
> > > and even if it was
> > > documented, having one kconfig option depend on something that's exposed
> > > in a _completely different menu_ is just made of fail.
> > 
> > Fine, but why pick on virtio?
> > This is extremely common in kconfig.
> > For example, a ton of network drivers depend
> > on PCI, it's exactly the same thing.
> 
> Never noticed where CONFIG_PCI is exposed in bus options?

I see it:

CONFIG_PCI:
  │ Find out whether you have a PCI motherboard. PCI is the name of a │  
  │ bus system, i.e. the way the CPU talks to the other stuff inside │  
  │ your box. Other bus systems are ISA, EISA, MicroChannel (MCA) or │  
  │ VESA. If you have PCI, say Y, otherwise N.  │  

> Nope, not the same thing.

You just happen to know what PCI is but not what VIRTIO PCI is.
This is fair enough, but not sure how to help in this case.
Your patch won't help though.

-- 
MST

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-06 10:02           ` Kent Overstreet
  2012-09-06 10:18             ` Michael S. Tsirkin
@ 2012-09-06 23:40             ` Rusty Russell
  2012-09-07  0:25               ` Kent Overstreet
  1 sibling, 1 reply; 21+ messages in thread
From: Rusty Russell @ 2012-09-06 23:40 UTC (permalink / raw)
  To: Kent Overstreet, Michael S. Tsirkin; +Cc: virtualization, linux-kernel

Kent Overstreet <koverstreet@google.com> writes:

> On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
>> On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
>> > Do you not understand the difference between depends an selects?
>> > Or did you not read my original mail?

Now you're getting insulting.

It's normal for options to depend on other options.  Sometimes they're
directly nested (eg. E1000 depends on NETDEVICES, and it's nested under
that option), sometimes they're not (eg. E1000 depends on PCI, which is
selected elsewhere).

The fact that you are only just realizing this is not Michael's problem.

>> > Flip off everything in drivers -> virtio
>> > 
>> > Now go to drivers -> block and try to turn on virtio-blk.
>> > 
>> > It's not listed!
>> 
>> Yes. Because you disabled all virtio backends.
>> It does not make sense to have any frontends.
>
> How's a user - or even another kernel developer who isn't familiar with
> virtio - supposed to know that?

I get annoyed that menuconfig doesn't show options whose dependencies
aren't possible, too.  (I got bitten the other way: it doesn't show
dependencies which can't be disabled, and I was trying to turn KALLSYMS
off).

But as I found out just last week, the '/' key allows you to find any
option, and shows what dependencies it has, and their values.

Hope that helps,
Rusty.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-06 23:40             ` Rusty Russell
@ 2012-09-07  0:25               ` Kent Overstreet
  2012-09-07  2:57                 ` Rusty Russell
  0 siblings, 1 reply; 21+ messages in thread
From: Kent Overstreet @ 2012-09-07  0:25 UTC (permalink / raw)
  To: Rusty Russell; +Cc: Michael S. Tsirkin, virtualization, linux-kernel

On Fri, Sep 07, 2012 at 09:10:25AM +0930, Rusty Russell wrote:
> Kent Overstreet <koverstreet@google.com> writes:
> 
> > On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
> >> On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
> >> > Do you not understand the difference between depends an selects?
> >> > Or did you not read my original mail?
> 
> Now you're getting insulting.

Yes, but at least I'm not being intentionally obtuse.

> It's normal for options to depend on other options.  Sometimes they're
> directly nested (eg. E1000 depends on NETDEVICES, and it's nested under
> that option), sometimes they're not (eg. E1000 depends on PCI, which is
> selected elsewhere).
> 
> The fact that you are only just realizing this is not Michael's problem.

Like I said, I'm well aware of that. The issue here isn't the
dependency, it's that it depends on something that isn't exposed
anywhere!

Think about it from the user's pov. They check what VIRTIO_BLK depends
on - just VIRTIO.

So they try to figure out how to flip on VIRTIO, or what VIRTIO even is.

See how that last step might be problematic? CONFIG_VIRTIO is not
exposed! It doesn't even seem to control anything!

Go back to your example. Checking the dependencies for E1000 would tell
you the user needs to flip on CONFIG_PCI. Done. Easy.

User checks the dependencies here and... what do _you_ expect people to
do?

Look, depending on a kconfig option that's supposed to be user
controllable but isn't exposed anywhere is flat out broken. The fact
that it's in a different submenu just makes it worse.

The problem is that VIRTIO_BLK's dependencies are not actually specified
in the kconfig. If it depends on VIRTIO_PCI, that's what the kconfig
should say. If it depends on having any of multiple virtio backends
enabled, then specify that!

depends VIRTIO_PCI || VIRTIO_WHATEVER

Or if you really want to have a fake config option that's enabled if you
have any virtio backend enabled, fix the damn comments and naming!

How is anyone supposed to know that CONFIG_VIRTIO really means "any
virtio backend?" Call it VIRTIO_ANY_BACKEND if that's what it really is.

And, if that is what you're doing with CONFIG_VIRTIO (I'm still not
sure) the comment at the top of drivers/virtio/Kconfig is _wrong_:

# Virtio always gets selected by whoever wants it.
VIRTIO
        tristate

How is _anyone_ supposed to know that really means "VIRTIO gets selected
by things that provide a virtio backend?"

C'mon, you've had to debug other people's code before. What would _you_
think if you were tripped up by something like that?

> >> > Flip off everything in drivers -> virtio
> >> > 
> >> > Now go to drivers -> block and try to turn on virtio-blk.
> >> > 
> >> > It's not listed!
> >> 
> >> Yes. Because you disabled all virtio backends.
> >> It does not make sense to have any frontends.
> >
> > How's a user - or even another kernel developer who isn't familiar with
> > virtio - supposed to know that?
> 
> I get annoyed that menuconfig doesn't show options whose dependencies
> aren't possible, too.  (I got bitten the other way: it doesn't show
> dependencies which can't be disabled, and I was trying to turn KALLSYMS
> off).
> 
> But as I found out just last week, the '/' key allows you to find any
> option, and shows what dependencies it has, and their values.

Yep, use it all the time. 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] virtio-blk: Fix kconfig option
  2012-09-07  0:25               ` Kent Overstreet
@ 2012-09-07  2:57                 ` Rusty Russell
  0 siblings, 0 replies; 21+ messages in thread
From: Rusty Russell @ 2012-09-07  2:57 UTC (permalink / raw)
  To: Kent Overstreet; +Cc: Michael S. Tsirkin, virtualization, linux-kernel

Kent Overstreet <koverstreet@google.com> writes:
> On Fri, Sep 07, 2012 at 09:10:25AM +0930, Rusty Russell wrote:
>> Kent Overstreet <koverstreet@google.com> writes:
>> 
>> > On Thu, Sep 06, 2012 at 12:49:56PM +0300, Michael S. Tsirkin wrote:
>> >> On Thu, Sep 06, 2012 at 02:25:12AM -0700, Kent Overstreet wrote:
>> >> > Do you not understand the difference between depends an selects?
>> >> > Or did you not read my original mail?
>> 
>> Now you're getting insulting.
>
> Yes, but at least I'm not being intentionally obtuse.

Insulting again.  Wow.

It took me this long to understand your complaint.  Perhaps I'm stupid.
Or perhaps you are terrible at explaining yourself, and it is only
through our patient and heroic efforts that we can comprehend you at
all?

> Think about it from the user's pov. They check what VIRTIO_BLK depends
> on - just VIRTIO.
>
> So they try to figure out how to flip on VIRTIO, or what VIRTIO even is.
>
> See how that last step might be problematic? CONFIG_VIRTIO is not
> exposed! It doesn't even seem to control anything!
>
> Go back to your example. Checking the dependencies for E1000 would tell
> you the user needs to flip on CONFIG_PCI. Done. Easy.

Actually, it depends on NET_VENDOR_INTEL which depends on CONFIG_PCI,
but yes, it's discoverable.

So your actual complaint is that:
1) CONFIG_VIRTIO is misleadingly documented both in comment and name.
2) It's not discoverable, since it's only selected via other things.

> And, if that is what you're doing with CONFIG_VIRTIO (I'm still not
> sure) the comment at the top of drivers/virtio/Kconfig is _wrong_:

As grep would show you, it's selected by LGUEST, S390_GUEST, RPMSG,
VIRTIO_PCI and VIRTIO_MMIO (VIRTIO_BALLOON is a cut & paste bug, already
patched by MST).

We could change every virtio device to depend on (CONFIG_LGUEST |
CONFIG_S390_GUEST | CONFIG_RPMSG | CONFIG_VIRTIO_PCI |
CONFIG_VIRTIO_MMIO), which is more discoverable but uglier.  How's this
workaround?

From: Rusty Russell <rusty@rustcorp.com.au>
Subject: virtio: add help to CONFIG_VIRTIO option.

Trying to enable a virtio driver (eg CONFIG_VIRTIO_BLK) is painful
because it depends on CONFIG_VIRTIO.  CONFIG_VIRTIO doesn't tell you
how to turn it on (it's selected from anything which provides a virtio
bus).

This patch at least adds some documentation, visible in menuconfig, as
a hint.

Reported-by: Kent Overstreet <koverstreet@google.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -1,8 +1,10 @@
-# Virtio always gets selected by whoever wants it.
 config VIRTIO
 	tristate
+	---help---
+	  This option is selected by any driver which implements the virtio
+	  bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_LGUEST,
+	  CONFIG_RPMSG or CONFIG_S390_GUEST.
 
-# Similarly the virtio ring implementation.
 config VIRTIO_RING
 	tristate
 	depends on VIRTIO

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2012-09-07  3:30 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-03  4:41 [PATCH] virtio-blk: Fix kconfig option Kent Overstreet
2012-09-03  4:46 ` Kent Overstreet
2012-09-04  6:23 ` Rusty Russell
2012-09-04 23:12   ` Kent Overstreet
2012-09-05  4:22   ` Asias He
2012-09-05  5:48     ` Michael S. Tsirkin
2012-09-05  5:54       ` Asias He
2012-09-05  6:04         ` Michael S. Tsirkin
2012-09-05  6:12   ` Michael S. Tsirkin
2012-09-06  1:46     ` Rusty Russell
2012-09-06  7:41   ` Kent Overstreet
2012-09-06  8:44     ` Michael S. Tsirkin
2012-09-06  9:25       ` Kent Overstreet
2012-09-06  9:49         ` Michael S. Tsirkin
2012-09-06 10:02           ` Kent Overstreet
2012-09-06 10:18             ` Michael S. Tsirkin
2012-09-06 10:31               ` Kent Overstreet
2012-09-06 11:09                 ` Michael S. Tsirkin
2012-09-06 23:40             ` Rusty Russell
2012-09-07  0:25               ` Kent Overstreet
2012-09-07  2:57                 ` Rusty Russell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).