Re: [PATCH v1 2/2] virtio-ism: introduce new device virtio-ism

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

On Mon, 14 Nov 2022 15:25:28 +0100, Cornelia Huck <cohuck@redhat.com> wrote:
> On Tue, Nov 01 2022, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> > The virtio ism device provides and manages many memory ism regions in
> > host. These ism regions can be alloc/attach/detach by driver. Every
> > ism region can be shared by token with other VM after allocation.
> > The driver obtains the memory region on the host through the memory on
> > the device.
> >
> > |-------------------------------------------------------------------------------------------------------------|
> > | |------------------------------------------------|       |------------------------------------------------| |
> > | | Guest                                          |       | Guest                                          | |
> > | |                                                |       |                                                | |
> > | |   ----------------                             |       |   ----------------                             | |
> > | |   |    driver    |     [M1]   [M2]   [M3]      |       |   |    driver    |             [M2]   [M3]     | |
> > | |   ----------------       |      |      |       |       |   ----------------               |      |      | |
> > | |    |cq|                  |map   |map   |map    |       |    |cq|                          |map   |map   | |
> > | |    |  |                  |      |      |       |       |    |  |                          |      |      | |
> > | |    |  |                -------------------     |       |    |  |                --------------------    | |
> > | |----|--|----------------|  device memory  |-----|       |----|--|----------------|  device memory   |----| |
> > | |    |  |                -------------------     |       |    |  |                --------------------    | |
> > | |                                |               |       |                               |                | |
> > | |                                |               |       |                               |                | |
> > | | Qemu                           |               |       | Qemu                          |                | |
> > | |--------------------------------+---------------|       |-------------------------------+----------------| |
> > |                                  |                                                       |                  |
> > |                                  |                                                       |                  |
> > |                                  |------------------------------+------------------------|                  |
> > |                                                                 |                                           |
> > |                                                                 |                                           |
> > |                                                   --------------------------                                |
> > |                                                    | M1 |   | M2 |   | M3 |                                 |
> > |                                                   --------------------------                                |
> > |                                                                                                             |
> > | HOST                                                                                                        |
> > ---------------------------------------------------------------------------------------------------------------
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
> > Signed-off-by: Dust Li <dust.li@linux.alibaba.com>
> > Signed-off-by: Tony Lu <tonylu@linux.alibaba.com>
> > Signed-off-by: Helin Guo <helinguo@linux.alibaba.com>
> > Signed-off-by: Hans Zhang <hans@linux.alibaba.com>
> > Signed-off-by: He Rongguang <herongguang@linux.alibaba.com>
> > ---
> >  content.tex    |   1 +
> >  virtio-ism.tex | 350 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 351 insertions(+)
> >  create mode 100644 virtio-ism.tex
>
> <mostly formal things>

Thank you for your patience.

I will fix these problems in the next version.

Thanks.

>
> (...)
>
> > +\devicenormative{\subsubsection}{Device configuration layout}{Device Types / ISM Device / Device configuration layout}
> > +
> > +The device MUST ensure that the gid generated each time on the same host is the
> > +same and different from the gid on other host.
>
> Maybe "The device MUST ensure that the gid is immutable and unique for
> the (host/<better term>)." ?
>
> Is there a way to avoid the term "host" (throughout this document)?
> IIUC, you need the uniqueness within the scope of the entity that
> launches the different instances that get shared access to the regions
> (which could conceivably a unit of hardware?) TBF, I don't know what
> requirements are actually needed for the gid's uniqueness... probably
> not world-wide :)
>
> > +
> > +On the same host, the device MUST ensure that the devid generated each time is
> > +unique and not 0.
>
> "The device MUST ensure that the devid is unique per (host/<better
> term>) and not 0." ?
>
> > +
> > +
> > +\subsection{Event}\label{sec:Device Types / Network Device / Device Operation / Event}
> > +
> > +When VIRTIO_ISM_F_EVENT_VQ or VIRTIO_ISM_F_EVENT_IRQ is negotiated, the ism
> > +device supports event notification of ism region update. After the device
> > +receives the notification from the driver, it MUST notify other guests that
>
> This "MUST" statement needs to go into a conformance section.
>
> > +refer to this ism region.
> > +
> > +Such a structure will be received if VIRTIO_ISM_F_EVENT_VQ is negotiated.
> > +
> > +\begin{lstlisting}
> > +struct virtio_ism_event {
> > +	le64 num;
> > +	le64 offset[];
> > +};
> > +\end{lstlisting}
> > +
> > +\begin{description}
> > +\item[\field{num}] The number of ism regions with update events.
> > +\item[\field{offset}] The offset of ism regions with update events.
> > +\end{description}
> > +
> > +If VIRTIO_ISM_F_EVENT_IRQ is negotiated, when the driver receives an interrupt,
> > +it means that the ism region associated with it has been updated.
> > +
> > +
> > +\subsection{Permissions}\label{sec:Device Types / Network Device / Device Operation / Permission}
> > +
> > +The driver can set independent permissions for a certain ism region. Restrict
> > +which devices can execute attach or read and write permissions after attach.
> > +
> > +By default, the ism region can be attached by any device, and the driver can set
> > +it to not allow attachment or only allow the specified device to attach.
> > +
> > +The driver can set the read and write permissions after it is attached by
> > +default, and can also set independent read and write permissions for some
> > +devices.
> > +
> > +When a driver has the management permission of the ism region,
> > +then it can modify the permissions of this ism region.
> > +By default, only the device that created the ism region has this permission.
> > +
> > +
> > +\subsection{Device Initialization}\label{sec:Device Types / ISM Device / Device Initialization}
> > +
> > +\devicenormative{\subsubsection}{Device Initialization}{Device Types / ISM Device / Device Initialization}
> > +
> > +The device MUST regenerate a \field{devid}. \field{devid} remains unchanged
>
> Why does it need to "regenerate" it?
>
> > +during reset. \field{devid} MUST NOT be 0;
>
> s/;/./
>
> > +
> > +The device shares memory to the guest based on shared memory regions
>
> Can we avoid the "guest" terminology as well?
>
> > +\ref{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}.
> > +However, it does not need to allocate physical memory during initialization.
>
> (...)
>
> You also need to wire up the normative statements in conformance.tex.
>