[virtio-comment] [PATCH v2 0/3] Large shared memory regions

[virtio-comment] [PATCH v2 0/3] Large shared memory regions

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Hi,
  This series formalises the idea of shared memory regions I'd
previously discussed and is intended for use with virtio-fs.

I've split it into three parts:

  a) A general definition of the idea

  b) A definition on virtio-pci using the layout previously
discussed; this still uses the list of capabilities.

  c) A definition on virtio-mmio; this is untested
however I thought best to include it since it shows that
it isn't tied to PCI.


This v2 addresses (hopefully all) comments from the initial posting,
except for CCW and the GPU/dynamic usage currently being
discussed by Frank.  Cornelia has a suggested approach for CCW
that is being discussed, so it looks like we're getting there on
that.  The GPU usage being discussed by Frank is diverging to a
very different usage.

Dave

Dr. David Alan Gilbert (3):
  shared memory: Define shared memory regions
  shared memory: Define PCI capability
  shared memory: Define mmio registers

 content.tex    | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
 shared-mem.tex | 33 +++++++++++++++++++++++
 2 files changed, 104 insertions(+)
 create mode 100644 shared-mem.tex

-- 
2.20.1


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/

[virtio-comment] [PATCH v2 1/3] shared memory: Define shared memory regions

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Define the requirements and idea behind shared memory regions.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 content.tex    |  2 ++
 shared-mem.tex | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 shared-mem.tex

diff --git a/content.tex b/content.tex
index 836ee52..3dd504c 100644
--- a/content.tex
+++ b/content.tex
@@ -371,6 +371,8 @@ making any more buffers available. When VIRTIO_F_NOTIFICATION_DATA
 has been negotiated, these notifications would then have
 identical \field{next_off} and \field{next_wrap} values.
 
+\input{shared-mem.tex}
+
 \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
 
 We start with an overview of device initialization, then expand on the
diff --git a/shared-mem.tex b/shared-mem.tex
new file mode 100644
index 0000000..85b0c55
--- /dev/null
+++ b/shared-mem.tex
@@ -0,0 +1,33 @@
+\section{Shared Memory Regions}\label{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}
+
+Shared memory regions are an additional facility
+available to devices that need a region of memory that's
+continuously shared between the host and the guest, rather
+than passed between them in the way virtqueue elements are.
+
+Example uses include shared caches and version pools for versioned
+data structures.
+
+The region is chosen by the host and presented to the guest, as
+such it is useful in situations where the memory is accessed on
+the host by other libraries that can't safely access guest RAM.
+
+Shared memory regions MUST NOT be used to control the operation
+of the device, nor to stream data; those should still be performed
+using virtqueues.
+
+A device may have multiple shared memory regions associated with
+it.  Each region has a \field{shmid} to identify it, the meaning
+of which is device-specific.
+
+Enumeration and location of shared memory regions is performed
+using a transport-specific data structure and mechanism.
+
+Memory consistency rules vary depending on the region and the
+device.  Devices MUST define the required behaviour for each
+region.
+
+The guest physical address and the host virtual address MUST NOT
+be used to identify structures within the memory regions; all
+addressing MUST be relative to the start of a particular region.
+
-- 
2.20.1


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/

[virtio-comment] [PATCH v2 2/3] shared memory: Define PCI capability

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Define the PCI capability used for enumerating shared memory regions.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 content.tex | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/content.tex b/content.tex
index 3dd504c..9838208 100644
--- a/content.tex
+++ b/content.tex
@@ -688,6 +688,8 @@ The fields are interpreted as follows:
 #define VIRTIO_PCI_CAP_DEVICE_CFG        4
 /* PCI configuration access */
 #define VIRTIO_PCI_CAP_PCI_CFG           5
+/* Shared memory region */
+#define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
 \end{lstlisting}
 
         Any other value is reserved for future use.
@@ -1052,6 +1054,41 @@ any device type which has a device-specific configuration.
 
 The \field{offset} for the device-specific configuration MUST be 4-byte aligned.
 
+\subsubsection{Shared memory capability}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Shared memory capability}
+
+Shared memory regions \ref{sec:Basic Facilities of a Virtio
+Device / Shared Memory Regions} are enumerated on the PCI transport
+as a sequence of VIRTIO_PCI_CAP_SHARED_MEMORY_CFG capabilities, one per region.
+
+The capability is immediately followed by an additional field like so:
+
+\begin{lstlisting}
+struct virtio_pci_shm_cap {
+        struct virtio_pci_cap cap;
+        u32 offset_hi;
+        u32 length_hi;
+        u8  id;
+};
+\end{lstlisting}
+
+Given that the \field{cap.length} and \field{cap.offset} fields
+are only 32 bit, the additional \field{offset_hi} and \field {length_hi}
+fields provide the most significant 32 bits of a total 64 bit offset and
+length within the bar specified by \field{cap.bar}.  \field{id}
+uniquely identifies the given shared memory region using a device
+specific identifier.
+
+While most capabilities can not be repeated, multiple
+VIRTIO_PCI_CAP_SHARED_MEMORY_CFG capabilities are allowed as long
+as the \field{id} is unique.
+
+\devicenormative{\paragraph}{Device-specific configuration}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Shared memory capability}
+
+The region defined by the combination of the offsets and length
+fields MUST be contained within the declared bar.
+
+The \field{id} MUST be unique for any one device instance.
+
 \subsubsection{PCI configuration access capability}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
 
 The VIRTIO_PCI_CAP_PCI_CFG capability
-- 
2.20.1


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/

[virtio-comment] [PATCH v2 3/3] shared memory: Define mmio registers

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Define an MMIO interface to enumerate and map shared
memory regions.

Note: This hasn't been implemented.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 content.tex | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/content.tex b/content.tex
index 9838208..c3070bd 100644
--- a/content.tex
+++ b/content.tex
@@ -1726,6 +1726,28 @@ All register values are organized as Little Endian.
     selected by writing to \field{QueueSel}.
   }
   \hline 
