OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

Subject: Re: [virtio-dev] Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero

On 2021/3/4 9:50 äå, Cornelia Huck wrote:
On Thu, 4 Mar 2021 16:24:16 +0800
Jason Wang <jasowang@redhat.com> wrote:

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


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.

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)},
   ÂÂÂÂ .end = endof(struct virtio_net_config, duplex)},
   ÂÂÂ {.flags = (1ULL << VIRTIO_NET_F_RSS) | (1ULL <<
   ÂÂÂÂ .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

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?
What about sidestepping this:

"...the corresponding space will still be present, unless no further
configuration fields exist."


It might work. (I wonder maybe we can give some example in the spec).


implies that a given field, if it exists, is always at the same offset
from the beginning of the configuration space."

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]