[PATCH v2 0/1] virtio-pmem: Support describing pmem as shared memory region

[PATCH v2 0/1] virtio-pmem: Support describing pmem as shared memory region

[tstark]

From: Taylor Stark <tstark@microsoft.com>

Changes from v1 [1]:
 - Added in a feature bit (VIRTIO_PMEM_F_SHMEM_REGION) for controlling how the
   device indicates the guest physical address ranges to the driver. This feature
   directly affects control flow of the driver, since it seemed weird to have
   the driver indicate support for shared memory regions, and then needing
   to include an enum (or similar) informing the driver how the device
   indicated guest physical address ranges. If devices want to indicate the
   ranges as guest absolute addresses, they can skip negotiating the feature.
 - The linux driver implementation has been updated and tested, but I'm holding
   off on posting the patches to get some feedback on the new approach.
 - Moved some changes to proper subsections (normative subsections).

[1] https://lists.oasis-open.org/archives/virtio-comment/202107/msg00111.html

---

This patch updates the virtio-pmem spec to add support for describing the pmem
region as a shared memory region. This is required to support virtio-pmem in
Hyper-V, since Hyper-V only allows PCI devices to operate on memory ranges
defined via BARs. When using the virtio PCI transport, shared memory regions
are described via PCI BARs.

As virtio-pmem hasn't been added to the virtio spec yet (see this issue [1]),
this patch is based off the RFC spec [2]. The linux driver implementation has
been posted at [3].

[1] https://github.com/oasis-tcs/virtio-spec/issues/78
[2] https://lists.oasis-open.org/archives/virtio-dev/201903/msg00083.html
[3] https://lore.kernel.org/nvdimm/20210715223505.GA29329@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net

Taylor Stark (1):
  virtio-pmem: Support describing pmem as shared memory region

 conformance.tex |  1 +
 virtio-pmem.tex | 30 +++++++++++++++++++++++++-----
 2 files changed, 26 insertions(+), 5 deletions(-)

-- 
2.32.0

[PATCH v2 1/1] virtio-pmem: Support describing pmem as shared memory region

[tstark]

From: Taylor Stark <tstark@microsoft.com>

Update the virtio-pmem RFC spec to add support for describing the pmem region
as a shared memory window. This is required to support virtio-pmem in Hyper-V,
since Hyper-V only allows PCI devices to operate on memory ranges defined via
BARs. When using the virtio PCI transport, shared memory regions are described
via PCI BARs.

Signed-off-by: Taylor Stark <tstark@microsoft.com>
---
 conformance.tex |  1 +
 virtio-pmem.tex | 30 +++++++++++++++++++++++++-----
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/conformance.tex b/conformance.tex
index 33fdac9..8e414eb 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -201,6 +201,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 
 \begin{itemize}
 \item \ref{devicenormative:Device Types / PMEM Device / Device Initialization}
+\item \ref{drivernormative:Device Types / PMEM Driver / Driver Initialization}
 \item \ref{drivernormative:Device Types / PMEM Driver / Driver Initialization / Direct access}
 \item \ref{drivernormative:Device Types / PMEM Driver / Driver Initialization / Virtio flush}
 \item \ref{drivernormative:Device Types / PMEM Driver / Driver Operation / Virtqueue command}
diff --git a/virtio-pmem.tex b/virtio-pmem.tex
index 04e07bb..9f25664 100644
--- a/virtio-pmem.tex
+++ b/virtio-pmem.tex
@@ -19,7 +19,10 @@ \subsection{Virtqueues}\label{sec:Device Types / PMEM Device / Virtqueues}
 
 \subsection{Feature bits}\label{sec:Device Types / PMEM Device / Feature bits}
 
-There are currently no feature bits defined for this device.
+\begin{description}
+\item[VIRTIO_PMEM_F_SHMEM_REGION (0)] The guest physical address range will be
+indicated as a shared memory region.
+\end{description}
 
 \subsection{Device configuration layout}\label{sec:Device Types / PMEM Device / Device configuration layout}
 
@@ -38,11 +41,11 @@ \subsection{Device configuration layout}\label{sec:Device Types / PMEM Device /
 \subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device Initialization}
 
 Device hotplugs physical memory to guest address space. Persistent memory device
-is emulated with file backed memory at host side.
-
+is emulated with file backed memory at host side. The device indicates the guest
+physical address to the driver in one of two ways:
 \begin{enumerate}
-\item Guest vpmem start is read from \field{start}.
-\item Guest vpmem end is read from \field{size}.
+\item As a guest absolute address.
+\item As a shared memory region.
 \end{enumerate}
 
 \devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization}
@@ -50,12 +53,29 @@ \subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device
 File backed memory SHOULD be memory mapped to guest address space with SHARED
 memory mapping.
 
+If VIRTIO_PMEM_F_SHMEM_REGION has been negotiated, the device MUST indicate the
+guest physical address as a shared memory region. The device MUST use shared
+memory region ID 0. The device SHOULD set \field{start} and \field{size} to zero.
+
+If VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated, the device MUST indicate
+the guest physical address as a guest absolute address. The device MUST set
+\field{start} to the absolute address and \field{size} to the size of the
+address range, in bytes.
+
 \subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization}
 
 Driver hotplugs the physical memory and registers associated
 region with the pmem API. Also, configures a flush callback
 function with the corresponding region.
 
