Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Cornelia Huck <cohuck@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	Si-Wei Liu <si-wei.liu@oracle.com>,
	elic@nvidia.com, linux-kernel@vger.kernel.org,
	virtualization@lists.linux-foundation.org,
	netdev@vger.kernel.org, virtio-dev@lists.oasis-open.org
Subject: Re: [virtio-dev] Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero
Date: Wed, 3 Mar 2021 09:29:05 +0100
Message-ID: <20210303092905.677eb66c.cohuck@redhat.com> (raw)
In-Reply-To: <5f6972fe-7246-b622-958d-9cab8dd98e21@redhat.com>

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
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)

> 
> 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.

> 
> Thanks
> 
> 
> >
> > I've opened https://github.com/oasis-tcs/virtio-spec/issues/98 for the
> > spec issues.
> >  


  parent reply index

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-19 11:54 Si-Wei Liu
2021-02-21 14:44 ` Eli Cohen
2021-02-21 21:52   ` Michael S. Tsirkin
2021-02-22  6:05     ` Eli Cohen
2021-02-23  9:26       ` Michael S. Tsirkin
2021-02-23  9:48         ` Jason Wang
2021-02-23  9:55           ` Michael S. Tsirkin
2021-02-22  4:14 ` Jason Wang
2021-02-22  7:34   ` Michael S. Tsirkin
2021-02-23  1:12     ` Si-Wei Liu
2021-02-23  2:03       ` Jason Wang
2021-02-23 13:26         ` Michael S. Tsirkin
2021-02-23 19:35           ` Si-Wei Liu
2021-02-24  3:20             ` Jason Wang
2021-02-24  5:17               ` Michael S. Tsirkin
2021-02-24  6:02                 ` Jason Wang
2021-02-24  6:45                 ` Eli Cohen
2021-02-24  6:47                   ` Michael S. Tsirkin
2021-02-24  6:55                     ` Jason Wang
2021-02-24  7:12                       ` Michael S. Tsirkin
2021-02-24 12:40                         ` Eli Cohen
2021-02-24  7:17                       ` Eli Cohen
2021-02-24  5:04             ` Michael S. Tsirkin
2021-02-24  6:04               ` Jason Wang
2021-02-24  6:46                 ` Michael S. Tsirkin
2021-02-24  6:53                   ` Jason Wang
2021-02-24  7:17                     ` Michael S. Tsirkin
     [not found]                       ` <babc654d-8dcd-d8a2-c3b6-d20cc4fc554c@redhat.com>
2021-02-24  8:43                         ` Michael S. Tsirkin
2021-02-24  9:30                           ` Jason Wang
2021-02-28 21:30                             ` Michael S. Tsirkin
2021-03-01  3:53                               ` Jason Wang
2021-02-24 18:24               ` Si-Wei Liu
2021-02-26  0:56                 ` Si-Wei Liu
2021-02-28 21:27                   ` Michael S. Tsirkin
2021-03-01 18:08                     ` Si-Wei Liu
2021-02-28 21:28                 ` Michael S. Tsirkin
2021-02-28 21:34                 ` Michael S. Tsirkin
2021-03-01  3:56                   ` Jason Wang
2021-03-02  9:47                     ` Michael S. Tsirkin
2021-03-02 10:53                       ` Jason Wang
2021-02-22 17:09   ` Si-Wei Liu
2021-02-23  2:03     ` Jason Wang
2021-02-23  9:25     ` Michael S. Tsirkin
2021-02-23  9:46       ` Jason Wang
2021-02-23 10:01         ` Michael S. Tsirkin
2021-02-23 10:17           ` Jason Wang
2021-02-24  9:40             ` Jason Wang
2021-02-23 10:04         ` [virtio-dev] " Cornelia Huck
2021-02-23 10:31           ` Jason Wang
2021-02-23 10:58             ` Cornelia Huck
2021-02-24  9:29               ` Jason Wang
2021-02-24 11:12                 ` Cornelia Huck
2021-02-25  4:36                   ` Jason Wang
2021-02-25 13:26                     ` Cornelia Huck
2021-02-25 18:53                     ` Michael S. Tsirkin
2021-02-26  8:19                       ` Jason Wang
2021-02-28 21:25                         ` Michael S. Tsirkin
2021-03-01  3:51                           ` Jason Wang
2021-03-02 12:08                             ` Cornelia Huck
     [not found]                               ` <5f6972fe-7246-b622-958d-9cab8dd98e21@redhat.com>
2021-03-03  8:29                                 ` Cornelia Huck [this message]
2021-03-04  8:24                                   ` Jason Wang
2021-03-04 13:50                                     ` Cornelia Huck
2021-03-05  3:01                                       ` Jason Wang
2021-02-23 12:26 ` Michael S. Tsirkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210303092905.677eb66c.cohuck@redhat.com \
    --to=cohuck@redhat.com \
    --cc=elic@nvidia.com \
    --cc=jasowang@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mst@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=si-wei.liu@oracle.com \
    --cc=virtio-dev@lists.oasis-open.org \
    --cc=virtualization@lists.linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git