Re: [PATCH v6 RESEND] virtio-vsock: Add support for multi devices

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Apr 10, 2025 at 06:47:27PM +0800, Xuewei Niu wrote:
> > On Thu, 10 Apr 2025 at 10:59, Xuewei Niu <niuxuewei97@gmail.com> wrote:
> > >
> > > > On Thu, 10 Apr 2025 at 05:06, Xuewei Niu <niuxuewei97@gmail.com> wrote:
> > > > >
> > > > > > On Wed, 9 Apr 2025 at 08:56, Xuewei Niu <niuxuewei97@gmail.com> wrote:
> > > > > > [...]
> > > > > > > >
> > > > > > > > The idea is to inform the guest which addresses are reachable by the
> > > > > > > > device, so the guest can easily decide which device to use. I'm
> > > > > > > > talking about the destination, so CID_HOST(2), CID_HYPERVISOS(0) or a
> > > > > > > > sibling VM (CID >=3).
> > > > > > > >
> > > > > > > > >
> > > > > > > > > We have had the guest_cid field in the config space. The guest knows all
> > > > > > > > > devices present in the VM.
> > > > > > > >
> > > > > > > > Okay, but how can the guest figure out from this information which
> > > > > > > > device to use to talk to the hypervisor or an application in the host?
> > > > > > > >
> > > > > > > > >
> > > > > > > > > If the app tries to bind a random CID, it will fail since the driver can't
> > > > > > > > > find the device by the CID.
> > > > > > > >
> > > > > > > > I'm not talking about the source CID on which to do bind() (which I
> > > > > > > > honestly don't like), but I'm talking about the destination CID on
> > > > > > > > which to do connect().
> > > > > > > >
> > > > > > > > > > so that the guest knows which device to use depending on the destination
> > > > > > > > > > CID.
> > > > > > > > >
> > > > > > > > > Yes, this is what I was describing in the previous comment. The message
> > > > > > > > > will be directed to the device by the destination CID.
> > > > > > > >
> > > > > > > > Sorry, I don't understand how you do this without having an
> > > > > > > > information from the device about what addresses it supports. Can you
> > > > > > > > elaborate a bit?
> > > > > > >
> > > > > > > Thanks for your explanation. So things you were talking about are as
> > > > > > > follows:
> > > > > > >
> > > > > > > 1) guest app as a server: the app MUST do `bind()` to a CID that is
> > > > > > > available in current VM.
> > > > > > > 2) guest app as a client: the guest driver picks a device and uses the
> > > > > > > device's CID as src CID, so that the guest app don't need to do `bind()`,
> > > > > > > but only do `connect()`.
> > > > > > >
> > > > > > > The key point is who takes responsibility for picking a device:
> > > > > > >
> > > > > > > 1) I prefer the guest app to do such thing: do `bind()` to pick one, then
> > > > > > > do `connect()`.
> > > > > >
> > > > > > Why?
> > > > > >
> > > > > > This implies that users must be informed in some way that they must
> > > > > > use a certain CID to communicate with the VMM and another to
> > > > > > communicate with the host application, when they could just as well
> > > > > > use CID_HYPERVISOS(0) or CID_HOST(2) for that and everything would be
> > > > > > transparent.
> > > > >
> > > > > Sorry, I didn't express it clearly. I just listed my original idea and your
> > > > > idea.
> > > > >
> > > > > What types of services can be considered as hypervisor services?
> > > >
> > > > TSI.
> > > > The vsock used to implement it end up in the VMM (i.e. libkrun), right?
> > > >
> > > > > In our
> > > > > case, the kata-agent is a control service for Kata, so could we consider it
> > > > > as a hypervisor service?
> > > >
> > > > Nope, kata-agent is clearly a host application.
> > >
> > > Make sense for me.
> > >
> > > > > How does the driver know which device is for hypervisor? A possible way is
> > > > > to add a feature, like `VIRTIO_VSOCK_F_HYPERVISOR`? If it is possible, I
> > > > > think the CID_HYPERVISOR can still be used in the case of 3+ devices to
> > > > > maintain the same behavior.
> > > >
> > > > Or adding a bitfield in the config space, where the device set several
> > > > flags depending on what kind of CIDs is able to handle.
> > > > They don't need to be negotiated, IMHO. Just advertised by the device.
> > >
> > > Will do in the v7.
> > 
> > BTW this could be part of another proposal if you don't want to speed this down.
> > I think we are talking about 2 features here: multiple devices and CID
> > supported per device.
> 
> IMO, it could be considered as one proposal but two features:
> CID_HYPERVISOR support and per device CID support. My idea is to split
> these two features into two patches.
> 
> WDYT?
> 
> Thanks,
> Xuewei

Split is always good.