Re: [PATCH v7] virtio-vsock: Add support for multi devices
Xuewei Niu <[email protected]>
| Newsgroups | dev.linux.lists.virtio-comment |
|---|---|
| Message-ID | <[email protected]> |
> On Mon, Jun 23, 2025 at 06:35:59PM +0800, Xuewei Niu wrote: > >> On Mon, Jun 23, 2025 at 04:48:33PM +0800, Xuewei Niu wrote: > >> >> On Thu, Jun 19, 2025 at 01:10:33PM +0800, Xuewei Niu wrote: > >> >> >> > From: Xuewei Niu <[email protected]> > >> >> >> > Sent: 19 June 2025 08:57 AM > >> >> >> > > >> >> >> > Hi Parav, > >> >> >> > > >> >> >> > Could you please take a look at the diagram in [1]? > >> >> >> > > >> >> >> > IIUC, for VM0, there are two groups, and for VM1, there is only one group. > >> >> >> > Am I right? If yes, I think the group concept is reasonable but we don't need > >> >> >> > at this time. > >> >> >> > > >> >> >> > I think the first thing is to figure out how to pick the right group. > >> >> >> > > >> >> >> > Standard socket doesn't provide a way to access the group information. > >> >> >> > > >> >> >> > Source and destination are from `bind()` and `connect()`, respectively. If we > >> >> >> > don't call `bind()`, only the destination is known. > >> >> >> > > >> >> >> > However, only destination is not enough to find the group. For example, the > >> >> >> > well-known CIDs (e.g. 2) are valid for all groups. > >> >> >> > > >> >> >> > 1: > >> >> >> > https://lore.kernel.org/virtio-comment/[email protected]/ > >> >> >> > >> >> >> Right. Sock addressing scheme is naïve presently to select the group. > >> >> >> Not sure when/how you or others plan to do. > >> >> >> This is transport layer problem to solve (not to confuse with transport = pci/mmio etc). > >> >> > >> >> > >> >> >> > >> >> >> However, at device level, we should have the construct of grouping. > >> >> >> Without this construct, all devices will be part of single group and one will not be able to build the group concept later. > >> >> >> So even if you don't need it explicitly now, grouping the device is what you need when connect() is called. > >> >> >> > >> >> >> So I was imagining a relatively simple scheme: > >> >> >> For example, virtio device level, some kind of group id is present. > >> >> >> So two devices which has same group id, are part of single group. > >> >> >> An example group id format can be a UUID. > >> >> >> > >> >> >> And this is completely optional for devices to implement. > >> >> >> Generic enough and usable beyond just vsock device in other use cases > >> >> >> we discussed in past. > >> >> > > >> >> >Fair enough. > >> >> > > >> >> >@Stefano, could you please take a look at this? I'd love to have some > >> >> >input > >> >> >from you. > >> >> > > >> >> >A brief summary of the idea is: The config space will be extended to > >> >> >include a group id. The devices with the same group id are considered > >> >> >to be > >> >> >in the same group. > >> >> > >> >> Thanks for the summary, but please avoid top posting, otherwise is very > >> >> hard to follow the discussion :-( > >> >> https://subspace.kernel.org/etiquette.html#do-not-top-post-when-replying > >> > > >> >Sorry, I'll avoid it in the future. > >> > > >> >> I like the idea of groups. What is not clear to me, is how groups will > >> >> allow the driver to select the default output device when the source > >> >> socket is not bind to any source CID. > >> > > >> >Well, we did discuss, but we need your input. > >> > > >> >I said in the thread [1] based on the standard socket API, the driver can't > >> >pick a group. Parav [2] suggested that the group, as a basic concept, > >> >should be present even if we are unable to use it. > >> > > >> >IMHO, we might use "{group_id}-{cid}" as the vsock addressing scheme to use > >> >the concept. But it is a very big change, leading to incompatibility with > >> >the existing apps. > >> > >> I still don't understand how the group_id will work :-( and how will > >> allow the driver to pick the default device. > > > >FYI, here is trying to explain what is group and why it is needed, but I > >agree with you about the idea of "types". > > > >I posted a diagram in the thread [1]. I think the group is something like a > >"namespace". I think it is reasonable, but my concerns are compatibility > >and complexity. > > > >1: https://lore.kernel.org/virtio-comment/[email protected]/ > > > >> This is the `sockaddr_vm`, so we should be careful of extending it: > >> > >> struct sockaddr_vm { > >> __kernel_sa_family_t svm_family; > >> unsigned short svm_reserved1; > >> unsigned int svm_port; > >> unsigned int svm_cid; > >> > >> #define VMADDR_FLAG_TO_HOST 0x01 > >> > >> __u8 svm_flags; > >> unsigned char svm_zero[sizeof(struct sockaddr) - > >> sizeof(sa_family_t) - > >> sizeof(unsigned short) - > >> sizeof(unsigned int) - > >> sizeof(unsigned int) - > >> sizeof(__u8)]; > >> }; > >> > >> >I think it might be beyond the scope of this patch, and would make the > >> >vsock more complex. The current conclusion is that we will keep the concept > >> >of grouping as a placeholder, but we will not use it. > >> > >> IMO we should first clarify better what we want to support. > >> As I already suggested some months ago, IMHO supporting any number of > >> vsock devices for a VM it's not really needed for your goal and I can't > >> see other use cases where a virtio-net device can't be use. Just a > >> reminder, vsock is not a network device, is more a P2P device where we > >> want to keep the configuration in the guest as simpler as possible (we > >> don't want to run ARP, DHCP, etc.). > > > >Agree that. > > > >> Till now vsock was more used just for guest-host communication, but > >> recently it was extended to communicate with sibling VMs. > >> > >> IIUC your use case, we just need to support different type of vsock > >> devices attached to the VM. With "type" I mean type of address handled. > >> I think we can define 3 types based on the CID we have: > >> - hypervisor: VMADDR_CID_HYPERVISOR(0) > >> - host: VMADDR_CID_HOST(2) > >> - sibling: CID >=3 > >> > >> The vhost-vsock device handles only VMADDR_CID_HOST, so it doesn't allow > >> to reach from the guest any other CIDs. > >> The vhost-user-vsock recently started to support sibling VMs. > >> The tcp-over-vsock of libkrun should use VMADDR_CID_HYPERVISOR(0). > >> > >> So, IMHO we should define new features or config flags that a device can > >> expose depending on which address is able to handle. > >> Also, in order to avoid to overcomplicate vsock, we should allow only > >> one device for each type (a single device should support multiple types, > >> but only one device can be registered for a type). > > > >I think only one device can be registered for sibling and hypervisor. And > >it is possible to have multiple devices for host. Otherwise, the goals of > >this patch will be not achieved. > > Why? > > IMO, as I already wrote, the libkrun service should use CID=0. As we before discussed, if there is only one backend for the libkrun service, we can use cid=0. I totally agree with you. So let us put this case aside first. > How you will handle multiple devices for host? How can the guest know > which device to use to reach HOST(2)? Do `bind()` explicitly, and a device with matching cid will be picked up. > IMO is easier to have the multiple device for sibling (e.g. the device > can advertise which dest CID its supports), but not for host. I think one device for sibling is enough. For example, dev1 is enough for communication with dev3 and dev4. So we don't need dev2 for sibling in the first VM, right? +──kernel(vhost-vsock)──────────────────────────────────────────────+ │+──────────────+ +──────────────+ +──────────────+ +──────────────+│ ││ dev1(cid1) │ │ dev2(cid2) │ │ dev3(cid3) │ │ dev4(cid4) ││ │+──────────────+ +──────────────+ +──────────────+ +──────────────+│ +────────▲────────────────▲────────────────▲────────────────▲───────+ │ │ │ │ │ │ ┌─────┘ │ │ │ │ │ +────────┴───────────+ │ +────────┴───────────+ +────────┴───────────+ │+──────────────+ VM0│ │ │+──────────────+ VM1│ │+──────────────+ VM2│ ││dev1(cid1,def)│ │ │ ││dev3(cid3,def)│ │ ││dev4(cid4,def)│ │ │+──────────────+ │ │ │+──────────────+ │ │+──────────────+ │ │+──────────────+ │ │ │ │ │ │ ││ dev2(cid2) │────┼────┘ │ │ │ │ │+──────────────+ │ │ │ │ │ +────────────────────+ +────────────────────+ +────────────────────+ The things go different for devices for host: 1. For dev1, it is used for sibling (dev1 <-> dev4), and the host (src_cid=cid1, dst_cid=2) means the real host (not a userapp), where doesn't show in the diagram. 2. For dev2 (src_cid=cid2), the host (dst_cid=2) is userapp1; 3. For dev3 (src_cid=cid3), the host (dst_cid=2) is userapp2. +──kernel(vhost-vsock)──────────────────────────────────────────────+ │+──────────────+ +──────────────+│ ││ dev1(cid1) │ │ dev4(cid4) ││ │+──────────────+ +──────────────+│ +────────▲──────────────────────────────────────────────────▲───────+ │ │ +────────┴───────────+ +────────────────────+ │ │+──────────────+ VM0│ │+──────────────+ │ │ ││dev1(cid1,def)│ │┌────▶│ dev2(cid2) │ │ +────────┴───────────+ │+──────────────+ ││ │+──────────────+ │ │+──────────────+ VM2│ │+──────────────+ ││ │ userapp1│ ││dev4(cid4,def)│ │ ││ dev2(cid2) │────┼┘ +────────────────────+ │+──────────────+ │ │+──────────────+ vhost-user-vsock─────────────+ │ │ │+──────────────+ │ │+──────────────+ │ │ │ ││ dev3(cid3) │────┼─────▶│ dev3(cid3) │ │ │ │ │+──────────────+ │ │+──────────────+ │ +────────────────────+ +────────────────────+ │ userapp2│ +────────────────────+ Thanks, Xuewei > > > >Then, we can get rid of the group concept. > > > >Big thanks for your constructive ideas :) > > You're welcome :-) > > Stefano