lkml.org 
[lkml]   [2021]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [virtio-dev] Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero
    From
    Date

    On 2021/3/3 4:29 下午, Cornelia Huck wrote:
    > On Wed, 3 Mar 2021 12:01:01 +0800
    > Jason Wang <jasowang@redhat.com> wrote:
    >
    >> On 2021/3/2 8:08 下午, Cornelia Huck wrote:
    >>> On Mon, 1 Mar 2021 11:51:08 +0800
    >>> Jason Wang <jasowang@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."
    >>
    >> This became interesting after re-reading some of the qemu codes.
    >>
    >> E.g in virtio-net.c we had:
    >>
    >> *static VirtIOFeature feature_sizes[] = {
    >>     {.flags = 1ULL << VIRTIO_NET_F_MAC,
    >>      .end = endof(struct virtio_net_config, mac)},
    >>     {.flags = 1ULL << VIRTIO_NET_F_STATUS,
    >>      .end = endof(struct virtio_net_config, status)},
    >>     {.flags = 1ULL << VIRTIO_NET_F_MQ,
    >>      .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
    >>     {.flags = 1ULL << VIRTIO_NET_F_MTU,
    >>      .end = endof(struct virtio_net_config, mtu)},
    >>     {.flags = 1ULL << VIRTIO_NET_F_SPEED_DUPLEX,
    >>      .end = endof(struct virtio_net_config, duplex)},
    >>     {.flags = (1ULL << VIRTIO_NET_F_RSS) | (1ULL <<
    >> VIRTIO_NET_F_HASH_REPORT),
    >>      .end = endof(struct virtio_net_config, supported_hash_types)},
    >>     {}
    >> };*
    >>
    >> *It has a implict dependency chain. E.g MTU doesn't presnet if
    >> DUPLEX/RSS is not offered ...
    >> *
    > But I think it covers everything up to the relevant field, no? So MTU
    > is included if we have the feature bit, even if we don't have
    > DUPLEX/RSS.
    >
    > Given that a config space may be shorter (but must not collapse
    > non-existing fields), maybe a better wording would be:
    >
    > "If an optional configuration field does not exist, the corresponding
    > space will still be present if it is not at the end of the
    > configuration space (i.e., further configuration fields exist.)


    This should work but I think we need to define the end of configuration
    space first?


    > This
    > implies that a given field, if it exists, is always at the same offset
    > from the beginning of the configuration space."
    >
    >
    >>> (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...
    >>
    >> Right, it looks to me ccw doesn't depends on config len which is kind of
    >> interesting. I think the answer depends on the implementation of both
    >> transport and device:
    > (virtio-ccw is a bit odd, because channel I/O does not have the concept
    > of a config space and I needed to come up with something)


    Ok.


    >
    >> Let's take virtio-net-pci as an example, it fills status unconditionally
    >> in virtio_net_set_config() even if VIRTIO_NET_F_STATUS is not
    >> negotiated. So with the above feature_sizes:
    >>
    >> 1) if one of the MQ, MTU, DUPLEX or RSS is implemented, driver can still
    >> read status in this case
    >> 2) if none of the above four is negotied, driver can only read a 0xFF
    >> (virtio_config_readb())
    >>
    >>
    >>> I guess the device could ignore
    >>> writes and return zeroes on read?)
    >>
    >> So consider the above, it might be too late to fix/clarify that in the
    >> spec on the expected behaviour of reading/writing non-exist fields.
    > We could still advise behaviour via SHOULD, though I'm not sure it adds
    > much at this point in time.
    >
    > It really feels a bit odd that a driver can still read and write fields
    > for features that it did not negotiate, but I fear we're stuck with it.


    Yes, since the device (at least virtio-pci) implment thing like this.

    Thanks


    >
    >> Thanks
    >>
    >>
    >>> I've opened https://github.com/oasis-tcs/virtio-spec/issues/98 for the
    >>> spec issues.
    >>>
    >
    > ---------------------------------------------------------------------
    > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
    > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
    >

    \
     
     \ /
      Last update: 2021-03-04 09:28    [W:4.409 / U:0.744 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site