Re: [virtio-comment] [PATCH 1/1] virtio-pmem: Support PCI BAR-relative addresses

[Cornelia Huck <cohuck@redhat.com>]

On Wed, Jul 21 2021, tstark@linux.microsoft.com wrote:

> From: Taylor Stark <tstark@microsoft.com>
>
> Update the virtio-pmem RFC spec to add support for describing the pmem region
> via PCI BARs. Shared memory windows are used to accomplish this, similar to
> virtio-fs and virtio-gpu. 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.

Given that we already have pmem support out there (even though the spec
has not been included yet, should this get a feature?

>
> Signed-off-by: Taylor Stark <tstark@microsoft.com>
> ---
>  virtio-pmem.tex | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/virtio-pmem.tex b/virtio-pmem.tex
> index 04e07bb..3f7d48e 100644
> --- a/virtio-pmem.tex
> +++ b/virtio-pmem.tex
> @@ -40,11 +40,21 @@ \subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device
>  Device hotplugs physical memory to guest address space. Persistent memory device
>  is emulated with file backed memory at host side.
>  
> +The device MUST indicate the guest physical address to the driver in one of two
> +ways:

This is not a normative section; better use "The device indicates...."?

>  \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}
>  
> +If the guest physical address is indicated as an absolute address, the device
> +MUST set \field{start} to the absolute address and \field{size} to the size of
> +the address range, in bytes.
> +
> +If the guest physical address is indicated as a shared memory region, the shared
> +memory region MUST be shared memory region ID 0. The device SHOULD set
> +\field{start} and \field{size} to zero.

And these two paragraphs should go into a normative section.

> +
>  \devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization}
>  
>  File backed memory SHOULD be memory mapped to guest address space with SHARED
> @@ -52,9 +62,11 @@ \subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device
>  
>  \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.
> +The driver SHOULD query the physical address ranges where the pmem was mapped.
> +When performing the query, the driver MUST first query if shared memory region
> +ID 0 is supported by the device. If present, the driver MUST NOT use \field{start}
> +or \field{size}. If not present, the driver SHOULD fallback to reading the
> +physical address ranges from \field{start} and \field{size}.

Reading this, I think we really need to have a feature to distinguish
between the two modes; otherwise, old drivers will be confused.

Also, requirements belong in a normative section; it's better to just
describe textually what the driver needs to do here and split out the
normative statements.

>  
>  \drivernormative{\subsubsection}{Driver Initialization: Filesystem direct access}{Device Types / PMEM Driver / Driver Initialization / Direct access}