Jason Wang
2021-Mar-01 03:51 UTC
[virtio-dev] Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero
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 <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 > 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 >>>>
Cornelia Huck
2021-Mar-02 12:08 UTC
[virtio-dev] Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero
On Mon, 1 Mar 2021 11:51:08 +0800 Jason Wang <jasowang at redhat.com> 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.