+  \mmioreg{SHMId}{Shared memory id}{0x0ac}{W}{%
+    Writing to this register selects the shared memory region \ref{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}
+    following operations on \field{SHMLenLow}, \field{SHMLenHigh},
+    \field{SHMBaseLow} and \field{SHMBaseHigh} apply to.
+  }
+  \hline 
+  \mmiodreg{SHMLenLow}{SHMLenHigh}{Shared memory region 64 bit long length}{0x0b0}{0x0xb4}{R}{%
+    These registers return the length of the shared memory
+    region, as defined by the device for the region selected by
+    the \field{SHMId} register.  Reading from a non-existent
+    region (i.e. where the ID written to \field{SHMId} is unused)
+    results in a length of -1.
+  }
+  \hline 
+  \mmiodreg{SHMBaseLow}{SHMBaseHigh}{Shared memory region 64 bit long physical address}{0x0b8}{0x0xbc}{W}{%
+    The driver writes these registers to indicate where it wishes
+    the device to map the shared memory region currently
+    selected.  The driver shall write \field{SHMBaseLow} first
+    and then \field{SHMBaseHigh}.  To avoid multiple remappings, the device shall not change
+    the mapping until it sees the write to \field{SHMBaseHigh}.
+  }
+  \hline 
   \mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{
     Reading from this register returns a value describing a version of the device-specific configuration space (see \field{Config}).
     The driver can then access the configuration space and, when finished, read \field{ConfigGeneration} again.
@@ -1761,6 +1783,9 @@ driver seeing an inconsistent configuration state.
 
 The device MUST NOT access virtual queue contents when \field{QueueReady} is zero (0x0).
 
+The device MUST NOT respond to SHM Base writes for none-existent
+shared memory regions.
+
 \drivernormative{\subsubsection}{MMIO Device Register Layout}{Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}
 The driver MUST NOT access memory locations not described in the
 table \ref{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}
@@ -1815,6 +1840,13 @@ If both values are valid, it MUST read \field{DeviceID}
 and if its value is zero (0x0) MUST abort initialization and
 MUST NOT access any other register.
 
+Devices expecting shared memory regions MUST enumerate the
+regions by selecting each region in tern and checking that the
+length is not -1.
+
+Devices not expecting shared memory MUST NOT use the shared
+memory registers.
+
 Further initialization MUST follow the procedure described in
 \ref{sec:General Initialization And Device Operation / Device Initialization}~\nameref{sec:General Initialization And Device Operation / Device Initialization}.
 
-- 
2.20.1


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/

[virtio-comment] Re: [PATCH v2 1/3] shared memory: Define shared memory regions

[Cornelia Huck]

On Fri, 22 Feb 2019 11:54:31 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Define the requirements and idea behind shared memory regions.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  content.tex    |  2 ++
>  shared-mem.tex | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
>  create mode 100644 shared-mem.tex
> 
> diff --git a/content.tex b/content.tex
> index 836ee52..3dd504c 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -371,6 +371,8 @@ making any more buffers available. When VIRTIO_F_NOTIFICATION_DATA
>  has been negotiated, these notifications would then have
>  identical \field{next_off} and \field{next_wrap} values.
>  
> +\input{shared-mem.tex}
> +
>  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
>  
>  We start with an overview of device initialization, then expand on the
> diff --git a/shared-mem.tex b/shared-mem.tex
> new file mode 100644
> index 0000000..85b0c55
> --- /dev/null
> +++ b/shared-mem.tex
> @@ -0,0 +1,33 @@
> +\section{Shared Memory Regions}\label{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}
> +
> +Shared memory regions are an additional facility
> +available to devices that need a region of memory that's
> +continuously shared between the host and the guest, rather
> +than passed between them in the way virtqueue elements are.
> +
> +Example uses include shared caches and version pools for versioned
> +data structures.
> +
> +The region is chosen by the host and presented to the guest, as
> +such it is useful in situations where the memory is accessed on
> +the host by other libraries that can't safely access guest RAM.

This explanation looks good to me.

> +
> +Shared memory regions MUST NOT be used to control the operation
> +of the device, nor to stream data; those should still be performed
> +using virtqueues.

The 'MUST NOT' makes it look like a normative statement; however, this
is more like design advice?

> +
> +A device may have multiple shared memory regions associated with
> +it.  Each region has a \field{shmid} to identify it, the meaning
> +of which is device-specific.
> +
> +Enumeration and location of shared memory regions is performed
> +using a transport-specific data structure and mechanism.
> +
> +Memory consistency rules vary depending on the region and the
> +device.  Devices MUST define the required behaviour for each
> +region.

Same here.

> +
> +The guest physical address and the host virtual address MUST NOT
> +be used to identify structures within the memory regions; all
> +addressing MUST be relative to the start of a particular region.
> +

For that high-level overview, I'm not sure if any normative statements
are needed/wanted, or whether those should be confined to the individual
transport or device type definitions...

The text on its own looks good to me.

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/

[virtio-comment] Re: [PATCH v2 2/3] shared memory: Define PCI capability

[Cornelia Huck]

On Fri, 22 Feb 2019 11:54:32 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Define the PCI capability used for enumerating shared memory regions.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  content.tex | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

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/

[virtio-comment] Re: [PATCH v2 1/3] shared memory: Define shared memory regions

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2604 bytes --]

