On Thu, Jul 28, 2022 at 2:41 PM Michael S. Tsirkin [off-list ref] wrote:

On Thu, Jul 28, 2022 at 01:53:51PM +0800, Jason Wang wrote:

quoted

On Thu, Jul 28, 2022 at 11:47 AM Zhu, Lingshan [off-list ref] wrote:

quoted

On 7/28/2022 9:21 AM, Jason Wang wrote:

quoted

On Wed, Jul 27, 2022 at 11:45 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Wed, Jul 27, 2022 at 05:50:59PM +0800, Jason Wang wrote:

quoted

On Wed, Jul 27, 2022 at 5:03 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Wed, Jul 27, 2022 at 02:54:13PM +0800, Jason Wang wrote:

quoted

On Wed, Jul 27, 2022 at 2:01 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Wed, Jul 27, 2022 at 03:47:35AM +0000, Parav Pandit wrote:

quoted

quoted

From: Zhu, Lingshan <redacted>
Sent: Tuesday, July 26, 2022 10:53 PM

On 7/27/2022 10:17 AM, Parav Pandit wrote:

quoted

quoted

From: Zhu, Lingshan <redacted>
Sent: Tuesday, July 26, 2022 10:15 PM

On 7/26/2022 11:56 PM, Parav Pandit wrote:

quoted

quoted

From: Zhu, Lingshan <redacted>
Sent: Tuesday, July 12, 2022 11:46 PM

quoted

When the user space which invokes netlink commands, detects that

_MQ

quoted

quoted

is not supported, hence it takes max_queue_pair = 1 by itself.
I think the kernel module have all necessary information and it is
the only one which have precise information of a device, so it
should answer precisely than let the user space guess. The kernel
module should be reliable than stay silent, leave the question to
the user space

tool.

quoted

Kernel is reliable. It doesn’t expose a config space field if the
field doesn’t

exist regardless of field should have default or no default.
so when you know it is one queue pair, you should answer one, not try
to guess.

quoted

User space should not guess either. User space gets to see if _MQ

present/not present. If _MQ present than get reliable data from kernel.

quoted

If _MQ not present, it means this device has one VQ pair.

it is still a guess, right? And all user space tools implemented this
feature need to guess

No. it is not a guess.
It is explicitly checking the _MQ feature and deriving the value.
The code you proposed will be present in the user space.
It will be uniform for _MQ and 10 other features that are present now and

in the future.
MQ and other features like RSS are different. If there is no _RSS_XX, there
are no attributes like max_rss_key_size, and there is not a default value.
But for MQ, we know it has to be 1 wihtout _MQ.

"we" = user space.
To keep the consistency among all the config space fields.

Actually I looked and the code some more and I'm puzzled:


         struct virtio_net_config config = {};
         u64 features;
         u16 val_u16;

         vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config));

         if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac),
                     config.mac))
                 return -EMSGSIZE;


Mac returned even without VIRTIO_NET_F_MAC


         val_u16 = le16_to_cpu(config.status);
         if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
                 return -EMSGSIZE;


status returned even without VIRTIO_NET_F_STATUS

         val_u16 = le16_to_cpu(config.mtu);
         if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
                 return -EMSGSIZE;


MTU returned even without VIRTIO_NET_F_MTU


What's going on here?

Probably too late to fix, but this should be fine as long as all
parents support STATUS/MTU/MAC.

Why is this too late to fix.

If we make this conditional on the features. This may break the
userspace that always expects VDPA_ATTR_DEV_NET_CFG_MTU?

Thanks

Well only on devices without MTU. I'm saying said userspace
was reading trash on such devices anyway.

It depends on the parent actually. For example, mlx5 query the lower
mtu unconditionally:

         err = query_mtu(mdev, &mtu);
         if (err)
                 goto err_alloc;

         ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);

Supporting MTU features seems to be a must for real hardware.
Otherwise the driver may not work correctly.

quoted

We don't generally maintain bug for bug compatiblity on a whim,
only if userspace is actually known to break if we fix a bug.

  So I think it should be fine to make this conditional then we should
have a consistent handling of other fields like MQ.

For some fields that have a default value, like MQ =1, we can return the
default value.
For other fields without a default value, like MAC, we return nothing.

Does this sounds good? So, for MTU, if without _F_MTU, I think we can
return 1500 by default.

Or we can just read MTU from the device.

But It looks to me Michael wants it conditional.

Thanks

I'm fine either way but let's keep it consistent. And I think
Parav wants it conditional.

Parav, what's your opinion here?

Michale spots some in-consistent stuffs, so I think we should either

1) make all conditional, so we should change both MTU and MAC

or

2) make them unconditional, so we should only change MQ

Thanks

quoted

quoted

Thanks,
Zhu Lingshan

quoted

Thanks

quoted

quoted

quoted

quoted

I wonder if we can add a check in the core and fail the device
registration in this case.

Thanks

quoted

--
MST