Virtualization - Feb 2021 - [PATCH] vdpa/mlx5: set_features should allow reset to zero

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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210224/3838e8f9/attachment.html>

On Wed, Feb 24, 2021 at 04:26:43PM +0800, Jason Wang
wrote:
>     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
ccw has a revision it can check. mmio does not have transitional devices
at all.

-- 
MST