Linux Virtualization - Apr 2020 - [PATCH V2] vhost: do not enable VHOST_MENU by default

On 2020/4/17 ??4:46, Michael S. Tsirkin wrote:
> On Fri, Apr 17, 2020 at 04:39:49PM +0800, Jason Wang wrote:
>> On 2020/4/17 ??4:29, Michael S. Tsirkin wrote:
>>> On Fri, Apr 17, 2020 at 03:36:52PM +0800, Jason Wang wrote:
>>>> On 2020/4/17 ??2:33, Michael S. Tsirkin wrote:
>>>>> On Fri, Apr 17, 2020 at 11:12:14AM +0800, Jason Wang wrote:
>>>>>> On 2020/4/17 ??6:55, Michael S. Tsirkin wrote:
>>>>>>> On Wed, Apr 15, 2020 at 10:43:56AM +0800, Jason
Wang wrote:
>>>>>>>> We try to keep the defconfig untouched after
decoupling CONFIG_VHOST
>>>>>>>> out of CONFIG_VIRTUALIZATION in commit
20c384f1ea1a
>>>>>>>> ("vhost: refine vhost and vringh
kconfig") by enabling VHOST_MENU by
>>>>>>>> default. Then the defconfigs can keep enabling
CONFIG_VHOST_NET
>>>>>>>> without the caring of CONFIG_VHOST.
>>>>>>>>
>>>>>>>> But this will leave a
"CONFIG_VHOST_MENU=y" in all defconfigs and even
>>>>>>>> for the ones that doesn't want vhost. So it
actually shifts the
>>>>>>>> burdens to the maintainers of all other to add
"CONFIG_VHOST_MENU is
>>>>>>>> not set". So this patch tries to enable
CONFIG_VHOST explicitly in
>>>>>>>> defconfigs that enables CONFIG_VHOST_NET and
CONFIG_VHOST_VSOCK.
>>>>>>>>
>>>>>>>> Acked-by: Christian Borntraeger<borntraeger
at de.ibm.com>   (s390)
>>>>>>>> Acked-by: Michael Ellerman<mpe at
ellerman.id.au>   (powerpc)
>>>>>>>> Cc: Thomas Bogendoerfer<tsbogend at
alpha.franken.de>
>>>>>>>> Cc: Benjamin Herrenschmidt<benh at
kernel.crashing.org>
>>>>>>>> Cc: Paul Mackerras<paulus at samba.org>
>>>>>>>> Cc: Michael Ellerman<mpe at
ellerman.id.au>
>>>>>>>> Cc: Heiko Carstens<heiko.carstens at
de.ibm.com>
>>>>>>>> Cc: Vasily Gorbik<gor at linux.ibm.com>
>>>>>>>> Cc: Christian Borntraeger<borntraeger at
de.ibm.com>
>>>>>>>> Reported-by: Geert Uytterhoeven<geert at
linux-m68k.org>
>>>>>>>> Signed-off-by: Jason Wang<jasowang at
redhat.com>
>>>>>>> I rebased this on top of OABI fix since that
>>>>>>> seems more orgent to fix.
>>>>>>> Pushed to my vhost branch pls take a look and
>>>>>>> if possible test.
>>>>>>> Thanks!
>>>>>> I test this patch by generating the defconfigs that
wants vhost_net or
>>>>>> vhost_vsock. All looks fine.
>>>>>>
>>>>>> But having CONFIG_VHOST_DPN=y may end up with the
similar situation that
>>>>>> this patch want to address.
>>>>>> Maybe we can let CONFIG_VHOST depends on !ARM || AEABI
then add another
>>>>>> menuconfig for VHOST_RING and do something similar?
>>>>>>
>>>>>> Thanks
>>>>> Sorry I don't understand. After this patch
CONFIG_VHOST_DPN is just
>>>>> an internal variable for the OABI fix. I kept it separate
>>>>> so it's easy to revert for 5.8. Yes we could squash it
into
>>>>> VHOST directly but I don't see how that changes logic
at all.
>>>> Sorry for being unclear.
>>>>
>>>> I meant since it was enabled by default,
"CONFIG_VHOST_DPN=y" will be left
>>>> in the defconfigs.
>>> But who cares?
>> FYI, please seehttps://www.spinics.net/lists/kvm/msg212685.html
> The complaint was not about the symbol IIUC.  It was that we caused
> everyone to build vhost unless they manually disabled it.

There could be some misunderstanding here. I thought it's somehow 
similar: a CONFIG_VHOST_MENU=y will be left in the defconfigs even if 
CONFIG_VHOST is not set.

Thanks

>

On Fri, Apr 17, 2020 at 04:51:19PM +0800, Jason Wang
wrote:
> 
> On 2020/4/17 ??4:46, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 04:39:49PM +0800, Jason Wang wrote:
> > > On 2020/4/17 ??4:29, Michael S. Tsirkin wrote:
> > > > On Fri, Apr 17, 2020 at 03:36:52PM +0800, Jason Wang wrote:
> > > > > On 2020/4/17 ??2:33, Michael S. Tsirkin wrote:
> > > > > > On Fri, Apr 17, 2020 at 11:12:14AM +0800, Jason
Wang wrote:
> > > > > > > On 2020/4/17 ??6:55, Michael S. Tsirkin
wrote:
> > > > > > > > On Wed, Apr 15, 2020 at 10:43:56AM
+0800, Jason Wang wrote:
> > > > > > > > > We try to keep the defconfig
untouched after decoupling CONFIG_VHOST
> > > > > > > > > out of CONFIG_VIRTUALIZATION in
commit 20c384f1ea1a
> > > > > > > > > ("vhost: refine vhost and
vringh kconfig") by enabling VHOST_MENU by
> > > > > > > > > default. Then the defconfigs can
keep enabling CONFIG_VHOST_NET
> > > > > > > > > without the caring of CONFIG_VHOST.
> > > > > > > > > 
> > > > > > > > > But this will leave a
"CONFIG_VHOST_MENU=y" in all defconfigs and even
> > > > > > > > > for the ones that doesn't want
vhost. So it actually shifts the
> > > > > > > > > burdens to the maintainers of all
other to add "CONFIG_VHOST_MENU is
> > > > > > > > > not set". So this patch tries
to enable CONFIG_VHOST explicitly in
> > > > > > > > > defconfigs that enables
CONFIG_VHOST_NET and CONFIG_VHOST_VSOCK.
> > > > > > > > > 
> > > > > > > > > Acked-by: Christian
Borntraeger<borntraeger at de.ibm.com>   (s390)
> > > > > > > > > Acked-by: Michael Ellerman<mpe
at ellerman.id.au>   (powerpc)
> > > > > > > > > Cc: Thomas Bogendoerfer<tsbogend
at alpha.franken.de>
> > > > > > > > > Cc: Benjamin Herrenschmidt<benh
at kernel.crashing.org>
> > > > > > > > > Cc: Paul Mackerras<paulus at
samba.org>
> > > > > > > > > Cc: Michael Ellerman<mpe at
ellerman.id.au>
> > > > > > > > > Cc: Heiko
Carstens<heiko.carstens at de.ibm.com>
> > > > > > > > > Cc: Vasily Gorbik<gor at
linux.ibm.com>
> > > > > > > > > Cc: Christian
Borntraeger<borntraeger at de.ibm.com>
> > > > > > > > > Reported-by: Geert
Uytterhoeven<geert at linux-m68k.org>
> > > > > > > > > Signed-off-by: Jason
Wang<jasowang at redhat.com>
> > > > > > > > I rebased this on top of OABI fix since
that
> > > > > > > > seems more orgent to fix.
> > > > > > > > Pushed to my vhost branch pls take a
look and
> > > > > > > > if possible test.
> > > > > > > > Thanks!
> > > > > > > I test this patch by generating the
defconfigs that wants vhost_net or
> > > > > > > vhost_vsock. All looks fine.
> > > > > > > 
> > > > > > > But having CONFIG_VHOST_DPN=y may end up with
the similar situation that
> > > > > > > this patch want to address.
> > > > > > > Maybe we can let CONFIG_VHOST depends on !ARM
|| AEABI then add another
> > > > > > > menuconfig for VHOST_RING and do something
similar?
> > > > > > > 
> > > > > > > Thanks
> > > > > > Sorry I don't understand. After this patch
CONFIG_VHOST_DPN is just
> > > > > > an internal variable for the OABI fix. I kept it
separate
> > > > > > so it's easy to revert for 5.8. Yes we could
squash it into
> > > > > > VHOST directly but I don't see how that
changes logic at all.
> > > > > Sorry for being unclear.
> > > > > 
> > > > > I meant since it was enabled by default,
"CONFIG_VHOST_DPN=y" will be left
> > > > > in the defconfigs.
> > > > But who cares?
> > > FYI, please seehttps://www.spinics.net/lists/kvm/msg212685.html
> > The complaint was not about the symbol IIUC.  It was that we caused
> > everyone to build vhost unless they manually disabled it.
> 
> 
> There could be some misunderstanding here. I thought it's somehow
similar: a
> CONFIG_VHOST_MENU=y will be left in the defconfigs even if CONFIG_VHOST is
> not set.
> 
> Thanks
Hmm. So looking at Documentation/kbuild/kconfig-language.rst :

        Things that merit "default y/m" include:

        a) A new Kconfig option for something that used to always be built
           should be "default y".


        b) A new gatekeeping Kconfig option that hides/shows other Kconfig
           options (but does not generate any code of its own), should be
           "default y" so people will see those other options.

        c) Sub-driver behavior or similar options for a driver that is
           "default n". This allows you to provide sane defaults.


