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.