+\drivernormative{\subsubsection}{Driver Initialization}{Device Types / PMEM Driver / Driver Initialization}
+
+The driver SHOULD query the physical address ranges where the pmem was mapped.
+If VIRTIO_PMEM_F_SHMEM_REGION has been negotiated, the driver SHOULD query
+shared memory ID 0 for the physical address ranges and MUST NOT use \field{start}
+or \field{size}. Else, the driver SHOULD read the physical address ranges from
+\field{start} and \field{size}.
+
 \drivernormative{\subsubsection}{Driver Initialization: Filesystem direct access}{Device Types / PMEM Driver / Driver Initialization / Direct access}
 
 Driver SHOULD enable filesystem direct access operations for
-- 
2.32.0

[virtio-comment] Re: [PATCH v2 1/1] virtio-pmem: Support describing pmem as shared memory region

[Cornelia Huck]

On Mon, Jul 26 2021, tstark@linux.microsoft.com wrote:

> @@ -50,12 +53,29 @@ \subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device
>  File backed memory SHOULD be memory mapped to guest address space with SHARED
>  memory mapping.
>  
> +If VIRTIO_PMEM_F_SHMEM_REGION has been negotiated, the device MUST indicate the
> +guest physical address as a shared memory region. The device MUST use shared
> +memory region ID 0. The device SHOULD set \field{start} and \field{size} to zero.
> +
> +If VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated, the device MUST indicate
> +the guest physical address as a guest absolute address. The device MUST set
> +\field{start} to the absolute address and \field{size} to the size of the
> +address range, in bytes.
> +
>  \subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization}
>  
>  Driver hotplugs the physical memory and registers associated
>  region with the pmem API. Also, configures a flush callback
>  function with the corresponding region.
>  
> +\drivernormative{\subsubsection}{Driver Initialization}{Device Types / PMEM Driver / Driver Initialization}
> +
> +The driver SHOULD query the physical address ranges where the pmem was mapped.

Isn't the driver quite useless if it doesn't discover where the pmem can
be found? I'm not sure we need that sentence.

> +If VIRTIO_PMEM_F_SHMEM_REGION has been negotiated, the driver SHOULD query
> +shared memory ID 0 for the physical address ranges and MUST NOT use \field{start}
> +or \field{size}. Else, the driver SHOULD read the physical address ranges from
> +\field{start} and \field{size}.

'SHOULD' is probably not the right way to specify this. What about the
following:

"In order to discover the physical address ranges where the pmem was
mapped:

If VIRTIO_PMEM_F_SHMEM_REGION has been negotiated, the driver MUST query
shared memory ID 0 for the physical address ranges, and MUST NOT use
\field{start} or \field{stop}.

If VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated, the driver MUST
read the physical address ranges from \field{start} and \field{stop}."

> +
>  \drivernormative{\subsubsection}{Driver Initialization: Filesystem direct access}{Device Types / PMEM Driver / Driver Initialization / Direct access}
>  
>  Driver SHOULD enable filesystem direct access operations for


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [PATCH v2 1/1] virtio-pmem: Support describing pmem as shared memory region

[Taylor Stark]

On Tue, Jul 27, 2021 at 05:06:43PM +0200, Cornelia Huck wrote:
> On Mon, Jul 26 2021, tstark@linux.microsoft.com wrote:
> 
> > @@ -50,12 +53,29 @@ \subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device
> >  File backed memory SHOULD be memory mapped to guest address space with SHARED
> >  memory mapping.
> >  
> > +If VIRTIO_PMEM_F_SHMEM_REGION has been negotiated, the device MUST indicate the
> > +guest physical address as a shared memory region. The device MUST use shared
> > +memory region ID 0. The device SHOULD set \field{start} and \field{size} to zero.
> > +
> > +If VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated, the device MUST indicate
> > +the guest physical address as a guest absolute address. The device MUST set
> > +\field{start} to the absolute address and \field{size} to the size of the
> > +address range, in bytes.
> > +
> >  \subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization}
> >  
> >  Driver hotplugs the physical memory and registers associated
> >  region with the pmem API. Also, configures a flush callback
> >  function with the corresponding region.
> >  
> > +\drivernormative{\subsubsection}{Driver Initialization}{Device Types / PMEM Driver / Driver Initialization}
> > +
> > +The driver SHOULD query the physical address ranges where the pmem was mapped.
> 
> Isn't the driver quite useless if it doesn't discover where the pmem can
> be found? I'm not sure we need that sentence.

It's about as useful as a paperweight if it doesn't discover where the pmem
was mapped :) Agreed, this isn't necessary.
 
> > +If VIRTIO_PMEM_F_SHMEM_REGION has been negotiated, the driver SHOULD query
> > +shared memory ID 0 for the physical address ranges and MUST NOT use \field{start}
> > +or \field{size}. Else, the driver SHOULD read the physical address ranges from
> > +\field{start} and \field{size}.
> 
> 'SHOULD' is probably not the right way to specify this. What about the
> following:
> 
> "In order to discover the physical address ranges where the pmem was
> mapped:
> 
> If VIRTIO_PMEM_F_SHMEM_REGION has been negotiated, the driver MUST query
> shared memory ID 0 for the physical address ranges, and MUST NOT use
> \field{start} or \field{stop}.
> 
> If VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated, the driver MUST
> read the physical address ranges from \field{start} and \field{stop}."

Sounds great to me. Will update in v3. Thanks again for the advice.

Thanks,
Taylor