So it looks like VHOST_MENU is actually matching rule b).
So what's the problem we are trying to solve with this patch, exactly?

Geert could you clarify pls?

> 
> >

On Fri, Apr 17, 2020 at 04:51:19PM +0800, Jason Wang
wrote:
> 
> On 2020/4/17 ??4:46, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 04:39:49PM +0800, Jason Wang wrote:
> > > On 2020/4/17 ??4:29, Michael S. Tsirkin wrote:
> > > > On Fri, Apr 17, 2020 at 03:36:52PM +0800, Jason Wang wrote:
> > > > > On 2020/4/17 ??2:33, Michael S. Tsirkin wrote:
> > > > > > On Fri, Apr 17, 2020 at 11:12:14AM +0800, Jason
Wang wrote:
> > > > > > > On 2020/4/17 ??6:55, Michael S. Tsirkin
wrote:
> > > > > > > > On Wed, Apr 15, 2020 at 10:43:56AM
+0800, Jason Wang wrote:
> > > > > > > > > We try to keep the defconfig
untouched after decoupling CONFIG_VHOST
> > > > > > > > > out of CONFIG_VIRTUALIZATION in
commit 20c384f1ea1a
> > > > > > > > > ("vhost: refine vhost and
vringh kconfig") by enabling VHOST_MENU by
> > > > > > > > > default. Then the defconfigs can
keep enabling CONFIG_VHOST_NET
> > > > > > > > > without the caring of CONFIG_VHOST.
> > > > > > > > > 
> > > > > > > > > But this will leave a
"CONFIG_VHOST_MENU=y" in all defconfigs and even
> > > > > > > > > for the ones that doesn't want
vhost. So it actually shifts the
> > > > > > > > > burdens to the maintainers of all
other to add "CONFIG_VHOST_MENU is
> > > > > > > > > not set". So this patch tries
to enable CONFIG_VHOST explicitly in
> > > > > > > > > defconfigs that enables
CONFIG_VHOST_NET and CONFIG_VHOST_VSOCK.
> > > > > > > > > 
> > > > > > > > > Acked-by: Christian
Borntraeger<borntraeger at de.ibm.com>   (s390)
> > > > > > > > > Acked-by: Michael Ellerman<mpe
at ellerman.id.au>   (powerpc)
> > > > > > > > > Cc: Thomas Bogendoerfer<tsbogend
at alpha.franken.de>
> > > > > > > > > Cc: Benjamin Herrenschmidt<benh
at kernel.crashing.org>
> > > > > > > > > Cc: Paul Mackerras<paulus at
samba.org>
> > > > > > > > > Cc: Michael Ellerman<mpe at
ellerman.id.au>
> > > > > > > > > Cc: Heiko
Carstens<heiko.carstens at de.ibm.com>
> > > > > > > > > Cc: Vasily Gorbik<gor at
linux.ibm.com>
> > > > > > > > > Cc: Christian
Borntraeger<borntraeger at de.ibm.com>
> > > > > > > > > Reported-by: Geert
Uytterhoeven<geert at linux-m68k.org>
> > > > > > > > > Signed-off-by: Jason
Wang<jasowang at redhat.com>
> > > > > > > > I rebased this on top of OABI fix since
that
> > > > > > > > seems more orgent to fix.
> > > > > > > > Pushed to my vhost branch pls take a
look and
> > > > > > > > if possible test.
> > > > > > > > Thanks!
> > > > > > > I test this patch by generating the
defconfigs that wants vhost_net or
> > > > > > > vhost_vsock. All looks fine.
> > > > > > > 
> > > > > > > But having CONFIG_VHOST_DPN=y may end up with
the similar situation that
> > > > > > > this patch want to address.
> > > > > > > Maybe we can let CONFIG_VHOST depends on !ARM
|| AEABI then add another
> > > > > > > menuconfig for VHOST_RING and do something
similar?
> > > > > > > 
> > > > > > > Thanks
> > > > > > Sorry I don't understand. After this patch
CONFIG_VHOST_DPN is just
> > > > > > an internal variable for the OABI fix. I kept it
separate
> > > > > > so it's easy to revert for 5.8. Yes we could
squash it into
> > > > > > VHOST directly but I don't see how that
changes logic at all.
> > > > > Sorry for being unclear.
> > > > > 
> > > > > I meant since it was enabled by default,
"CONFIG_VHOST_DPN=y" will be left
> > > > > in the defconfigs.
> > > > But who cares?
> > > FYI, please seehttps://www.spinics.net/lists/kvm/msg212685.html
> > The complaint was not about the symbol IIUC.  It was that we caused
> > everyone to build vhost unless they manually disabled it.
> 
> 
> There could be some misunderstanding here. I thought it's somehow
similar: a
> CONFIG_VHOST_MENU=y will be left in the defconfigs even if CONFIG_VHOST is
> not set.
> 
> Thanks
> 
BTW do entries with no prompt actually appear in defconfig?
> >

Hi Michael,

On Fri, Apr 17, 2020 at 10:57 AM Michael S. Tsirkin <mst at redhat.com>
wrote:
> On Fri, Apr 17, 2020 at 04:51:19PM +0800, Jason Wang wrote:
> > On 2020/4/17 ??4:46, Michael S. Tsirkin wrote:
> > > On Fri, Apr 17, 2020 at 04:39:49PM +0800, Jason Wang wrote:
> > > > On 2020/4/17 ??4:29, Michael S. Tsirkin wrote:
> > > > > On Fri, Apr 17, 2020 at 03:36:52PM +0800, Jason Wang
wrote:
> > > > > > On 2020/4/17 ??2:33, Michael S. Tsirkin wrote:
> > > > > > > On Fri, Apr 17, 2020 at 11:12:14AM +0800,
Jason Wang wrote:
> > > > > > > > On 2020/4/17 ??6:55, Michael S. Tsirkin
wrote:
> > > > > > > > > On Wed, Apr 15, 2020 at 10:43:56AM
+0800, Jason Wang wrote:
> > > > > > > > > > We try to keep the defconfig
untouched after decoupling CONFIG_VHOST
> > > > > > > > > > out of CONFIG_VIRTUALIZATION
in commit 20c384f1ea1a
> > > > > > > > > > ("vhost: refine vhost and
vringh kconfig") by enabling VHOST_MENU by
> > > > > > > > > > default. Then the defconfigs
can keep enabling CONFIG_VHOST_NET
> > > > > > > > > > without the caring of
CONFIG_VHOST.
> > > > > > > > > >
> > > > > > > > > > But this will leave a
"CONFIG_VHOST_MENU=y" in all defconfigs and even
> > > > > > > > > > for the ones that doesn't
want vhost. So it actually shifts the
> > > > > > > > > > burdens to the maintainers of
all other to add "CONFIG_VHOST_MENU is
> > > > > > > > > > not set". So this patch
tries to enable CONFIG_VHOST explicitly in
> > > > > > > > > > defconfigs that enables
CONFIG_VHOST_NET and CONFIG_VHOST_VSOCK.
> > > > > > > > > >
> > > > > > > > > > Acked-by: Christian
Borntraeger<borntraeger at de.ibm.com>   (s390)
> > > > > > > > > > Acked-by: Michael
Ellerman<mpe at ellerman.id.au>   (powerpc)
> > > > > > > > > > Cc: Thomas
Bogendoerfer<tsbogend at alpha.franken.de>
> > > > > > > > > > Cc: Benjamin
Herrenschmidt<benh at kernel.crashing.org>
> > > > > > > > > > Cc: Paul Mackerras<paulus
at samba.org>
> > > > > > > > > > Cc: Michael Ellerman<mpe at
ellerman.id.au>
> > > > > > > > > > Cc: Heiko
Carstens<heiko.carstens at de.ibm.com>
> > > > > > > > > > Cc: Vasily Gorbik<gor at
linux.ibm.com>
> > > > > > > > > > Cc: Christian
Borntraeger<borntraeger at de.ibm.com>
> > > > > > > > > > Reported-by: Geert
Uytterhoeven<geert at linux-m68k.org>
> > > > > > > > > > Signed-off-by: Jason
Wang<jasowang at redhat.com>
> > > > > > > > > I rebased this on top of OABI fix
since that
> > > > > > > > > seems more orgent to fix.
> > > > > > > > > Pushed to my vhost branch pls take
a look and
> > > > > > > > > if possible test.
> > > > > > > > > Thanks!
> > > > > > > > I test this patch by generating the
defconfigs that wants vhost_net or
> > > > > > > > vhost_vsock. All looks fine.
> > > > > > > >
> > > > > > > > But having CONFIG_VHOST_DPN=y may end up
with the similar situation that
> > > > > > > > this patch want to address.
> > > > > > > > Maybe we can let CONFIG_VHOST depends on
!ARM || AEABI then add another
> > > > > > > > menuconfig for VHOST_RING and do
something similar?
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > Sorry I don't understand. After this
patch CONFIG_VHOST_DPN is just
> > > > > > > an internal variable for the OABI fix. I kept
it separate
> > > > > > > so it's easy to revert for 5.8. Yes we
could squash it into
> > > > > > > VHOST directly but I don't see how that
changes logic at all.
> > > > > > Sorry for being unclear.
> > > > > >
> > > > > > I meant since it was enabled by default,
"CONFIG_VHOST_DPN=y" will be left
> > > > > > in the defconfigs.
> > > > > But who cares?
> > > > FYI, please
seehttps://www.spinics.net/lists/kvm/msg212685.html
> > > The complaint was not about the symbol IIUC.  It was that we
caused
> > > everyone to build vhost unless they manually disabled it.
> >
> > There could be some misunderstanding here. I thought it's somehow
similar: a
> > CONFIG_VHOST_MENU=y will be left in the defconfigs even if
CONFIG_VHOST is
> > not set.
> >
> > Thanks
>
> Hmm. So looking at Documentation/kbuild/kconfig-language.rst :
>
>         Things that merit "default y/m" include:
>
>         a) A new Kconfig option for something that used to always be built
>            should be "default y".
>
>         b) A new gatekeeping Kconfig option that hides/shows other Kconfig
>            options (but does not generate any code of its own), should be
>            "default y" so people will see those other options.
>
>         c) Sub-driver behavior or similar options for a driver that is
>            "default n". This allows you to provide sane defaults.
>
>
> So it looks like VHOST_MENU is actually matching rule b).
> So what's the problem we are trying to solve with this patch, exactly?
>
> Geert could you clarify pls?
I can confirm VHOST_MENU is matching rule b), so it is safe to always
enable it.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at
linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or
something like that.
                                -- Linus Torvalds

On 2020/4/17 ??5:01, Michael S. Tsirkin wrote:
>> There could be some misunderstanding here. I thought it's somehow
similar: a
>> CONFIG_VHOST_MENU=y will be left in the defconfigs even if CONFIG_VHOST
is
>> not set.
>>
>> Thanks
>>
> BTW do entries with no prompt actually appear in defconfig?
>
Yes. I can see CONFIG_VHOST_DPN=y after make ARCH=m68k defconfig

Thanks