Re: [PATCH V2] vhost: do not enable VHOST_MENU by default

[Jason Wang <jasowang@redhat.com>]

On 2020/4/17 下午5:25, Geert Uytterhoeven wrote:
> Hi Michael,
>
> On Fri, Apr 17, 2020 at 10:57 AM Michael S. Tsirkin<mst@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@de.ibm.com>    (s390)
>>>>>>>>>>> Acked-by: Michael Ellerman<mpe@ellerman.id.au>    (powerpc)
>>>>>>>>>>> Cc: Thomas Bogendoerfer<tsbogend@alpha.franken.de>
>>>>>>>>>>> Cc: Benjamin Herrenschmidt<benh@kernel.crashing.org>
>>>>>>>>>>> Cc: Paul Mackerras<paulus@samba.org>
>>>>>>>>>>> Cc: Michael Ellerman<mpe@ellerman.id.au>
>>>>>>>>>>> Cc: Heiko Carstens<heiko.carstens@de.ibm.com>
>>>>>>>>>>> Cc: Vasily Gorbik<gor@linux.ibm.com>
>>>>>>>>>>> Cc: Christian Borntraeger<borntraeger@de.ibm.com>
>>>>>>>>>>> Reported-by: Geert Uytterhoeven<geert@linux-m68k.org>
>>>>>>>>>>> Signed-off-by: Jason Wang<jasowang@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


Right, so I think we can drop this patch.

Thanks