On Fri, Feb 22, 2019 at 01:48:39PM +0100, Cornelia Huck wrote:
> On Fri, 22 Feb 2019 11:54:31 +0000
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Define the requirements and idea behind shared memory regions.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  content.tex    |  2 ++
> >  shared-mem.tex | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> >  create mode 100644 shared-mem.tex
> > 
> > diff --git a/content.tex b/content.tex
> > index 836ee52..3dd504c 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -371,6 +371,8 @@ making any more buffers available. When VIRTIO_F_NOTIFICATION_DATA
> >  has been negotiated, these notifications would then have
> >  identical \field{next_off} and \field{next_wrap} values.
> >  
> > +\input{shared-mem.tex}
> > +
> >  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> >  
> >  We start with an overview of device initialization, then expand on the
> > diff --git a/shared-mem.tex b/shared-mem.tex
> > new file mode 100644
> > index 0000000..85b0c55
> > --- /dev/null
> > +++ b/shared-mem.tex
> > @@ -0,0 +1,33 @@
> > +\section{Shared Memory Regions}\label{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}
> > +
> > +Shared memory regions are an additional facility
> > +available to devices that need a region of memory that's
> > +continuously shared between the host and the guest, rather
> > +than passed between them in the way virtqueue elements are.
> > +
> > +Example uses include shared caches and version pools for versioned
> > +data structures.
> > +
> > +The region is chosen by the host and presented to the guest, as
> > +such it is useful in situations where the memory is accessed on
> > +the host by other libraries that can't safely access guest RAM.
> 
> This explanation looks good to me.
> 
> > +
> > +Shared memory regions MUST NOT be used to control the operation
> > +of the device, nor to stream data; those should still be performed
> > +using virtqueues.
> 
> The 'MUST NOT' makes it look like a normative statement; however, this
> is more like design advice?

It also reads strangely because "MUST NOT" is used but then the next
clause uses "should".

Regarding normative statements, in the VIRTIO spec they are made in
separate device or driver normative sections.  I'm not sure if using
them in a non-normative section is okay.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[virtio-comment] Re: [PATCH v2 3/3] shared memory: Define mmio registers

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

On Fri, Feb 22, 2019 at 11:54:33AM +0000, Dr. David Alan Gilbert (git) wrote:
> diff --git a/content.tex b/content.tex
> index 9838208..c3070bd 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -1726,6 +1726,28 @@ All register values are organized as Little Endian.
>      selected by writing to \field{QueueSel}.
>    }
>    \hline 
> +  \mmioreg{SHMId}{Shared memory id}{0x0ac}{W}{%
> +    Writing to this register selects the shared memory region \ref{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}
> +    following operations on \field{SHMLenLow}, \field{SHMLenHigh},
> +    \field{SHMBaseLow} and \field{SHMBaseHigh} apply to.
> +  }
> +  \hline 
> +  \mmiodreg{SHMLenLow}{SHMLenHigh}{Shared memory region 64 bit long length}{0x0b0}{0x0xb4}{R}{%

QueueDescLow/High explicitly documents the low and high 32 bits
relationship of these two registers.  Please do the same of
SHMLenLow/High and SHMBaseLow/High for clarity and consistency.  That
way readers know the purpose of Low and High.

> +    These registers return the length of the shared memory

Adding "in bytes" makes the units explicit.

> +    region, as defined by the device for the region selected by
> +    the \field{SHMId} register.  Reading from a non-existent
> +    region (i.e. where the ID written to \field{SHMId} is unused)
> +    results in a length of -1.

-1 is used to indicate the absence of a region, does 0 have a meaning?

> @@ -1761,6 +1783,9 @@ driver seeing an inconsistent configuration state.
>  
>  The device MUST NOT access virtual queue contents when \field{QueueReady} is zero (0x0).
>  
> +The device MUST NOT respond to SHM Base writes for none-existent

s/none/non/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [virtio-comment] Re: [PATCH v2 3/3] shared memory: Define mmio registers

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Fri, Feb 22, 2019 at 11:54:33AM +0000, Dr. David Alan Gilbert (git) wrote:
> > diff --git a/content.tex b/content.tex
> > index 9838208..c3070bd 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -1726,6 +1726,28 @@ All register values are organized as Little Endian.
> >      selected by writing to \field{QueueSel}.
> >    }
> >    \hline 
> > +  \mmioreg{SHMId}{Shared memory id}{0x0ac}{W}{%
> > +    Writing to this register selects the shared memory region \ref{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}
> > +    following operations on \field{SHMLenLow}, \field{SHMLenHigh},
> > +    \field{SHMBaseLow} and \field{SHMBaseHigh} apply to.
> > +  }
> > +  \hline 
> > +  \mmiodreg{SHMLenLow}{SHMLenHigh}{Shared memory region 64 bit long length}{0x0b0}{0x0xb4}{R}{%
> 
> QueueDescLow/High explicitly documents the low and high 32 bits
> relationship of these two registers.  Please do the same of
> SHMLenLow/High and SHMBaseLow/High for clarity and consistency.  That
> way readers know the purpose of Low and High.

Done (and to SHMBase)

> 
> > +    These registers return the length of the shared memory
> 
> Adding "in bytes" makes the units explicit.

Done

> > +    region, as defined by the device for the region selected by
> > +    the \field{SHMId} register.  Reading from a non-existent
> > +    region (i.e. where the ID written to \field{SHMId} is unused)
> > +    results in a length of -1.
> 
> -1 is used to indicate the absence of a region, does 0 have a meaning?

No, I'd be happy to switch; although this does lead to another question;
what happens on an older virtio-mmio implementation when a device
tries to read these registers to detect if the region exists?

> > @@ -1761,6 +1783,9 @@ driver seeing an inconsistent configuration state.
> >  
> >  The device MUST NOT access virtual queue contents when \field{QueueReady} is zero (0x0).
> >  
> > +The device MUST NOT respond to SHM Base writes for none-existent
> 
> s/none/non/

Done

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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/

[virtio-comment] Re: [virtio-dev] Re: [PATCH v2 1/3] shared memory: Define shared memory regions

[Dr. David Alan Gilbert]

* Cornelia Huck (cohuck@redhat.com) wrote:
> On Fri, 22 Feb 2019 11:54:31 +0000
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Define the requirements and idea behind shared memory regions.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  content.tex    |  2 ++
> >  shared-mem.tex | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> >  create mode 100644 shared-mem.tex
> > 
> > diff --git a/content.tex b/content.tex
> > index 836ee52..3dd504c 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -371,6 +371,8 @@ making any more buffers available. When VIRTIO_F_NOTIFICATION_DATA
> >  has been negotiated, these notifications would then have
> >  identical \field{next_off} and \field{next_wrap} values.
> >  
> > +\input{shared-mem.tex}
> > +
> >  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> >  
> >  We start with an overview of device initialization, then expand on the
> > diff --git a/shared-mem.tex b/shared-mem.tex
> > new file mode 100644
> > index 0000000..85b0c55
> > --- /dev/null
> > +++ b/shared-mem.tex
> > @@ -0,0 +1,33 @@
> > +\section{Shared Memory Regions}\label{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}
> > +
> > +Shared memory regions are an additional facility
> > +available to devices that need a region of memory that's
> > +continuously shared between the host and the guest, rather
> > +than passed between them in the way virtqueue elements are.
> > +
> > +Example uses include shared caches and version pools for versioned
> > +data structures.
> > +
> > +The region is chosen by the host and presented to the guest, as
> > +such it is useful in situations where the memory is accessed on
> > +the host by other libraries that can't safely access guest RAM.
> 
> This explanation looks good to me.
> 
> > +
> > +Shared memory regions MUST NOT be used to control the operation
> > +of the device, nor to stream data; those should still be performed
> > +using virtqueues.
> 
> The 'MUST NOT' makes it look like a normative statement; however, this
> is more like design advice?

Hmm I'm not sure - the intention of this line is to try and stop people
using it as a hack to avoid standardising protocol for no good reason.

> > +
> > +A device may have multiple shared memory regions associated with
> > +it.  Each region has a \field{shmid} to identify it, the meaning
> > +of which is device-specific.
> > +
> > +Enumeration and location of shared memory regions is performed
> > +using a transport-specific data structure and mechanism.
> > +
> > +Memory consistency rules vary depending on the region and the
> > +device.  Devices MUST define the required behaviour for each
> > +region.
> 
> Same here.

OK, so should I reword this?

> > +
> > +The guest physical address and the host virtual address MUST NOT
> > +be used to identify structures within the memory regions; all
> > +addressing MUST be relative to the start of a particular region.
> > +
> 
> For that high-level overview, I'm not sure if any normative statements
> are needed/wanted, or whether those should be confined to the individual
> transport or device type definitions...

I think this one is stronger than the previous two; if people start
passing GPA/HVA in the underlying structures things are going to get
messy.

Dave

> The text on its own looks good to me.
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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: [virtio-comment] Re: [virtio-dev] Re: [PATCH v2 1/3] shared memory: Define shared memory regions

[Halil Pasic]

On Tue, 26 Feb 2019 15:11:34 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Cornelia Huck (cohuck@redhat.com) wrote:
> > On Fri, 22 Feb 2019 11:54:31 +0000
> > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > 
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > Define the requirements and idea behind shared memory regions.
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > >  content.tex    |  2 ++
> > >  shared-mem.tex | 33 +++++++++++++++++++++++++++++++++
> > >  2 files changed, 35 insertions(+)
> > >  create mode 100644 shared-mem.tex
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index 836ee52..3dd504c 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -371,6 +371,8 @@ making any more buffers available. When VIRTIO_F_NOTIFICATION_DATA
> > >  has been negotiated, these notifications would then have
> > >  identical \field{next_off} and \field{next_wrap} values.
> > >  
> > > +\input{shared-mem.tex}
> > > +
> > >  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> > >  
> > >  We start with an overview of device initialization, then expand on the
> > > diff --git a/shared-mem.tex b/shared-mem.tex
> > > new file mode 100644
> > > index 0000000..85b0c55
> > > --- /dev/null
> > > +++ b/shared-mem.tex
> > > @@ -0,0 +1,33 @@
> > > +\section{Shared Memory Regions}\label{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}
> > > +
> > > +Shared memory regions are an additional facility
> > > +available to devices that need a region of memory that's
> > > +continuously shared between the host and the guest, rather
> > > +than passed between them in the way virtqueue elements are.
> > > +
> > > +Example uses include shared caches and version pools for versioned
> > > +data structures.
> > > +
> > > +The region is chosen by the host and presented to the guest, as
> > > +such it is useful in situations where the memory is accessed on
> > > +the host by other libraries that can't safely access guest RAM.
> > 
> > This explanation looks good to me.
> > 
> > > +
> > > +Shared memory regions MUST NOT be used to control the operation
> > > +of the device, nor to stream data; those should still be performed
> > > +using virtqueues.
> > 
> > The 'MUST NOT' makes it look like a normative statement; however, this
> > is more like design advice?
> 
> Hmm I'm not sure - the intention of this line is to try and stop people
> using it as a hack to avoid standardising protocol for no good reason.
> 

I had the same thoughts as Stefan but he was faster. I think his concern
regarding the formalism -- mine is for sure.

IMHO you making it a normative statement is a good idea. 

> > > +
> > > +A device may have multiple shared memory regions associated with
> > > +it.  Each region has a \field{shmid} to identify it, the meaning
> > > +of which is device-specific.
> > > +
> > > +Enumeration and location of shared memory regions is performed
> > > +using a transport-specific data structure and mechanism.
> > > +
> > > +Memory consistency rules vary depending on the region and the
> > > +device.  Devices MUST define the required behaviour for each
> > > +region.
> > 
> > Same here.
> 
> OK, so should I reword this?
> 

Same here. 

> > > +
> > > +The guest physical address and the host virtual address MUST NOT
> > > +be used to identify structures within the memory regions; all
> > > +addressing MUST be relative to the start of a particular region.
> > > +
> > 
> > For that high-level overview, I'm not sure if any normative statements
> > are needed/wanted, or whether those should be confined to the individual
> > transport or device type definitions...
> 
> I think this one is stronger than the previous two; if people start
> passing GPA/HVA in the underlying structures things are going to get
> messy.
>

And here. If it is supposed to be normative (and I think it is) it
should be made formally look like a normative statement. How to do it
exactly, no idea :(.

Regards,
Halil

 
> Dave
> 
> > The text on its own looks good to me.
> > 
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 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/
> 


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/

[virtio-comment] Re: [virtio-dev] Re: [PATCH v2 1/3] shared memory: Define shared memory regions

[Cornelia Huck]

On Tue, 26 Feb 2019 15:11:34 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Cornelia Huck (cohuck@redhat.com) wrote:
> > On Fri, 22 Feb 2019 11:54:31 +0000
> > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> >   
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > Define the requirements and idea behind shared memory regions.
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > >  content.tex    |  2 ++
> > >  shared-mem.tex | 33 +++++++++++++++++++++++++++++++++
> > >  2 files changed, 35 insertions(+)
> > >  create mode 100644 shared-mem.tex
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index 836ee52..3dd504c 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -371,6 +371,8 @@ making any more buffers available. When VIRTIO_F_NOTIFICATION_DATA
> > >  has been negotiated, these notifications would then have
> > >  identical \field{next_off} and \field{next_wrap} values.
> > >  
> > > +\input{shared-mem.tex}
> > > +
> > >  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> > >  
> > >  We start with an overview of device initialization, then expand on the
> > > diff --git a/shared-mem.tex b/shared-mem.tex
> > > new file mode 100644
> > > index 0000000..85b0c55
> > > --- /dev/null
> > > +++ b/shared-mem.tex
> > > @@ -0,0 +1,33 @@
> > > +\section{Shared Memory Regions}\label{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}
> > > +
> > > +Shared memory regions are an additional facility
> > > +available to devices that need a region of memory that's
> > > +continuously shared between the host and the guest, rather
> > > +than passed between them in the way virtqueue elements are.
> > > +
> > > +Example uses include shared caches and version pools for versioned
> > > +data structures.
> > > +
> > > +The region is chosen by the host and presented to the guest, as
> > > +such it is useful in situations where the memory is accessed on
> > > +the host by other libraries that can't safely access guest RAM.  
> > 
> > This explanation looks good to me.
> >   
> > > +
> > > +Shared memory regions MUST NOT be used to control the operation
> > > +of the device, nor to stream data; those should still be performed
> > > +using virtqueues.  
> > 
> > The 'MUST NOT' makes it look like a normative statement; however, this
> > is more like design advice?  
> 
> Hmm I'm not sure - the intention of this line is to try and stop people
> using it as a hack to avoid standardising protocol for no good reason.

I've looked through the spec again and it seems the best place for this
would indeed be a device-normative section for shared regions; it would
imply that a device needs to comply with this statement, or it is not
conformant.

It would need some tweaking, however. Perhaps something like the
following:

"The device MUST NOT expose shared memory regions which are used to
control the operation of the device, or to stream data."

Not sure where to put the pointer to use virtqueues for that.

> 
> > > +
> > > +A device may have multiple shared memory regions associated with
> > > +it.  Each region has a \field{shmid} to identify it, the meaning
> > > +of which is device-specific.
> > > +
> > > +Enumeration and location of shared memory regions is performed
> > > +using a transport-specific data structure and mechanism.
> > > +
> > > +Memory consistency rules vary depending on the region and the
> > > +device.  Devices MUST define the required behaviour for each
> > > +region.  
> > 
> > Same here.  
> 
> OK, so should I reword this?

Maybe put into the device normative section:

"A device MUST define the required behaviour for each region."

But I'm not sure whether this is a good normative statement... it's
more that the regions have specific requirements, which both the device
and the driver need to fulfill, and those requirements obviously need
to be put down somewhere.

> 
> > > +
> > > +The guest physical address and the host virtual address MUST NOT
> > > +be used to identify structures within the memory regions; all
> > > +addressing MUST be relative to the start of a particular region.
> > > +  
> > 
> > For that high-level overview, I'm not sure if any normative statements
> > are needed/wanted, or whether those should be confined to the individual
> > transport or device type definitions...  
> 
> I think this one is stronger than the previous two; if people start
> passing GPA/HVA in the underlying structures things are going to get
> messy.

What about

"The device MUST handle any access to the memory region as addressed
relatively to the beginning of the region, regardless whether it
accesses the region itself or the driver does so."

"The driver MUST address any particular region relatively to the start
of that region."

for device and driver normative statements, respectively. Better
wordings welcome.

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/

[virtio-comment] Re: [PATCH v2 3/3] shared memory: Define mmio registers

[Cornelia Huck]

On Fri, 22 Feb 2019 11:54:33 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Define an MMIO interface to enumerate and map shared
> memory regions.
> 
> Note: This hasn't been implemented.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  content.tex | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)

(...)

> @@ -1815,6 +1840,13 @@ If both values are valid, it MUST read \field{DeviceID}
>  and if its value is zero (0x0) MUST abort initialization and
>  MUST NOT access any other register.
>  
> +Devices expecting shared memory regions MUST enumerate the
> +regions by selecting each region in tern and checking that the

s/tern/turn/

> +length is not -1.
> +
> +Devices not expecting shared memory MUST NOT use the shared
> +memory registers.

Isn't that the _driver_ requirements section? These sentences also make
more sense if they talk about a driver expecting something.

What I also find a bit confusing is the requirement that they enumerate
all regions (at least, that's how I read it.)

Say we have a device type with shared regions foo, bar, and the
optional baz. It should be fine if a driver finds foo, bar, and baz; as
well as if a driver only finds foo and bar. Conversely, a driver only
looking for foo and bar should work with a device exposing foo, bar,
and baz.

> +
>  Further initialization MUST follow the procedure described in
>  \ref{sec:General Initialization And Device Operation / Device Initialization}~\nameref{sec:General Initialization And Device Operation / Device Initialization}.
>  


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: [virtio-comment] Re: [PATCH v2 3/3] shared memory: Define mmio registers

[Cornelia Huck]

On Mon, 25 Feb 2019 18:45:06 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > On Fri, Feb 22, 2019 at 11:54:33AM +0000, Dr. David Alan Gilbert (git) wrote:  

> > > +    region, as defined by the device for the region selected by
> > > +    the \field{SHMId} register.  Reading from a non-existent
> > > +    region (i.e. where the ID written to \field{SHMId} is unused)
> > > +    results in a length of -1.  
> > 
> > -1 is used to indicate the absence of a region, does 0 have a meaning?  
> 
> No, I'd be happy to switch; although this does lead to another question;
> what happens on an older virtio-mmio implementation when a device
> tries to read these registers to detect if the region exists?

You probably mean that the driver is reading, right? Not sure if we
ever specified what happens if a driver interacts with a register that
was not specified when the device was written... maybe we need to bump
the device version number? But that would break old drivers if they get
a 3 but expected a 2.

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: [virtio-comment] Re: [PATCH v2 3/3] shared memory: Define mmio registers

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1553 bytes --]

On Wed, Feb 27, 2019 at 02:56:52PM +0100, Cornelia Huck wrote:
> On Mon, 25 Feb 2019 18:45:06 +0000
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > On Fri, Feb 22, 2019 at 11:54:33AM +0000, Dr. David Alan Gilbert (git) wrote:  
> 
> > > > +    region, as defined by the device for the region selected by
> > > > +    the \field{SHMId} register.  Reading from a non-existent
> > > > +    region (i.e. where the ID written to \field{SHMId} is unused)
> > > > +    results in a length of -1.  
> > > 
> > > -1 is used to indicate the absence of a region, does 0 have a meaning?  
> > 
> > No, I'd be happy to switch; although this does lead to another question;
> > what happens on an older virtio-mmio implementation when a device
> > tries to read these registers to detect if the region exists?
> 
> You probably mean that the driver is reading, right? Not sure if we
> ever specified what happens if a driver interacts with a register that
> was not specified when the device was written... maybe we need to bump
> the device version number? But that would break old drivers if they get
> a 3 but expected a 2.

I see the rationale for using 0xffffffff since that's what loads produce
when there is nothing at the memory address.  It would be good to
document that :).

Incrementing the version as Cornelia suggested is cleaner though, if
existing drivers handle that gracefully.  Or maybe a transport feature
bit (are they in short supply?).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[virtio-comment] Re: [virtio-dev] Re: [PATCH v2 3/3] shared memory: Define mmio registers

[Dr. David Alan Gilbert]

* Cornelia Huck (cohuck@redhat.com) wrote:
> On Fri, 22 Feb 2019 11:54:33 +0000
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Define an MMIO interface to enumerate and map shared
> > memory regions.
> > 
> > Note: This hasn't been implemented.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  content.tex | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> 
> (...)
> 
> > @@ -1815,6 +1840,13 @@ If both values are valid, it MUST read \field{DeviceID}
> >  and if its value is zero (0x0) MUST abort initialization and
> >  MUST NOT access any other register.
> >  
> > +Devices expecting shared memory regions MUST enumerate the
> > +regions by selecting each region in tern and checking that the
> 
> s/tern/turn/

I'm removing this paragraph; see below.

> > +length is not -1.
> > +
> > +Devices not expecting shared memory MUST NOT use the shared
> > +memory registers.
> 
> Isn't that the _driver_ requirements section? These sentences also make
> more sense if they talk about a driver expecting something.

Yes, this should be 'Drivers not expecting ...'

> What I also find a bit confusing is the requirement that they enumerate
> all regions (at least, that's how I read it.)
> 
> Say we have a device type with shared regions foo, bar, and the
> optional baz. It should be fine if a driver finds foo, bar, and baz; as
> well as if a driver only finds foo and bar. Conversely, a driver only
> looking for foo and bar should work with a device exposing foo, bar,
> and baz.

Yes, I've killed off the paragraph about enumeration.

Dave

> > +
> >  Further initialization MUST follow the procedure described in
> >  \ref{sec:General Initialization And Device Operation / Device Initialization}~\nameref{sec:General Initialization And Device Operation / Device Initialization}.
> >  
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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/

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v2 3/3] shared memory: Define mmio registers

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Wed, Feb 27, 2019 at 02:56:52PM +0100, Cornelia Huck wrote:
> > On Mon, 25 Feb 2019 18:45:06 +0000
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > 
> > > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > > On Fri, Feb 22, 2019 at 11:54:33AM +0000, Dr. David Alan Gilbert (git) wrote:  
> > 
> > > > > +    region, as defined by the device for the region selected by
> > > > > +    the \field{SHMId} register.  Reading from a non-existent
> > > > > +    region (i.e. where the ID written to \field{SHMId} is unused)
> > > > > +    results in a length of -1.  
> > > > 
> > > > -1 is used to indicate the absence of a region, does 0 have a meaning?  
> > > 
> > > No, I'd be happy to switch; although this does lead to another question;
> > > what happens on an older virtio-mmio implementation when a device
> > > tries to read these registers to detect if the region exists?
> > 
> > You probably mean that the driver is reading, right? Not sure if we
> > ever specified what happens if a driver interacts with a register that
> > was not specified when the device was written... maybe we need to bump
> > the device version number? But that would break old drivers if they get
> > a 3 but expected a 2.
> 
> I see the rationale for using 0xffffffff since that's what loads produce
> when there is nothing at the memory address.  It would be good to
> document that :).

