Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero

[Jason Wang <jasowang@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5073 bytes --]


On 2021/2/24 3:17 下午, Michael S. Tsirkin wrote:
> On Wed, Feb 24, 2021 at 02:53:08PM +0800, Jason Wang wrote:
>> On 2021/2/24 2:46 下午, Michael S. Tsirkin wrote:
>>> On Wed, Feb 24, 2021 at 02:04:36PM +0800, Jason Wang wrote:
>>>> On 2021/2/24 1:04 下午, Michael S. Tsirkin wrote:
>>>>> On Tue, Feb 23, 2021 at 11:35:57AM -0800, Si-Wei Liu wrote:
>>>>>> On 2/23/2021 5:26 AM, Michael S. Tsirkin wrote:
>>>>>>> On Tue, Feb 23, 2021 at 10:03:57AM +0800, Jason Wang wrote:
>>>>>>>> On 2021/2/23 9:12 上午, Si-Wei Liu wrote:
>>>>>>>>> On 2/21/2021 11:34 PM, Michael S. Tsirkin wrote:
>>>>>>>>>> On Mon, Feb 22, 2021 at 12:14:17PM +0800, Jason Wang wrote:
>>>>>>>>>>> On 2021/2/19 7:54 下午, Si-Wei Liu wrote:
>>>>>>>>>>>> Commit 452639a64ad8 ("vdpa: make sure set_features is invoked
>>>>>>>>>>>> for legacy") made an exception for legacy guests to reset
>>>>>>>>>>>> features to 0, when config space is accessed before features
>>>>>>>>>>>> are set. We should relieve the verify_min_features() check
>>>>>>>>>>>> and allow features reset to 0 for this case.
>>>>>>>>>>>>
>>>>>>>>>>>> It's worth noting that not just legacy guests could access
>>>>>>>>>>>> config space before features are set. For instance, when
>>>>>>>>>>>> feature VIRTIO_NET_F_MTU is advertised some modern driver
>>>>>>>>>>>> will try to access and validate the MTU present in the config
>>>>>>>>>>>> space before virtio features are set.
>>>>>>>>>>> This looks like a spec violation:
>>>>>>>>>>>
>>>>>>>>>>> "
>>>>>>>>>>>
>>>>>>>>>>> The following driver-read-only field, mtu only exists if
>>>>>>>>>>> VIRTIO_NET_F_MTU is
>>>>>>>>>>> set.
>>>>>>>>>>> This field specifies the maximum MTU for the driver to use.
>>>>>>>>>>> "
>>>>>>>>>>>
>>>>>>>>>>> Do we really want to workaround this?
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>> And also:
>>>>>>>>>>
>>>>>>>>>> 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”.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> so accessing config space before FEATURES_OK is a spec violation, right?
>>>>>>>>> It is, but it's not relevant to what this commit tries to address. I
>>>>>>>>> thought the legacy guest still needs to be supported.
>>>>>>>>>
>>>>>>>>> Having said, a separate patch has to be posted to fix the guest driver
>>>>>>>>> issue where this discrepancy is introduced to virtnet_validate() (since
>>>>>>>>> commit fe36cbe067). But it's not technically related to this patch.
>>>>>>>>>
>>>>>>>>> -Siwei
>>>>>>>> I think it's a bug to read config space in validate, we should move it to
>>>>>>>> virtnet_probe().
>>>>>>>>
>>>>>>>> Thanks
>>>>>>> I take it back, reading but not writing seems to be explicitly allowed by spec.
>>>>>>> So our way to detect a legacy guest is bogus, need to think what is
>>>>>>> the best way to handle this.
>>>>>> Then maybe revert commit fe36cbe067 and friends, and have QEMU detect legacy
>>>>>> guest? Supposedly only config space write access needs to be guarded before
>>>>>> setting FEATURES_OK.
>>>>>>
>>>>>> -Siwie
>>>>> Detecting it isn't enough though, we will need a new ioctl to notify
>>>>> the kernel that it's a legacy guest. Ugh:(
>>>> I'm not sure I get this, how can we know if there's a legacy driver before
>>>> set_features()?
>>> qemu knows for sure. It does not communicate this information to the
>>> kernel right now unfortunately.
>> I may miss something, but I still don't get how the new ioctl is supposed to
>> work.
>>
>> Thanks
>
> Basically on first guest access QEMU would tell kernel whether
> guest is using the legacy or the modern interface.
> E.g. virtio_pci_config_read/virtio_pci_config_write will call ioctl(ENABLE_LEGACY, 1)
> while virtio_pci_common_read will call ioctl(ENABLE_LEGACY, 0)


But this trick work only for PCI I think?

Thanks


>
> Or maybe we just add GET_CONFIG_MODERN and GET_CONFIG_LEGACY and
> call the correct ioctl ... there are many ways to build this API.
>

[-- Attachment #1.2: Type: text/html, Size: 7219 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization