Re: [virtio-dev] [PATCH v2 1/1] virtio-ism: introduce new device virtio-ism

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

On Wed, 11 Jan 2023 22:12:38 +0100, Halil Pasic <pasic@linux.ibm.com> wrote:
> On Wed, 11 Jan 2023 19:08:53 +0800
> Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> > > > +The device shares memory to the driver based on shared memory regions
> > > > +\ref{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}.
> > > > +However, it does not need to allocate physical memory during initialization.
> > > > +
> > > > +The \field{shmid} of a region MUST be one of the following
> > > > +\begin{lstlisting}
> > > > +enum virtio_ism_shm_id {
> > > > +	VIRTIO_ISM_SHM_ID_UNDEFINED = 0,
> > > > +	VIRTIO_ISM_SHM_ID_REGIONS   = 1,
> > > > +	VIRTIO_ISM_SHM_ID_NOTIFY    = 2,
> > > > +};
> > > > +\end{lstlisting}
> > > > +
> > > > +The shared memory whose shmid is VIRTIO_ISM_SHM_ID_REGIONS is used to implement
> > > > +ism regions.
> > >
> > > Hm. AFAIU, these are supposed to be used for exchanging data between the
> > > VMs that are supposed to communicate with each other via ism.
> > >
> > > How does this relate to the following normative section:
> > > """
> > > 2.10.2 Device Requirements: Shared Memory Regions
> > >
> > > Shared memory regions MUST NOT expose shared memory regions which are used to control the operation of the device, nor to stream data.
> > > """
> >
> > From https://lists.oasis-open.org/archives/virtio-comment/201906/msg00010.html
> >
> > 	I'm trying to find a way to say that people shouldn't side-step virtio
> > 	queues by putting everything in a big blob of shared memory.
> >
> > So I think this should not be conflict.
>
> AFAIU putting everything in a big blob of shared memory is what ISM
> does. I'm not saying that this is bad. But I'm for sure confused.
>
> In my opinion we should either clarify what is meant by that sentence
> and maybe also why, or get rid of it.
>
> @MST: What is your take on this?
>
> >
> > >
> > > > If there are multiple shared memories whose shmid is
> > > > +VIRTIO_ISM_SHM_ID_REGIONS, they are used as contiguous memory in the order of
> > > > +acquisition.
> > >
> > > Hm, I used to think that the shmid is an unique identifier for a shared
> > > memory region. How can multiple virtio shared memory regions have the
> > > same shmid?
> >
> > Shmid is used to define what this sharing memory is used to do, not its unique
> > identifier.
>
> As far as I can tell the shmid is supposed to uniquely identify a single
> shared memory region!
>
> " A device may have multiple shared memory regions associated with it.
> Each region has a shmid to identify it, the meaning of which is device-specific."
>
> @MST: Can you please help us to get clarity on this?
>
> BTW, this is why I pointed you to the corresponding MMIO interface:
> """
> SHMSel	             Shared memory id
> 0x0ac
> W
>
> Writing to this register selects the shared memory region 2.10 following
> operations on SHMLenLow, SHMLenHigh, SHMBaseLow and SHMBaseHigh apply
> to.
>
> SHMLenLow            Shared memory region 64 bit long length
> 0x0b0
> SHMLenHigh
> 0x0b4
> R
>
>
> These registers return the length of the shared memory region in bytes,
> as defined by the device for the region selected by the SHMSel register.
> The lower 32 bits of the length are read from SHMLenLow and the higher
> 32 bits from SHMLenHigh. Reading from a non-existent region (i.e. where
> the ID written to SHMSel is unused) results in a length of -1.
>
>
> SHMBaseLow         Shared memory region 64 bit long physical address
> 0x0b8
> SHMBaseHigh
> 0x0bc
> R
>
> The driver reads these registers to discover the base address of the
> region in physical address space. This address is chosen by the device
> (or other part of the VMM). The lower 32 bits of the address are read
> from SHMBaseLow with the higher 32 bits from SHMBaseHigh. Reading from a
> non-existent region (i.e. where the ID written to SHMSel is unused)
> results in a base address of 0xffffffffffffffff.
> """
>
> I.e. after you write the shmid into SHMSel you can obtain the base
> address and the length of that region, provided there is a region
> with that id.


Oh, I may really be wrong.

@MST: we need your help.

The good news is that if my understanding is wrong, then this will be
simpler.

Thanks.


>
> >
> > This is the point that device provides multiple shared memory areas.
> > These memory areas are used to provide ISM Region.
> >
>
> Thus I don't think this is possible without violating the currently
> standing rules on shared memory regions.
>
> > >
> > > Can you have a look at the MMIO interface for virtio shared memory
> > > regions.
> >
> > Is there any special attention?
> >
>
> Please see above.
>
> >
> > >
> > > How far what ISM tries to do consistent with virtio shared memory. I
> > > mean, AFAIU once the shared memory is advertised, the  shared memory is
> > > supposed to be there and accessible by the driver for both writes and
> > > reads.
> >
> > Before allocation, VMM did not allocate physical memory for it. Therefore, it
> > was illegal to read and write this memory.
>
> I'm not sure whether generating addressing exceptions on accesses to the
> advertised virtio shared memory regions is acceptable or not. Maybe
> Michael, Connie and the TC members more versed in shared memory regions
> can help us get clarity on this.
>
> I don't think it is per-se prohibited, but on the other hand, to me
> virtio shared memory regions read more like memory that is supposed to
> work , and less like an address space that may or may not be backed by
> memory. So if this is allowed, maybe adding a sentence to the part that
> describes virtio shared memory regions in the basic facilities portion of
> the spec would be beneficial.
>
> Regards,
> Halil