Is that actually defined that we get 0xffffffff? - in which case
yes I'd add it as a reasoning; but I hadn't realised it was actually
defined anywhere.

> Incrementing the version as Cornelia suggested is cleaner though, if
> existing drivers handle that gracefully.  Or maybe a transport feature
> bit (are they in short supply?).

We do have transport feature bits for other transports don't we, and
they can overlap those.

Dave

> Stefan


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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/

[virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v2 3/3] shared memory: Define mmio registers

[Dr. David Alan Gilbert]

In discussions with Sebastien (cc'd) who is actually trying to use the
mmio definition, there's two other changes:

  a) Change ShmId to ShmSel to be consistent with the other MMIO
registers
  b) Make SHMBaseLow & High be read-only by the driver and be set by
the device/VMM - i.e. like the proposed CCW setup, in the MMIO setup
it becomes the VMMs problem to chose the GPA rather than the guests.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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/

[virtio-comment] Re: [virtio-dev] Re: [PATCH v2 1/3] shared memory: Define shared memory regions

[Dr. David Alan Gilbert]

* Cornelia Huck (cohuck@redhat.com) wrote:
> On Tue, 26 Feb 2019 15:11:34 +0000
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Cornelia Huck (cohuck@redhat.com) wrote:
> > > On Fri, 22 Feb 2019 11:54:31 +0000
> > > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > >   
> > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > 
> > > > Define the requirements and idea behind shared memory regions.
> > > > 
> > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > ---
> > > >  content.tex    |  2 ++
> > > >  shared-mem.tex | 33 +++++++++++++++++++++++++++++++++
> > > >  2 files changed, 35 insertions(+)
> > > >  create mode 100644 shared-mem.tex
> > > > 
> > > > diff --git a/content.tex b/content.tex
> > > > index 836ee52..3dd504c 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -371,6 +371,8 @@ making any more buffers available. When VIRTIO_F_NOTIFICATION_DATA
> > > >  has been negotiated, these notifications would then have
> > > >  identical \field{next_off} and \field{next_wrap} values.
> > > >  
> > > > +\input{shared-mem.tex}
> > > > +
> > > >  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> > > >  
> > > >  We start with an overview of device initialization, then expand on the
> > > > diff --git a/shared-mem.tex b/shared-mem.tex
> > > > new file mode 100644
> > > > index 0000000..85b0c55
> > > > --- /dev/null
> > > > +++ b/shared-mem.tex
> > > > @@ -0,0 +1,33 @@
> > > > +\section{Shared Memory Regions}\label{sec:Basic Facilities of a Virtio Device / Shared Memory Regions}
> > > > +
> > > > +Shared memory regions are an additional facility
> > > > +available to devices that need a region of memory that's
> > > > +continuously shared between the host and the guest, rather
> > > > +than passed between them in the way virtqueue elements are.
> > > > +
> > > > +Example uses include shared caches and version pools for versioned
> > > > +data structures.
> > > > +
> > > > +The region is chosen by the host and presented to the guest, as
> > > > +such it is useful in situations where the memory is accessed on
> > > > +the host by other libraries that can't safely access guest RAM.  
> > > 
> > > This explanation looks good to me.
> > >   
> > > > +
> > > > +Shared memory regions MUST NOT be used to control the operation
> > > > +of the device, nor to stream data; those should still be performed
> > > > +using virtqueues.  
> > > 
> > > The 'MUST NOT' makes it look like a normative statement; however, this
> > > is more like design advice?  
> > 
> > Hmm I'm not sure - the intention of this line is to try and stop people
> > using it as a hack to avoid standardising protocol for no good reason.
> 
> I've looked through the spec again and it seems the best place for this
> would indeed be a device-normative section for shared regions; it would
> imply that a device needs to comply with this statement, or it is not
> conformant.
> 
> It would need some tweaking, however. Perhaps something like the
> following:
> 
> "The device MUST NOT expose shared memory regions which are used to
> control the operation of the device, or to stream data."

> Not sure where to put the pointer to use virtqueues for that.

Thanks I've turned that into a device normative section and dropped the
explicit mention of virtqueues.

> 
> > 
> > > > +
> > > > +A device may have multiple shared memory regions associated with
> > > > +it.  Each region has a \field{shmid} to identify it, the meaning
> > > > +of which is device-specific.
> > > > +
> > > > +Enumeration and location of shared memory regions is performed
> > > > +using a transport-specific data structure and mechanism.
> > > > +
> > > > +Memory consistency rules vary depending on the region and the
> > > > +device.  Devices MUST define the required behaviour for each
> > > > +region.  
> > > 
> > > Same here.  
> > 
> > OK, so should I reword this?
> 
> Maybe put into the device normative section:
> 
> "A device MUST define the required behaviour for each region."
> 
> But I'm not sure whether this is a good normative statement... it's
> more that the regions have specific requirements, which both the device
> and the driver need to fulfill, and those requirements obviously need
> to be put down somewhere.
> 
> > 
> > > > +
> > > > +The guest physical address and the host virtual address MUST NOT
> > > > +be used to identify structures within the memory regions; all
> > > > +addressing MUST be relative to the start of a particular region.
> > > > +  
> > > 
> > > For that high-level overview, I'm not sure if any normative statements
> > > are needed/wanted, or whether those should be confined to the individual
> > > transport or device type definitions...  
> > 
> > I think this one is stronger than the previous two; if people start
> > passing GPA/HVA in the underlying structures things are going to get
> > messy.
> 
> What about
> 
> "The device MUST handle any access to the memory region as addressed
> relatively to the beginning of the region, regardless whether it
> accesses the region itself or the driver does so."
> 
> "The driver MUST address any particular region relatively to the start
> of that region."
> 
> for device and driver normative statements, respectively. Better
> wordings welcome.

