Michael S. Tsirkin
2021-Feb-25 18:53 UTC
[virtio-dev] Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero
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 <jasowang at redhat.com> wrote: > > > > > On 2021/2/23 6:58 ??, Cornelia Huck wrote: > > > > On Tue, 23 Feb 2021 18:31:07 +0800 > > > > Jason Wang <jasowang at redhat.com> wrote: > > > > > On 2021/2/23 6:04 ??, Cornelia Huck wrote: > > > > > > On Tue, 23 Feb 2021 17:46:20 +0800 > > > > > > Jason Wang <jasowang at redhat.com> 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 >
Jason Wang
2021-Feb-26 08:19 UTC
[virtio-dev] Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero
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 <jasowang at redhat.com> wrote: >>> >>>> On 2021/2/23 6:58 ??, Cornelia Huck wrote: >>>>> On Tue, 23 Feb 2021 18:31:07 +0800 >>>>> Jason Wang <jasowang at redhat.com> wrote: >>>>>> On 2021/2/23 6:04 ??, Cornelia Huck wrote: >>>>>>> On Tue, 23 Feb 2021 17:46:20 +0800 >>>>>>> Jason Wang <jasowang at redhat.com> 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 >>