Messages in this thread | | | Subject | Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero | From | Jason Wang <> | Date | Wed, 24 Feb 2021 17:40:24 +0800 |
| |
On 2021/2/23 6:17 下午, Jason Wang wrote: > > On 2021/2/23 6:01 下午, Michael S. Tsirkin wrote: >> On Tue, Feb 23, 2021 at 05:46:20PM +0800, Jason Wang wrote: >>> On 2021/2/23 下午5:25, Michael S. Tsirkin wrote: >>>> On Mon, Feb 22, 2021 at 09:09:28AM -0800, Si-Wei Liu wrote: >>>>> On 2/21/2021 8:14 PM, 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? >>>>> Isn't the commit 452639a64ad8 itself is a workaround for legacy >>>>> guest? >>>>> >>>>> I think the point is, since there's legacy guest we'd have to >>>>> support, this >>>>> host side workaround is unavoidable. Although I agree the >>>>> violating driver >>>>> should be fixed (yes, it's in today's upstream kernel which exists >>>>> for a >>>>> while now). >>>> Oh you are right: >>>> >>>> >>>> static int virtnet_validate(struct virtio_device *vdev) >>>> { >>>> if (!vdev->config->get) { >>>> dev_err(&vdev->dev, "%s failure: config access >>>> disabled\n", >>>> __func__); >>>> return -EINVAL; >>>> } >>>> >>>> if (!virtnet_validate_features(vdev)) >>>> return -EINVAL; >>>> >>>> if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) { >>>> int mtu = virtio_cread16(vdev, >>>> offsetof(struct >>>> virtio_net_config, >>>> mtu)); >>>> if (mtu < MIN_MTU) >>>> __virtio_clear_bit(vdev, VIRTIO_NET_F_MTU); >>> >>> I wonder why not simply fail here? >> Back in 2016 it went like this: >> >> On Thu, Jun 02, 2016 at 05:10:59PM -0400, Aaron Conole wrote: >> > + if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) { >> > + dev->mtu = virtio_cread16(vdev, >> > + offsetof(struct >> virtio_net_config, >> > + mtu)); >> > + } >> > + >> > if (vi->any_header_sg) >> > dev->needed_headroom = vi->hdr_len; >> > >> >> One comment though: I think we should validate the mtu. >> If it's invalid, clear VIRTIO_NET_F_MTU and ignore. >> >> >> Too late at this point :) >> >> I guess it's a way to tell device "I can not live with this MTU", >> device can fail FEATURES_OK if it wants to. MIN_MTU >> is an internal linux thing and at the time I felt it's better to >> try to make progress. > > > What if e.g the device advertise a large MTU. E.g 64K here?
Ok, consider we use add_recvbuf_small() when neither GSO nor mrg_rxbuf. This means we should fail the probing if MTU is greater than 1500 in this case.
Thanks
> In that case, the driver can not live either. Clearing MTU won't help > here. > > Thanks > > >> >> >>>> } >>>> >>>> return 0; >>>> } >>>> >>>> And the spec says: >>>> >>>> >>>> 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”. >>>> >>>> >>>> Item 4 on the list explicitly allows reading config space before >>>> FEATURES_OK. >>>> >>>> I conclude that VIRTIO_NET_F_MTU is set means "set in device >>>> features". >>> >>> So this probably need some clarification. "is set" is used many >>> times in the >>> spec that has different implications. >>> >>> Thanks >>> >>> >>>> Generally it is worth going over feature dependent config fields >>>> and checking whether they should be present when device feature is set >>>> or when feature bit has been negotiated, and making this clear. >>>> >
| |