RE: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout

quoted

From: Jason Wang <redacted>
Sent: Monday, March 1, 2021 9:29 AM

On 2021/2/26 4:50 下午, Parav Pandit wrote:

From: Jason Wang <redacted>
Sent: Friday, February 26, 2021 1:56 PM

On 2021/2/26 1:32 下午, Parav Pandit wrote:
From: Jason Wang <redacted>
Sent: Wednesday, February 24, 2021 2:18 PM

On 2021/2/24 2:18 下午, Parav Pandit wrote:
+
+	if (nla_put_u16(msg,
VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
+			config->max_virtqueue_pairs))
+		return -EMSGSIZE;
We probably need to check VIRTIO_NET_F_RSS here.
Yes. Will use it in v2.

+	if (nla_put_u8(msg,
VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
+		       config->rss_max_key_size))
+		return -EMSGSIZE;
+
+	rss_field = le16_to_cpu(config->rss_max_key_size);
+	if (nla_put_u16(msg,
VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,
rss_field))
+		return -EMSGSIZE;
+
+	hash_types = le32_to_cpu(config->supported_hash_types);
+	if (nla_put_u32(msg,
VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
+			config->supported_hash_types))
+		return -EMSGSIZE;
+	return 0;
+}
+
+static int vdpa_dev_net_config_fill(struct vdpa_device *vdev,
+struct sk_buff *msg) {
+	struct virtio_net_config config = {};
+
+	vdev->config->get_config(vdev, 0, &config, sizeof(config));
Do we need to sync with other possible get_config calls here?
To readers of get_config() is ok. No need to sync.
However, I think set_config() and get_config() should sync otherwise
get_config can get partial value.
Will probably have to add vdpa_device->config_access_lock().
Probably. And the set_config() should be synchronized with the
requrest that comes from vDPA bus.
Yes, a rw semaphore is good enough.
Device config fields can be change from the device side anyway, such as
link status anyway.
Sync using lock shouldn’t be problem.

This makes me think whether we should go back to two phases method,
create and enable.

The vDPA device is only registred after enabling, then there's no
need to sync with vDPA bus, and mgmt is not allowed to change config
after enalbing?
In that case we should be able to disable it as well. Disable should take the
device of the bus.
I find it weird to have this model to linger around the device without
anchoring on the bus.
For example device is not yet enabled, so it is not anchored on the bus, but
its name is still have to remain unique.

Can we do some synchornization between vdpa device allocation and vdpa
device registier to solve the naming issue?
mgmt tool querying the device config space after vdpa device is in use is real.
So I don't see solving it any differently.

Now upper layers of vdpa to not access the device on the placed on the vdpa bus, can be taken care by existing driver code using
echo 0 > /sys/bus/vdpa/drivers_autoprobe

By default vdpa device placed on the bus wont bind to any driver (net/vhost etc).
1. Decision to bind to which driver is done after config of the vdpa device is done.
2. orchestration sw does create and config before it binds to the right driver
3. which driver to bind to decided based on the use case of that individual vdpa device
For example, 
(a) vdpa0 binds to net driver to have Netdev for container
(b) vdpa1 binds to vhost driver to map vdpa device to VM

Do we have to anchor vdpa device on the bus? Can vdpa be a class?

I think not, it's a bus that is expected to be bound to different driver
instead of a subsystem.
Yeah, I realized it lately after writing the previous email.
Lets see if above scheme works or not to use existing drivers_autoprobe file.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help