Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1f4ac369-75f8-f65d-6f31-9c4a5a2a357f@redhat.com>
Date:   Tue, 4 Jul 2023 21:22:34 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v2 3/3] vduse: Temporarily disable control queue features
Content-Language: en-US
To:     "Michael S. Tsirkin" <mst@redhat.com>
Cc:     xieyongji@bytedance.com, jasowang@redhat.com,
        david.marchand@redhat.com, lulu@redhat.com,
        linux-kernel@vger.kernel.org,
        virtualization@lists.linux-foundation.org, netdev@vger.kernel.org,
        xuanzhuo@linux.alibaba.com, eperezma@redhat.com
References: <20230704164045.39119-1-maxime.coquelin@redhat.com>
 <20230704164045.39119-4-maxime.coquelin@redhat.com>
 <20230704124245-mutt-send-email-mst@kernel.org>
From:   Maxime Coquelin <maxime.coquelin@redhat.com>
In-Reply-To: <20230704124245-mutt-send-email-mst@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 7/4/23 18:43, Michael S. Tsirkin wrote:
> On Tue, Jul 04, 2023 at 06:40:45PM +0200, Maxime Coquelin wrote:
>> Virtio-net driver control queue implementation is not safe
>> when used with VDUSE. If the VDUSE application does not
>> reply to control queue messages, it currently ends up
>> hanging the kernel thread sending this command.
>>
>> Some work is on-going to make the control queue
>> implementation robust with VDUSE. Until it is completed,
>> let's disable control virtqueue and features that depend on
>> it.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   drivers/vdpa/vdpa_user/vduse_dev.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>> index 1271c9796517..04367a53802b 100644
>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>> @@ -1778,6 +1778,25 @@ static struct attribute *vduse_dev_attrs[] = {
>>   
>>   ATTRIBUTE_GROUPS(vduse_dev);
>>   
>> +static void vduse_dev_features_fixup(struct vduse_dev_config *config)
>> +{
>> +	if (config->device_id == VIRTIO_ID_NET) {
>> +		/*
>> +		 * Temporarily disable control virtqueue and features that
>> +		 * depend on it while CVQ is being made more robust for VDUSE.
>> +		 */
>> +		config->features &= ~((1ULL << VIRTIO_NET_F_CTRL_VQ) |
>> +				(1ULL << VIRTIO_NET_F_CTRL_RX) |
>> +				(1ULL << VIRTIO_NET_F_CTRL_VLAN) |
>> +				(1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE) |
>> +				(1ULL << VIRTIO_NET_F_MQ) |
>> +				(1ULL << VIRTIO_NET_F_CTRL_MAC_ADDR) |
>> +				(1ULL << VIRTIO_NET_F_RSS) |
>> +				(1ULL << VIRTIO_NET_F_HASH_REPORT) |
>> +				(1ULL << VIRTIO_NET_F_NOTF_COAL));
>> +	}
>> +}
>> +
> 
> 
> This will never be exhaustive, we are adding new features.
> Please add an allowlist with just legal ones instead.

Ok, got it!
I'll post a new revision.

Thanks,
Maxime

> 
> 
>>   static int vduse_create_dev(struct vduse_dev_config *config,
>>   			    void *config_buf, u64 api_version)
>>   {
>> @@ -1793,6 +1812,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>>   	if (!dev)
>>   		goto err;
>>   
>> +	vduse_dev_features_fixup(config);
>> +
>>   	dev->api_version = api_version;
>>   	dev->device_features = config->features;
>>   	dev->device_id = config->device_id;
>> -- 
>> 2.41.0
>