Re: [Qemu-devel] [PATCH] virtio-net: do not start queues that are not enabled by the guest

[Jason Wang <jasowang@redhat.com>]

On 2019/2/22 下午12:22, Michael S. Tsirkin wrote:
> On Thu, Feb 21, 2019 at 10:10:08PM -0500, Michael S. Tsirkin wrote:
>> On Fri, Feb 22, 2019 at 11:04:05AM +0800, Jason Wang wrote:
>>> On 2019/2/22 上午9:35, Michael S. Tsirkin wrote:
>>>> On Thu, Feb 21, 2019 at 05:40:22PM +0800, Jason Wang wrote:
>>>>> On 2019/2/21 下午4:18, Yuri Benditovich wrote:
>>>>>
>>>>>           For 1.0 device, we can fix the queue_enable, but for 0.9x device how do
>>>>>           you enable one specific queue in this case? (setting status?)
>>>>>
>>>>>
>>>>>       Do I understand correctly that for 0.9 device in some cases the device will
>>>>>       receive feature _MQ set, but will not receive VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET?
>>>>>       Or the problem is different?
>>>>>
>>>>>
>>>>> Let me clarify, VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET is used to control the the
>>>>> number of queue pairs used by device for doing transmission and reception. It
>>>>> was not used to enable or disable a virtqueue.
>>>>>
>>>>> For 1.0 device, we should use queue_enable in pci cfg to enable and disable
>>>>> queue:
>>>>>
>>>>>
>>>>> We could do:
>>>>>
>>>>> 1) allocate memory and set queue_enable for vq0
>>>>>
>>>>> 2) allocate memory and set queue_enable for vq1
>>>>>
>>>>> 3) Set vq paris to 1
>>>>>
>>>>> 4) allocate memory and set queue_enable for vq2
>>>>>
>>>>> 5) allocate memory and set queue_enable for vq3
>>>>>
>>>>> 6) set vq pairs to 2
>>>> I do not think spec allows this.
>>>>
>>>>
>>>> The driver MUST follow this sequence to initialize a device:
>>>> 1. Reset the device.
>>>> 2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device.
>>>> 3. Set the DRIVER status bit: the guest OS knows how to drive the device.
>>>> 4. Read device feature bits, and write the subset of feature bits understood by the OS and driver to the
>>>> device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration
>>>> fields to check that it can support the device before accepting it.
>>>> 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step.
>>>> 6. Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not
>>>> support our subset of features and the device is unusable.
>>>> 7. Perform device-specific setup, including discovery of virtqueues for the device, optional per-bus setup,
>>>> reading and possibly writing the device’s virtio configuration space, and population of virtqueues.
>>>> 8. Set the DRIVER_OK status bit. At this point the device is “live”.
>>>>
>>>>
>>>> Thus vqs are setup at step 7.
>>>>
>>>> # of vq pairs are set up through a command which is a special
>>>> buffer, and spec says:
>>>>
>>>> The driver MUST NOT send any buffer available notifications to the device before setting DRIVER_OK.
>>>
>>> So you meant write to queue_enable is forbidden after DRIVER_OK (though it's
>>> not very clear to me from the  spec). And if a driver want to enable new
>>> queues, it must reset the device?
>>
>> That's my reading.  What do you think?


Looks like I can infer this from the spec, maybe it's better to clarify.


> Btw some legacy drivers might violate this by addig buffers
> before driver_ok.


Yes, but it's probably too late to fix them.

Thanks.


>>>>
>>>>> But this requires a proper implementation for queue_enable for vhost which is
>>>>> missed in qemu and probably what you really want to do.
>>>>>
>>>>> but for 0.9x device, there's no such way to do this. That's the issue.
>>>> 0.9x there's no queue enable, assumption is PA!=0 means VQ has
>>>> been enabled.
>>>>
>>>>
>>>>> So
>>>>> driver must allocate all queBes before starting the device, otherwise there's
>>>>> no way to enable it afterwards.
>>>> As per spec queues must be allocated before DRIVER_OK.
>>>>
>>>> That is universal.
>>>
>>> If I understand correctly, this is not what is done by current windows
>>> drivers.
>>>
>>> Thanks
>>>
>>>
>>>>> There're tricks to make it work like what is
>>>>> done in your patch, but it depends on a specific implementation like qemu which
>>>>> is sub-optimal.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>           A fundamental question is what prevents you from just initialization all
>>>>>           queues during driver start? It looks to me this save lots of efforts
>>>>>           than allocating queue dynamically.
>>>>>
>>>>>
>>>>>       This is not so trivial in Windows driver, as it does not have objects for queues
>>>>>       that it does not use. Linux driver first of all allocates all the
>>>>>       queues and then
>>>>>       adds Rx/Tx to those it will use. Windows driver first decides how many queues
>>>>>       it will use then allocates objects for them and initializes them from zero to
>>>>>       fully functional state.
>>>>>
>>>>>
>>>>> Well, you just need to allocate some memory for the virtqueue, there's no need
>>>>> to make it visible to the rest until it was enabled.
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>>
>>>>>