What I've done here is turn this into just a subsection describing
how it should work and stopped using the magic MUST.
It's not really a requirement on the device - it's more an indication
of how to design a device.

Dave

> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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: [virtio-comment] [PATCH v2 0/3] Large shared memory regions

[Frank Yang]

[-- Attachment #1: Type: text/plain, Size: 2286 bytes --]

On Fri, Feb 22, 2019 at 3:54 AM Dr. David Alan Gilbert (git) <
dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Hi,
>   This series formalises the idea of shared memory regions I'd
> previously discussed and is intended for use with virtio-fs.
>
> I've split it into three parts:
>
>   a) A general definition of the idea
>
>   b) A definition on virtio-pci using the layout previously
> discussed; this still uses the list of capabilities.
>
>   c) A definition on virtio-mmio; this is untested
> however I thought best to include it since it shows that
> it isn't tied to PCI.
>
>
> This v2 addresses (hopefully all) comments from the initial posting,
> except for CCW and the GPU/dynamic usage currently being
> discussed by Frank.  Cornelia has a suggested approach for CCW
> that is being discussed, so it looks like we're getting there on
> that.  The GPU usage being discussed by Frank is diverging to a
> very different usage.
>
>
I've modified the virtio-hostmem stuff to not be dynamic (and posted it);
I think we can get that kind of usage using your current definitions.

