On 7/27/2022 2:01 AM, Michael S. Tsirkin wrote:
On Wed, Jul 27, 2022 at 12:50:33AM -0700, Si-Wei Liu wrote:
On 7/26/2022 11:01 PM, Michael S. Tsirkin wrote:
On Wed, Jul 27, 2022 at 03:47:35AM +0000, Parav Pandit wrote:
From: Zhu, Lingshan <lingshan.zhu@xxxxxxxxx>
Sent: Tuesday, July 26, 2022 10:53 PM
On 7/27/2022 10:17 AM, Parav Pandit wrote:
From: Zhu, Lingshan <lingshan.zhu@xxxxxxxxx>
Sent: Tuesday, July 26, 2022 10:15 PM
On 7/26/2022 11:56 PM, Parav Pandit wrote:
From: Zhu, Lingshan <lingshan.zhu@xxxxxxxxx>
Sent: Tuesday, July 12, 2022 11:46 PM
When the user space which invokes netlink commands, detects that
_MQ
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.
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.
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.
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?
I guess this is spec thing (historical debt), I vaguely recall these fields
are always present in config space regardless the existence of corresponding
feature bit.
-Siwei
Nope:
2.5.1 Driver Requirements: Device Configuration Space
...
For optional configuration space fields, the driver MUST check that the corresponding feature is offered
before accessing that part of the configuration space.
Well, this is driver side of requirement. As this interface is for host
admin tool to query or configure vdpa device, we don't have to wait
until feature negotiation is done on guest driver to extract vdpa
attributes/parameters, say if we want to replicate another vdpa device
with the same config on migration destination. I think what may need to
be fix is to move off from using .vdpa_get_config_unlocked() which
depends on feature negotiation. And/or expose config space register
values through another set of attributes.
-Siwei
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
