On Tue, 23 Feb 2021 18:31:07 +0800
Jason Wang <[email protected]> wrote:
> On 2021/2/23 6:04 下午, Cornelia Huck wrote:
> > On Tue, 23 Feb 2021 17:46:20 +0800
> > Jason Wang <[email protected]> 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?
> > I think both failing or not accepting the feature can be argued to make
> > sense: "the device presented us with a mtu size that does not make
> > sense" would point to failing, "we cannot work with the mtu size that
> > the device presented us" would point to not negotiating the feature.
> >
> >>
> >>> }
> >>>
> >>> 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.
> > Before FEATURES_OK is set by the driver, I guess it means "the device
> > has offered the feature";
>
>
> For me this part is ok since it clarify that it's the driver that set
> the bit.
>
>
>
> > during normal usage, it means "the feature
> > has been negotiated".
>
> /?
>
> It looks to me the feature negotiation is done only after device set
> FEATURES_OK, or FEATURES_OK could be read from device status?
I'd consider feature negotiation done when the driver reads FEATURES_OK
back from the status.
>
>
> > (This is a bit fuzzy for legacy mode.)
...because legacy does not have FEATURES_OK.
>
>
> The problem is the MTU description for example:
>
> "The following driver-read-only field, mtu only exists if
> VIRTIO_NET_F_MTU is set."
>
> It looks to me need to use "if VIRTIO_NET_F_MTU is set by device".
"offered by the device"? I don't think it should 'disappear' from the
config space if the driver won't use it. (Same for other config space
fields that are tied to feature bits.)
> Otherwise readers (at least for me), may think the MTU is only valid
> if driver set the bit.
I think it would still be 'valid' in the sense that it exists and has
some value in there filled in by the device, but a driver reading it
without negotiating the feature would be buggy. (Like in the kernel
code above; the kernel not liking the value does not make the field
invalid.)
Maybe a statement covering everything would be:
"The following driver-read-only field mtu only exists if the device
offers VIRTIO_NET_F_MTU and may be read by the driver during feature
negotiation and after VIRTIO_NET_F_MTU has been successfully
negotiated."
>
>
> >
> > Should we add a wording clarification to the spec?
>
>
> I think so.
Some clarification would be needed for each field that depends on a
feature; that would be quite verbose. Maybe we can get away with a
clarifying statement?
"Some config space fields may depend on a certain feature. In that
case, the field exits if the device has offered the corresponding
feature, and may be read by the driver during feature negotiation, and
accessed by the driver after the feature has been successfully
negotiated. A shorthand for this is a statement that a field only
exists if a certain feature bit is set."
On 2021/2/23 6:58 下午, Cornelia Huck wrote:
> On Tue, 23 Feb 2021 18:31:07 +0800
> Jason Wang <[email protected]> wrote:
>
>> On 2021/2/23 6:04 下午, Cornelia Huck wrote:
>>> On Tue, 23 Feb 2021 17:46:20 +0800
>>> Jason Wang <[email protected]> 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?
>>> I think both failing or not accepting the feature can be argued to make
>>> sense: "the device presented us with a mtu size that does not make
>>> sense" would point to failing, "we cannot work with the mtu size that
>>> the device presented us" would point to not negotiating the feature.
>>>
>>>>
>>>>> }
>>>>>
>>>>> 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.
>>> Before FEATURES_OK is set by the driver, I guess it means "the device
>>> has offered the feature";
>>
>> For me this part is ok since it clarify that it's the driver that set
>> the bit.
>>
>>
>>
>>> during normal usage, it means "the feature
>>> has been negotiated".
>> /?
>>
>> It looks to me the feature negotiation is done only after device set
>> FEATURES_OK, or FEATURES_OK could be read from device status?
> I'd consider feature negotiation done when the driver reads FEATURES_OK
> back from the status.
I agree.
>
>>
>>> (This is a bit fuzzy for legacy mode.)
> ...because legacy does not have FEATURES_OK.
>
>>
>> The problem is the MTU description for example:
>>
>> "The following driver-read-only field, mtu only exists if
>> VIRTIO_NET_F_MTU is set."
>>
>> It looks to me need to use "if VIRTIO_NET_F_MTU is set by device".
> "offered by the device"? I don't think it should 'disappear' from the
> config space if the driver won't use it. (Same for other config space
> fields that are tied to feature bits.)
But what happens if e.g device doesn't offer VIRTIO_NET_F_MTU? It looks
to according to the spec there will be no mtu field.
And a more interesting case is VIRTIO_NET_F_MQ is not offered but
VIRTIO_NET_F_MTU offered. To me, it means we don't have
max_virtqueue_pairs but it's not how the driver is wrote today.
>
>> Otherwise readers (at least for me), may think the MTU is only valid
>> if driver set the bit.
> I think it would still be 'valid' in the sense that it exists and has
> some value in there filled in by the device, but a driver reading it
> without negotiating the feature would be buggy. (Like in the kernel
> code above; the kernel not liking the value does not make the field
> invalid.)
See Michael's reply, the spec allows read the config before setting
features.
>
> Maybe a statement covering everything would be:
>
> "The following driver-read-only field mtu only exists if the device
> offers VIRTIO_NET_F_MTU and may be read by the driver during feature
> negotiation and after VIRTIO_NET_F_MTU has been successfully
> negotiated."
>
>>
>>> Should we add a wording clarification to the spec?
>>
>> I think so.
> Some clarification would be needed for each field that depends on a
> feature; that would be quite verbose. Maybe we can get away with a
> clarifying statement?
>
> "Some config space fields may depend on a certain feature. In that
> case, the field exits if the device has offered the corresponding
> feature,
So this implies for !VIRTIO_NET_F_MQ && VIRTIO_NET_F_MTU, the config
will look like:
struct virtio_net_config {
u8 mac[6];
le16 status;
le16 mtu;
};
> and may be read by the driver during feature negotiation, and
> accessed by the driver after the feature has been successfully
> negotiated. A shorthand for this is a statement that a field only
> exists if a certain feature bit is set."
I'm not sure using "shorthand" is good for the spec, at least we can
limit the its scope only to the configuration space part.
Thanks
>
On Wed, 24 Feb 2021 17:29:07 +0800
Jason Wang <[email protected]> wrote:
> On 2021/2/23 6:58 下午, Cornelia Huck wrote:
> > On Tue, 23 Feb 2021 18:31:07 +0800
> > Jason Wang <[email protected]> wrote:
> >
> >> On 2021/2/23 6:04 下午, Cornelia Huck wrote:
> >>> On Tue, 23 Feb 2021 17:46:20 +0800
> >>> Jason Wang <[email protected]> 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?
> >>> I think both failing or not accepting the feature can be argued to make
> >>> sense: "the device presented us with a mtu size that does not make
> >>> sense" would point to failing, "we cannot work with the mtu size that
> >>> the device presented us" would point to not negotiating the feature.
> >>>
> >>>>
> >>>>> }
> >>>>>
> >>>>> 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.
> >>> Before FEATURES_OK is set by the driver, I guess it means "the device
> >>> has offered the feature";
> >>
> >> For me this part is ok since it clarify that it's the driver that set
> >> the bit.
> >>
> >>
> >>
> >>> during normal usage, it means "the feature
> >>> has been negotiated".
> >> /?
> >>
> >> It looks to me the feature negotiation is done only after device set
> >> FEATURES_OK, or FEATURES_OK could be read from device status?
> > I'd consider feature negotiation done when the driver reads FEATURES_OK
> > back from the status.
>
>
> I agree.
>
>
> >
> >>
> >>> (This is a bit fuzzy for legacy mode.)
> > ...because legacy does not have FEATURES_OK.
> >
> >>
> >> The problem is the MTU description for example:
> >>
> >> "The following driver-read-only field, mtu only exists if
> >> VIRTIO_NET_F_MTU is set."
> >>
> >> It looks to me need to use "if VIRTIO_NET_F_MTU is set by device".
> > "offered by the device"? I don't think it should 'disappear' from the
> > config space if the driver won't use it. (Same for other config space
> > fields that are tied to feature bits.)
>
>
> But what happens if e.g device doesn't offer VIRTIO_NET_F_MTU? It looks
> to according to the spec there will be no mtu field.
I think so, yes.
>
> And a more interesting case is VIRTIO_NET_F_MQ is not offered but
> VIRTIO_NET_F_MTU offered. To me, it means we don't have
> max_virtqueue_pairs but it's not how the driver is wrote today.
That would be a bug, but it seems to me that the virtio-net driver
reads max_virtqueue_pairs conditionally and handles absence of the
feature correctly?
>
>
> >
> >> Otherwise readers (at least for me), may think the MTU is only valid
> >> if driver set the bit.
> > I think it would still be 'valid' in the sense that it exists and has
> > some value in there filled in by the device, but a driver reading it
> > without negotiating the feature would be buggy. (Like in the kernel
> > code above; the kernel not liking the value does not make the field
> > invalid.)
>
>
> See Michael's reply, the spec allows read the config before setting
> features.
Yes, the period prior to finishing negotiation is obviously special.
>
>
> >
> > Maybe a statement covering everything would be:
> >
> > "The following driver-read-only field mtu only exists if the device
> > offers VIRTIO_NET_F_MTU and may be read by the driver during feature
> > negotiation and after VIRTIO_NET_F_MTU has been successfully
> > negotiated."
> >
> >>
> >>> Should we add a wording clarification to the spec?
> >>
> >> I think so.
> > Some clarification would be needed for each field that depends on a
> > feature; that would be quite verbose. Maybe we can get away with a
> > clarifying statement?
> >
> > "Some config space fields may depend on a certain feature. In that
> > case, the field exits if the device has offered the corresponding
> > feature,
>
>
> So this implies for !VIRTIO_NET_F_MQ && VIRTIO_NET_F_MTU, the config
> will look like:
>
> struct virtio_net_config {
> u8 mac[6];
> le16 status;
> le16 mtu;
> };
>
I agree.
>
> > and may be read by the driver during feature negotiation, and
> > accessed by the driver after the feature has been successfully
> > negotiated. A shorthand for this is a statement that a field only
> > exists if a certain feature bit is set."
>
>
> I'm not sure using "shorthand" is good for the spec, at least we can
> limit the its scope only to the configuration space part.
Maybe "a shorthand expression"?
On 2021/2/24 7:12 下午, Cornelia Huck wrote:
> On Wed, 24 Feb 2021 17:29:07 +0800
> Jason Wang <[email protected]> wrote:
>
>> On 2021/2/23 6:58 下午, Cornelia Huck wrote:
>>> On Tue, 23 Feb 2021 18:31:07 +0800
>>> Jason Wang <[email protected]> wrote:
>>>
>>>> On 2021/2/23 6:04 下午, Cornelia Huck wrote:
>>>>> On Tue, 23 Feb 2021 17:46:20 +0800
>>>>> Jason Wang <[email protected]> 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?
>>>>> I think both failing or not accepting the feature can be argued to make
>>>>> sense: "the device presented us with a mtu size that does not make
>>>>> sense" would point to failing, "we cannot work with the mtu size that
>>>>> the device presented us" would point to not negotiating the feature.
>>>>>
>>>>>>
>>>>>>> }
>>>>>>>
>>>>>>> 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.
>>>>> Before FEATURES_OK is set by the driver, I guess it means "the device
>>>>> has offered the feature";
>>>> For me this part is ok since it clarify that it's the driver that set
>>>> the bit.
>>>>
>>>>
>>>>
>>>>> during normal usage, it means "the feature
>>>>> has been negotiated".
>>>> /?
>>>>
>>>> It looks to me the feature negotiation is done only after device set
>>>> FEATURES_OK, or FEATURES_OK could be read from device status?
>>> I'd consider feature negotiation done when the driver reads FEATURES_OK
>>> back from the status.
>>
>> I agree.
>>
>>
>>>
>>>>
>>>>> (This is a bit fuzzy for legacy mode.)
>>> ...because legacy does not have FEATURES_OK.
>>>
>>>> The problem is the MTU description for example:
>>>>
>>>> "The following driver-read-only field, mtu only exists if
>>>> VIRTIO_NET_F_MTU is set."
>>>>
>>>> It looks to me need to use "if VIRTIO_NET_F_MTU is set by device".
>>> "offered by the device"? I don't think it should 'disappear' from the
>>> config space if the driver won't use it. (Same for other config space
>>> fields that are tied to feature bits.)
>>
>> But what happens if e.g device doesn't offer VIRTIO_NET_F_MTU? It looks
>> to according to the spec there will be no mtu field.
> I think so, yes.
>
>> And a more interesting case is VIRTIO_NET_F_MQ is not offered but
>> VIRTIO_NET_F_MTU offered. To me, it means we don't have
>> max_virtqueue_pairs but it's not how the driver is wrote today.
> That would be a bug, but it seems to me that the virtio-net driver
> reads max_virtqueue_pairs conditionally and handles absence of the
> feature correctly?
Yes, see the avove codes:
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);
}
So it's probably too late to fix the driver.
>
>>
>>>
>>>> Otherwise readers (at least for me), may think the MTU is only valid
>>>> if driver set the bit.
>>> I think it would still be 'valid' in the sense that it exists and has
>>> some value in there filled in by the device, but a driver reading it
>>> without negotiating the feature would be buggy. (Like in the kernel
>>> code above; the kernel not liking the value does not make the field
>>> invalid.)
>>
>> See Michael's reply, the spec allows read the config before setting
>> features.
> Yes, the period prior to finishing negotiation is obviously special.
>
>>
>>> Maybe a statement covering everything would be:
>>>
>>> "The following driver-read-only field mtu only exists if the device
>>> offers VIRTIO_NET_F_MTU and may be read by the driver during feature
>>> negotiation and after VIRTIO_NET_F_MTU has been successfully
>>> negotiated."
>>>
>>>>
>>>>> Should we add a wording clarification to the spec?
>>>> I think so.
>>> Some clarification would be needed for each field that depends on a
>>> feature; that would be quite verbose. Maybe we can get away with a
>>> clarifying statement?
>>>
>>> "Some config space fields may depend on a certain feature. In that
>>> case, the field exits if the device has offered the corresponding
>>> feature,
>>
>> So this implies for !VIRTIO_NET_F_MQ && VIRTIO_NET_F_MTU, the config
>> will look like:
>>
>> struct virtio_net_config {
>> u8 mac[6];
>> le16 status;
>> le16 mtu;
>> };
>>
> I agree.
So consider it's probably too late to fix the driver which assumes some
field are always persent, it looks to me need fix the spec do declare
the fields are always existing instead.
>
>>> and may be read by the driver during feature negotiation, and
>>> accessed by the driver after the feature has been successfully
>>> negotiated. A shorthand for this is a statement that a field only
>>> exists if a certain feature bit is set."
>>
>> I'm not sure using "shorthand" is good for the spec, at least we can
>> limit the its scope only to the configuration space part.
> Maybe "a shorthand expression"?
So the questions is should we use this for all over the spec or it will
be only used in this speicifc part (device configuration).
Thanks
On Thu, 25 Feb 2021 12:36:07 +0800
Jason Wang <[email protected]> wrote:
> On 2021/2/24 7:12 下午, Cornelia Huck wrote:
> > On Wed, 24 Feb 2021 17:29:07 +0800
> > Jason Wang <[email protected]> wrote:
> >
> >> On 2021/2/23 6:58 下午, Cornelia Huck wrote:
> >>> On Tue, 23 Feb 2021 18:31:07 +0800
> >>> Jason Wang <[email protected]> wrote:
> >>>> The problem is the MTU description for example:
> >>>>
> >>>> "The following driver-read-only field, mtu only exists if
> >>>> VIRTIO_NET_F_MTU is set."
> >>>>
> >>>> It looks to me need to use "if VIRTIO_NET_F_MTU is set by device".
> >>> "offered by the device"? I don't think it should 'disappear' from the
> >>> config space if the driver won't use it. (Same for other config space
> >>> fields that are tied to feature bits.)
> >>
> >> But what happens if e.g device doesn't offer VIRTIO_NET_F_MTU? It looks
> >> to according to the spec there will be no mtu field.
> > I think so, yes.
> >
> >> And a more interesting case is VIRTIO_NET_F_MQ is not offered but
> >> VIRTIO_NET_F_MTU offered. To me, it means we don't have
> >> max_virtqueue_pairs but it's not how the driver is wrote today.
> > That would be a bug, but it seems to me that the virtio-net driver
> > reads max_virtqueue_pairs conditionally and handles absence of the
> > feature correctly?
>
>
> Yes, see the avove codes:
>
> 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);
> }
>
Ouch, you're right. The virtio_cread accessors all operate on offsets
into a structure, it's just more obvious here.
> So it's probably too late to fix the driver.
It is never too late to fix a driver :)
It seems involved, though. We'd need different config space structures
based upon which set of features has been negotiated. It's not too bad
when features build upon each other and add fields at the end (this
should be fine right now, if the code remembered to check for the
feature), but can become ugly if an in-between field depends upon a
feature.
I guess we've been lucky that it seems to be an extremely uncommon
configuration.
>
>
> >
> >>
> >>>
> >>>> Otherwise readers (at least for me), may think the MTU is only valid
> >>>> if driver set the bit.
> >>> I think it would still be 'valid' in the sense that it exists and has
> >>> some value in there filled in by the device, but a driver reading it
> >>> without negotiating the feature would be buggy. (Like in the kernel
> >>> code above; the kernel not liking the value does not make the field
> >>> invalid.)
> >>
> >> See Michael's reply, the spec allows read the config before setting
> >> features.
> > Yes, the period prior to finishing negotiation is obviously special.
> >
> >>
> >>> Maybe a statement covering everything would be:
> >>>
> >>> "The following driver-read-only field mtu only exists if the device
> >>> offers VIRTIO_NET_F_MTU and may be read by the driver during feature
> >>> negotiation and after VIRTIO_NET_F_MTU has been successfully
> >>> negotiated."
> >>>
> >>>>
> >>>>> Should we add a wording clarification to the spec?
> >>>> I think so.
> >>> Some clarification would be needed for each field that depends on a
> >>> feature; that would be quite verbose. Maybe we can get away with a
> >>> clarifying statement?
> >>>
> >>> "Some config space fields may depend on a certain feature. In that
> >>> case, the field exits if the device has offered the corresponding
> >>> feature,
> >>
> >> So this implies for !VIRTIO_NET_F_MQ && VIRTIO_NET_F_MTU, the config
> >> will look like:
> >>
> >> struct virtio_net_config {
> >> u8 mac[6];
> >> le16 status;
> >> le16 mtu;
> >> };
> >>
> > I agree.
>
>
> So consider it's probably too late to fix the driver which assumes some
> field are always persent, it looks to me need fix the spec do declare
> the fields are always existing instead.
The problem with that is that it has been in the spec already, and a
compliant device that did not provide the fields without the features
would suddenly become non-compliant...
>
>
> >
> >>> and may be read by the driver during feature negotiation, and
> >>> accessed by the driver after the feature has been successfully
> >>> negotiated. A shorthand for this is a statement that a field only
> >>> exists if a certain feature bit is set."
> >>
> >> I'm not sure using "shorthand" is good for the spec, at least we can
> >> limit the its scope only to the configuration space part.
> > Maybe "a shorthand expression"?
>
>
> So the questions is should we use this for all over the spec or it will
> be only used in this speicifc part (device configuration).
For command structures and the like, "feature is set" should always
mean "feature has been negotiated"; I think config space is special
because the driver can read it before feature negotiation is finished,
so device configuration is probably the proper place for it.
On Thu, Feb 25, 2021 at 12:36:07PM +0800, Jason Wang wrote:
>
> On 2021/2/24 7:12 下午, Cornelia Huck wrote:
> > On Wed, 24 Feb 2021 17:29:07 +0800
> > Jason Wang <[email protected]> wrote:
> >
> > > On 2021/2/23 6:58 下午, Cornelia Huck wrote:
> > > > On Tue, 23 Feb 2021 18:31:07 +0800
> > > > Jason Wang <[email protected]> wrote:
> > > > > On 2021/2/23 6:04 下午, Cornelia Huck wrote:
> > > > > > On Tue, 23 Feb 2021 17:46:20 +0800
> > > > > > Jason Wang <[email protected]> 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?
> > > > > > I think both failing or not accepting the feature can be argued to make
> > > > > > sense: "the device presented us with a mtu size that does not make
> > > > > > sense" would point to failing, "we cannot work with the mtu size that
> > > > > > the device presented us" would point to not negotiating the feature.
> > > > > > > > }
> > > > > > > >
> > > > > > > > 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.
> > > > > > Before FEATURES_OK is set by the driver, I guess it means "the device
> > > > > > has offered the feature";
> > > > > For me this part is ok since it clarify that it's the driver that set
> > > > > the bit.
> > > > >
> > > > >
> > > > > > during normal usage, it means "the feature
> > > > > > has been negotiated".
> > > > > /?
> > > > >
> > > > > It looks to me the feature negotiation is done only after device set
> > > > > FEATURES_OK, or FEATURES_OK could be read from device status?
> > > > I'd consider feature negotiation done when the driver reads FEATURES_OK
> > > > back from the status.
> > >
> > > I agree.
> > >
> > >
> > > > > > (This is a bit fuzzy for legacy mode.)
> > > > ...because legacy does not have FEATURES_OK.
> > > > > The problem is the MTU description for example:
> > > > >
> > > > > "The following driver-read-only field, mtu only exists if
> > > > > VIRTIO_NET_F_MTU is set."
> > > > >
> > > > > It looks to me need to use "if VIRTIO_NET_F_MTU is set by device".
> > > > "offered by the device"? I don't think it should 'disappear' from the
> > > > config space if the driver won't use it. (Same for other config space
> > > > fields that are tied to feature bits.)
> > >
> > > But what happens if e.g device doesn't offer VIRTIO_NET_F_MTU? It looks
> > > to according to the spec there will be no mtu field.
> > I think so, yes.
> >
> > > And a more interesting case is VIRTIO_NET_F_MQ is not offered but
> > > VIRTIO_NET_F_MTU offered. To me, it means we don't have
> > > max_virtqueue_pairs but it's not how the driver is wrote today.
> > That would be a bug, but it seems to me that the virtio-net driver
> > reads max_virtqueue_pairs conditionally and handles absence of the
> > feature correctly?
>
>
> Yes, see the avove codes:
>
> 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);
> }
>
> So it's probably too late to fix the driver.
>
Confused. What is wrong with the above? It never reads the
field unless the feature has been offered by device.
> >
> > >
> > > > > Otherwise readers (at least for me), may think the MTU is only valid
> > > > > if driver set the bit.
> > > > I think it would still be 'valid' in the sense that it exists and has
> > > > some value in there filled in by the device, but a driver reading it
> > > > without negotiating the feature would be buggy. (Like in the kernel
> > > > code above; the kernel not liking the value does not make the field
> > > > invalid.)
> > >
> > > See Michael's reply, the spec allows read the config before setting
> > > features.
> > Yes, the period prior to finishing negotiation is obviously special.
> >
> > >
> > > > Maybe a statement covering everything would be:
> > > >
> > > > "The following driver-read-only field mtu only exists if the device
> > > > offers VIRTIO_NET_F_MTU and may be read by the driver during feature
> > > > negotiation and after VIRTIO_NET_F_MTU has been successfully
> > > > negotiated."
> > > > > > Should we add a wording clarification to the spec?
> > > > > I think so.
> > > > Some clarification would be needed for each field that depends on a
> > > > feature; that would be quite verbose. Maybe we can get away with a
> > > > clarifying statement?
> > > >
> > > > "Some config space fields may depend on a certain feature. In that
> > > > case, the field exits if the device has offered the corresponding
> > > > feature,
> > >
> > > So this implies for !VIRTIO_NET_F_MQ && VIRTIO_NET_F_MTU, the config
> > > will look like:
> > >
> > > struct virtio_net_config {
> > > u8 mac[6];
> > > le16 status;
> > > le16 mtu;
> > > };
> > >
> > I agree.
>
>
> So consider it's probably too late to fix the driver which assumes some
> field are always persent, it looks to me need fix the spec do declare the
> fields are always existing instead.
>
>
> >
> > > > and may be read by the driver during feature negotiation, and
> > > > accessed by the driver after the feature has been successfully
> > > > negotiated. A shorthand for this is a statement that a field only
> > > > exists if a certain feature bit is set."
> > >
> > > I'm not sure using "shorthand" is good for the spec, at least we can
> > > limit the its scope only to the configuration space part.
> > Maybe "a shorthand expression"?
>
>
> So the questions is should we use this for all over the spec or it will be
> only used in this speicifc part (device configuration).
>
> Thanks
>
On 2021/2/26 2:53 上午, Michael S. Tsirkin wrote:
> On Thu, Feb 25, 2021 at 12:36:07PM +0800, Jason Wang wrote:
>> On 2021/2/24 7:12 下午, Cornelia Huck wrote:
>>> On Wed, 24 Feb 2021 17:29:07 +0800
>>> Jason Wang <[email protected]> wrote:
>>>
>>>> On 2021/2/23 6:58 下午, Cornelia Huck wrote:
>>>>> On Tue, 23 Feb 2021 18:31:07 +0800
>>>>> Jason Wang <[email protected]> wrote:
>>>>>> On 2021/2/23 6:04 下午, Cornelia Huck wrote:
>>>>>>> On Tue, 23 Feb 2021 17:46:20 +0800
>>>>>>> Jason Wang <[email protected]> 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?
>>>>>>> I think both failing or not accepting the feature can be argued to make
>>>>>>> sense: "the device presented us with a mtu size that does not make
>>>>>>> sense" would point to failing, "we cannot work with the mtu size that
>>>>>>> the device presented us" would point to not negotiating the feature.
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>> Before FEATURES_OK is set by the driver, I guess it means "the device
>>>>>>> has offered the feature";
>>>>>> For me this part is ok since it clarify that it's the driver that set
>>>>>> the bit.
>>>>>>
>>>>>>
>>>>>>> during normal usage, it means "the feature
>>>>>>> has been negotiated".
>>>>>> /?
>>>>>>
>>>>>> It looks to me the feature negotiation is done only after device set
>>>>>> FEATURES_OK, or FEATURES_OK could be read from device status?
>>>>> I'd consider feature negotiation done when the driver reads FEATURES_OK
>>>>> back from the status.
>>>> I agree.
>>>>
>>>>
>>>>>>> (This is a bit fuzzy for legacy mode.)
>>>>> ...because legacy does not have FEATURES_OK.
>>>>>> The problem is the MTU description for example:
>>>>>>
>>>>>> "The following driver-read-only field, mtu only exists if
>>>>>> VIRTIO_NET_F_MTU is set."
>>>>>>
>>>>>> It looks to me need to use "if VIRTIO_NET_F_MTU is set by device".
>>>>> "offered by the device"? I don't think it should 'disappear' from the
>>>>> config space if the driver won't use it. (Same for other config space
>>>>> fields that are tied to feature bits.)
>>>> But what happens if e.g device doesn't offer VIRTIO_NET_F_MTU? It looks
>>>> to according to the spec there will be no mtu field.
>>> I think so, yes.
>>>
>>>> And a more interesting case is VIRTIO_NET_F_MQ is not offered but
>>>> VIRTIO_NET_F_MTU offered. To me, it means we don't have
>>>> max_virtqueue_pairs but it's not how the driver is wrote today.
>>> That would be a bug, but it seems to me that the virtio-net driver
>>> reads max_virtqueue_pairs conditionally and handles absence of the
>>> feature correctly?
>>
>> Yes, see the avove codes:
>>
>> 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);
>> }
>>
>> So it's probably too late to fix the driver.
>>
> Confused. What is wrong with the above? It never reads the
> field unless the feature has been offered by device.
So the spec said:
"
The following driver-read-only field, max_virtqueue_pairs only exists if
VIRTIO_NET_F_MQ is set.
"
If I read this correctly, there will be no max_virtqueue_pairs field if
the VIRTIO_NET_F_MQ is not offered by device? If yes the offsetof()
violates what spec said.
Thanks
>
>
>>>>>> Otherwise readers (at least for me), may think the MTU is only valid
>>>>>> if driver set the bit.
>>>>> I think it would still be 'valid' in the sense that it exists and has
>>>>> some value in there filled in by the device, but a driver reading it
>>>>> without negotiating the feature would be buggy. (Like in the kernel
>>>>> code above; the kernel not liking the value does not make the field
>>>>> invalid.)
>>>> See Michael's reply, the spec allows read the config before setting
>>>> features.
>>> Yes, the period prior to finishing negotiation is obviously special.
>>>
>>>>> Maybe a statement covering everything would be:
>>>>>
>>>>> "The following driver-read-only field mtu only exists if the device
>>>>> offers VIRTIO_NET_F_MTU and may be read by the driver during feature
>>>>> negotiation and after VIRTIO_NET_F_MTU has been successfully
>>>>> negotiated."
>>>>>>> Should we add a wording clarification to the spec?
>>>>>> I think so.
>>>>> Some clarification would be needed for each field that depends on a
>>>>> feature; that would be quite verbose. Maybe we can get away with a
>>>>> clarifying statement?
>>>>>
>>>>> "Some config space fields may depend on a certain feature. In that
>>>>> case, the field exits if the device has offered the corresponding
>>>>> feature,
>>>> So this implies for !VIRTIO_NET_F_MQ && VIRTIO_NET_F_MTU, the config
>>>> will look like:
>>>>
>>>> struct virtio_net_config {
>>>> u8 mac[6];
>>>> le16 status;
>>>> le16 mtu;
>>>> };
>>>>
>>> I agree.
>>
>> So consider it's probably too late to fix the driver which assumes some
>> field are always persent, it looks to me need fix the spec do declare the
>> fields are always existing instead.
>>
>>
>>>>> and may be read by the driver during feature negotiation, and
>>>>> accessed by the driver after the feature has been successfully
>>>>> negotiated. A shorthand for this is a statement that a field only
>>>>> exists if a certain feature bit is set."
>>>> I'm not sure using "shorthand" is good for the spec, at least we can
>>>> limit the its scope only to the configuration space part.
>>> Maybe "a shorthand expression"?
>>
>> So the questions is should we use this for all over the spec or it will be
>> only used in this speicifc part (device configuration).
>>
>> Thanks
>>
On Fri, Feb 26, 2021 at 04:19:16PM +0800, Jason Wang wrote:
>
> On 2021/2/26 2:53 上午, Michael S. Tsirkin wrote:
> > On Thu, Feb 25, 2021 at 12:36:07PM +0800, Jason Wang wrote:
> > > On 2021/2/24 7:12 下午, Cornelia Huck wrote:
> > > > On Wed, 24 Feb 2021 17:29:07 +0800
> > > > Jason Wang <[email protected]> wrote:
> > > >
> > > > > On 2021/2/23 6:58 下午, Cornelia Huck wrote:
> > > > > > On Tue, 23 Feb 2021 18:31:07 +0800
> > > > > > Jason Wang <[email protected]> wrote:
> > > > > > > On 2021/2/23 6:04 下午, Cornelia Huck wrote:
> > > > > > > > On Tue, 23 Feb 2021 17:46:20 +0800
> > > > > > > > Jason Wang <[email protected]> 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?
> > > > > > > > I think both failing or not accepting the feature can be argued to make
> > > > > > > > sense: "the device presented us with a mtu size that does not make
> > > > > > > > sense" would point to failing, "we cannot work with the mtu size that
> > > > > > > > the device presented us" would point to not negotiating the feature.
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > 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.
> > > > > > > > Before FEATURES_OK is set by the driver, I guess it means "the device
> > > > > > > > has offered the feature";
> > > > > > > For me this part is ok since it clarify that it's the driver that set
> > > > > > > the bit.
> > > > > > >
> > > > > > >
> > > > > > > > during normal usage, it means "the feature
> > > > > > > > has been negotiated".
> > > > > > > /?
> > > > > > >
> > > > > > > It looks to me the feature negotiation is done only after device set
> > > > > > > FEATURES_OK, or FEATURES_OK could be read from device status?
> > > > > > I'd consider feature negotiation done when the driver reads FEATURES_OK
> > > > > > back from the status.
> > > > > I agree.
> > > > >
> > > > >
> > > > > > > > (This is a bit fuzzy for legacy mode.)
> > > > > > ...because legacy does not have FEATURES_OK.
> > > > > > > The problem is the MTU description for example:
> > > > > > >
> > > > > > > "The following driver-read-only field, mtu only exists if
> > > > > > > VIRTIO_NET_F_MTU is set."
> > > > > > >
> > > > > > > It looks to me need to use "if VIRTIO_NET_F_MTU is set by device".
> > > > > > "offered by the device"? I don't think it should 'disappear' from the
> > > > > > config space if the driver won't use it. (Same for other config space
> > > > > > fields that are tied to feature bits.)
> > > > > But what happens if e.g device doesn't offer VIRTIO_NET_F_MTU? It looks
> > > > > to according to the spec there will be no mtu field.
> > > > I think so, yes.
> > > >
> > > > > And a more interesting case is VIRTIO_NET_F_MQ is not offered but
> > > > > VIRTIO_NET_F_MTU offered. To me, it means we don't have
> > > > > max_virtqueue_pairs but it's not how the driver is wrote today.
> > > > That would be a bug, but it seems to me that the virtio-net driver
> > > > reads max_virtqueue_pairs conditionally and handles absence of the
> > > > feature correctly?
> > >
> > > Yes, see the avove codes:
> > >
> > > 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);
> > > }
> > >
> > > So it's probably too late to fix the driver.
> > >
> > Confused. What is wrong with the above? It never reads the
> > field unless the feature has been offered by device.
>
>
> So the spec said:
>
> "
>
> The following driver-read-only field, max_virtqueue_pairs only exists if
> VIRTIO_NET_F_MQ is set.
>
> "
>
> If I read this correctly, there will be no max_virtqueue_pairs field if the
> VIRTIO_NET_F_MQ is not offered by device? If yes the offsetof() violates
> what spec said.
>
> Thanks
I think that's a misunderstanding. This text was never intended to
imply that field offsets change beased on feature bits.
We had this pain with legacy and we never wanted to go back there.
This merely implies that without VIRTIO_NET_F_MQ the field
should not be accessed. Exists in the sense "is accessible to driver".
Let's just clarify that in the spec, job done.
>
> >
> >
> > > > > > > Otherwise readers (at least for me), may think the MTU is only valid
> > > > > > > if driver set the bit.
> > > > > > I think it would still be 'valid' in the sense that it exists and has
> > > > > > some value in there filled in by the device, but a driver reading it
> > > > > > without negotiating the feature would be buggy. (Like in the kernel
> > > > > > code above; the kernel not liking the value does not make the field
> > > > > > invalid.)
> > > > > See Michael's reply, the spec allows read the config before setting
> > > > > features.
> > > > Yes, the period prior to finishing negotiation is obviously special.
> > > >
> > > > > > Maybe a statement covering everything would be:
> > > > > >
> > > > > > "The following driver-read-only field mtu only exists if the device
> > > > > > offers VIRTIO_NET_F_MTU and may be read by the driver during feature
> > > > > > negotiation and after VIRTIO_NET_F_MTU has been successfully
> > > > > > negotiated."
> > > > > > > > Should we add a wording clarification to the spec?
> > > > > > > I think so.
> > > > > > Some clarification would be needed for each field that depends on a
> > > > > > feature; that would be quite verbose. Maybe we can get away with a
> > > > > > clarifying statement?
> > > > > >
> > > > > > "Some config space fields may depend on a certain feature. In that
> > > > > > case, the field exits if the device has offered the corresponding
> > > > > > feature,
> > > > > So this implies for !VIRTIO_NET_F_MQ && VIRTIO_NET_F_MTU, the config
> > > > > will look like:
> > > > >
> > > > > struct virtio_net_config {
> > > > > u8 mac[6];
> > > > > le16 status;
> > > > > le16 mtu;
> > > > > };
> > > > >
> > > > I agree.
> > >
> > > So consider it's probably too late to fix the driver which assumes some
> > > field are always persent, it looks to me need fix the spec do declare the
> > > fields are always existing instead.
> > >
> > >
> > > > > > and may be read by the driver during feature negotiation, and
> > > > > > accessed by the driver after the feature has been successfully
> > > > > > negotiated. A shorthand for this is a statement that a field only
> > > > > > exists if a certain feature bit is set."
> > > > > I'm not sure using "shorthand" is good for the spec, at least we can
> > > > > limit the its scope only to the configuration space part.
> > > > Maybe "a shorthand expression"?
> > >
> > > So the questions is should we use this for all over the spec or it will be
> > > only used in this speicifc part (device configuration).
> > >
> > > Thanks
> > >
On 2021/3/1 5:25 上午, Michael S. Tsirkin wrote:
> On Fri, Feb 26, 2021 at 04:19:16PM +0800, Jason Wang wrote:
>> On 2021/2/26 2:53 上午, Michael S. Tsirkin wrote:
>>> On Thu, Feb 25, 2021 at 12:36:07PM +0800, Jason Wang wrote:
>>>> On 2021/2/24 7:12 下午, Cornelia Huck wrote:
>>>>> On Wed, 24 Feb 2021 17:29:07 +0800
>>>>> Jason Wang <[email protected]> wrote:
>>>>>
>>>>>> On 2021/2/23 6:58 下午, Cornelia Huck wrote:
>>>>>>> On Tue, 23 Feb 2021 18:31:07 +0800
>>>>>>> Jason Wang <[email protected]> wrote:
>>>>>>>> On 2021/2/23 6:04 下午, Cornelia Huck wrote:
>>>>>>>>> On Tue, 23 Feb 2021 17:46:20 +0800
>>>>>>>>> Jason Wang <[email protected]> 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?
>>>>>>>>> I think both failing or not accepting the feature can be argued to make
>>>>>>>>> sense: "the device presented us with a mtu size that does not make
>>>>>>>>> sense" would point to failing, "we cannot work with the mtu size that
>>>>>>>>> the device presented us" would point to not negotiating the feature.
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> 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.
>>>>>>>>> Before FEATURES_OK is set by the driver, I guess it means "the device
>>>>>>>>> has offered the feature";
>>>>>>>> For me this part is ok since it clarify that it's the driver that set
>>>>>>>> the bit.
>>>>>>>>
>>>>>>>>
>>>>>>>>> during normal usage, it means "the feature
>>>>>>>>> has been negotiated".
>>>>>>>> /?
>>>>>>>>
>>>>>>>> It looks to me the feature negotiation is done only after device set
>>>>>>>> FEATURES_OK, or FEATURES_OK could be read from device status?
>>>>>>> I'd consider feature negotiation done when the driver reads FEATURES_OK
>>>>>>> back from the status.
>>>>>> I agree.
>>>>>>
>>>>>>
>>>>>>>>> (This is a bit fuzzy for legacy mode.)
>>>>>>> ...because legacy does not have FEATURES_OK.
>>>>>>>> The problem is the MTU description for example:
>>>>>>>>
>>>>>>>> "The following driver-read-only field, mtu only exists if
>>>>>>>> VIRTIO_NET_F_MTU is set."
>>>>>>>>
>>>>>>>> It looks to me need to use "if VIRTIO_NET_F_MTU is set by device".
>>>>>>> "offered by the device"? I don't think it should 'disappear' from the
>>>>>>> config space if the driver won't use it. (Same for other config space
>>>>>>> fields that are tied to feature bits.)
>>>>>> But what happens if e.g device doesn't offer VIRTIO_NET_F_MTU? It looks
>>>>>> to according to the spec there will be no mtu field.
>>>>> I think so, yes.
>>>>>
>>>>>> And a more interesting case is VIRTIO_NET_F_MQ is not offered but
>>>>>> VIRTIO_NET_F_MTU offered. To me, it means we don't have
>>>>>> max_virtqueue_pairs but it's not how the driver is wrote today.
>>>>> That would be a bug, but it seems to me that the virtio-net driver
>>>>> reads max_virtqueue_pairs conditionally and handles absence of the
>>>>> feature correctly?
>>>> Yes, see the avove codes:
>>>>
>>>> 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);
>>>> }
>>>>
>>>> So it's probably too late to fix the driver.
>>>>
>>> Confused. What is wrong with the above? It never reads the
>>> field unless the feature has been offered by device.
>>
>> So the spec said:
>>
>> "
>>
>> The following driver-read-only field, max_virtqueue_pairs only exists if
>> VIRTIO_NET_F_MQ is set.
>>
>> "
>>
>> If I read this correctly, there will be no max_virtqueue_pairs field if the
>> VIRTIO_NET_F_MQ is not offered by device? If yes the offsetof() violates
>> what spec said.
>>
>> Thanks
> I think that's a misunderstanding. This text was never intended to
> imply that field offsets change beased on feature bits.
> We had this pain with legacy and we never wanted to go back there.
>
> This merely implies that without VIRTIO_NET_F_MQ the field
> should not be accessed. Exists in the sense "is accessible to driver".
>
> Let's just clarify that in the spec, job done.
Ok, agree. That will make things more eaiser.
Thanks
>
>
>
>
>>>
>>>>>>>> Otherwise readers (at least for me), may think the MTU is only valid
>>>>>>>> if driver set the bit.
>>>>>>> I think it would still be 'valid' in the sense that it exists and has
>>>>>>> some value in there filled in by the device, but a driver reading it
>>>>>>> without negotiating the feature would be buggy. (Like in the kernel
>>>>>>> code above; the kernel not liking the value does not make the field
>>>>>>> invalid.)
>>>>>> See Michael's reply, the spec allows read the config before setting
>>>>>> features.
>>>>> Yes, the period prior to finishing negotiation is obviously special.
>>>>>
>>>>>>> Maybe a statement covering everything would be:
>>>>>>>
>>>>>>> "The following driver-read-only field mtu only exists if the device
>>>>>>> offers VIRTIO_NET_F_MTU and may be read by the driver during feature
>>>>>>> negotiation and after VIRTIO_NET_F_MTU has been successfully
>>>>>>> negotiated."
>>>>>>>>> Should we add a wording clarification to the spec?
>>>>>>>> I think so.
>>>>>>> Some clarification would be needed for each field that depends on a
>>>>>>> feature; that would be quite verbose. Maybe we can get away with a
>>>>>>> clarifying statement?
>>>>>>>
>>>>>>> "Some config space fields may depend on a certain feature. In that
>>>>>>> case, the field exits if the device has offered the corresponding
>>>>>>> feature,
>>>>>> So this implies for !VIRTIO_NET_F_MQ && VIRTIO_NET_F_MTU, the config
>>>>>> will look like:
>>>>>>
>>>>>> struct virtio_net_config {
>>>>>> u8 mac[6];
>>>>>> le16 status;
>>>>>> le16 mtu;
>>>>>> };
>>>>>>
>>>>> I agree.
>>>> So consider it's probably too late to fix the driver which assumes some
>>>> field are always persent, it looks to me need fix the spec do declare the
>>>> fields are always existing instead.
>>>>
>>>>
>>>>>>> and may be read by the driver during feature negotiation, and
>>>>>>> accessed by the driver after the feature has been successfully
>>>>>>> negotiated. A shorthand for this is a statement that a field only
>>>>>>> exists if a certain feature bit is set."
>>>>>> I'm not sure using "shorthand" is good for the spec, at least we can
>>>>>> limit the its scope only to the configuration space part.
>>>>> Maybe "a shorthand expression"?
>>>> So the questions is should we use this for all over the spec or it will be
>>>> only used in this speicifc part (device configuration).
>>>>
>>>> Thanks
>>>>
On Mon, 1 Mar 2021 11:51:08 +0800
Jason Wang <[email protected]> wrote:
> On 2021/3/1 5:25 上午, Michael S. Tsirkin wrote:
> > On Fri, Feb 26, 2021 at 04:19:16PM +0800, Jason Wang wrote:
> >> On 2021/2/26 2:53 上午, Michael S. Tsirkin wrote:
> >>> Confused. What is wrong with the above? It never reads the
> >>> field unless the feature has been offered by device.
> >>
> >> So the spec said:
> >>
> >> "
> >>
> >> The following driver-read-only field, max_virtqueue_pairs only exists if
> >> VIRTIO_NET_F_MQ is set.
> >>
> >> "
> >>
> >> If I read this correctly, there will be no max_virtqueue_pairs field if the
> >> VIRTIO_NET_F_MQ is not offered by device? If yes the offsetof() violates
> >> what spec said.
> >>
> >> Thanks
> > I think that's a misunderstanding. This text was never intended to
> > imply that field offsets change beased on feature bits.
> > We had this pain with legacy and we never wanted to go back there.
> >
> > This merely implies that without VIRTIO_NET_F_MQ the field
> > should not be accessed. Exists in the sense "is accessible to driver".
> >
> > Let's just clarify that in the spec, job done.
>
>
> Ok, agree. That will make things more eaiser.
Yes, that makes much more sense.
What about adding the following to the "Basic Facilities of a Virtio
Device/Device Configuration Space" section of the spec:
"If an optional configuration field does not exist, the corresponding
space is still present, but reserved."
(Do we need to specify what a device needs to do if the driver tries to
access a non-existing field? We cannot really make assumptions about
config space accesses; virtio-ccw can just copy a chunk of config space
that contains non-existing fields... I guess the device could ignore
writes and return zeroes on read?)
I've opened https://github.com/oasis-tcs/virtio-spec/issues/98 for the
spec issues.