Dave
>
> Dr. David Alan Gilbert (3):
>   shared memory: Define shared memory regions
>   shared memory: Define PCI capability
>   shared memory: Define mmio registers
>
>  content.tex    | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  shared-mem.tex | 33 +++++++++++++++++++++++
>  2 files changed, 104 insertions(+)
>  create mode 100644 shared-mem.tex
>
> --
> 2.20.1
>
>
> 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/
>
>

[-- Attachment #2: Type: text/html, Size: 3795 bytes --]

Re: [virtio-comment] [PATCH v2 0/3] Large shared memory regions

[Michael S. Tsirkin]

On Tue, Mar 05, 2019 at 07:47:52AM -0800, Frank Yang wrote:
> 
> 
> On Fri, Feb 22, 2019 at 3:54 AM Dr. David Alan Gilbert (git) <
> dgilbert@redhat.com> wrote:
> 
>     From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
>     Hi,
>       This series formalises the idea of shared memory regions I'd
>     previously discussed and is intended for use with virtio-fs.
> 
>     I've split it into three parts:
> 
>       a) A general definition of the idea
> 
>       b) A definition on virtio-pci using the layout previously
>     discussed; this still uses the list of capabilities.
> 
>       c) A definition on virtio-mmio; this is untested
>     however I thought best to include it since it shows that
>     it isn't tied to PCI.
> 
> 
>     This v2 addresses (hopefully all) comments from the initial posting,
>     except for CCW and the GPU/dynamic usage currently being
>     discussed by Frank.  Cornelia has a suggested approach for CCW
>     that is being discussed, so it looks like we're getting there on
>     that.  The GPU usage being discussed by Frank is diverging to a
>     very different usage.
> 
> 
>  
> I've modified the virtio-hostmem stuff to not be dynamic (and posted it);
> I think we can get that kind of usage using your current definitions.

OK that's an ack then?

> 
>     Dave
> 
>     Dr. David Alan Gilbert (3):
>       shared memory: Define shared memory regions
>       shared memory: Define PCI capability
>       shared memory: Define mmio registers
> 
>      content.tex    | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>      shared-mem.tex | 33 +++++++++++++++++++++++
>      2 files changed, 104 insertions(+)
>      create mode 100644 shared-mem.tex
> 
>     --
>     2.20.1
> 
> 
>     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/
> 
> 

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: [virtio-comment] [PATCH v2 0/3] Large shared memory regions

[Frank Yang]

[-- Attachment #1: Type: text/plain, Size: 2790 bytes --]

Yup

On Mon, Jun 17, 2019 at 11:38 AM Michael S. Tsirkin <mst@redhat.com> wrote:

> On Tue, Mar 05, 2019 at 07:47:52AM -0800, Frank Yang wrote:
> >
> >
> > On Fri, Feb 22, 2019 at 3:54 AM Dr. David Alan Gilbert (git) <
> > dgilbert@redhat.com> wrote:
> >
> >     From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> >     Hi,
> >       This series formalises the idea of shared memory regions I'd
> >     previously discussed and is intended for use with virtio-fs.
> >
> >     I've split it into three parts:
> >
> >       a) A general definition of the idea
> >
> >       b) A definition on virtio-pci using the layout previously
> >     discussed; this still uses the list of capabilities.
> >
> >       c) A definition on virtio-mmio; this is untested
> >     however I thought best to include it since it shows that
> >     it isn't tied to PCI.
> >
> >
> >     This v2 addresses (hopefully all) comments from the initial posting,
> >     except for CCW and the GPU/dynamic usage currently being
> >     discussed by Frank.  Cornelia has a suggested approach for CCW
> >     that is being discussed, so it looks like we're getting there on
> >     that.  The GPU usage being discussed by Frank is diverging to a
> >     very different usage.
> >
> >
> >
> > I've modified the virtio-hostmem stuff to not be dynamic (and posted it);
> > I think we can get that kind of usage using your current definitions.
>
> OK that's an ack then?
>
> >
> >     Dave
> >
> >     Dr. David Alan Gilbert (3):
> >       shared memory: Define shared memory regions
> >       shared memory: Define PCI capability
> >       shared memory: Define mmio registers
> >
> >      content.tex    | 71
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> >      shared-mem.tex | 33 +++++++++++++++++++++++
> >      2 files changed, 104 insertions(+)
> >      create mode 100644 shared-mem.tex
> >
> >     --
> >     2.20.1
> >
> >
> >     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/
> >
> >
>

[-- Attachment #2: Type: text/html, Size: 4530 bytes --]