* [virtio-comment] [PATCH v5 0/1] virtio-pmem: Support describing pmem as shared memory region @ 2021-11-10 18:55 tstark 2021-11-10 18:55 ` [virtio-comment] [PATCH v5 1/1] " tstark 0 siblings, 1 reply; 13+ messages in thread From: tstark @ 2021-11-10 18:55 UTC (permalink / raw) To: virtio-comment; +Cc: grahamwo, benhill, tstark, pankaj.gupta.linux From: Taylor Stark <tstark@microsoft.com> Changes from v4 [1]: - Clarified which fields of virtio_pmem_config are valid depending on shared memory feature negotiation (suggested by Cornelia Huck). Changes from v3 [1]: - Rebased onto master now that the base virtio-pmem spec has been merged. Changes from v2: - Incorporated suggestions from Cornelia Huck on rewording driver initialization. Changes from v1: - 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/202111/msg00004.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 | 14 ++++++++++++-- virtio-pmem.tex | 40 +++++++++++++++++++++++++++++++++------- 2 files changed, 45 insertions(+), 9 deletions(-) -- 2.33.0.vfs.0.0 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/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* [virtio-comment] [PATCH v5 1/1] virtio-pmem: Support describing pmem as shared memory region 2021-11-10 18:55 [virtio-comment] [PATCH v5 0/1] virtio-pmem: Support describing pmem as shared memory region tstark @ 2021-11-10 18:55 ` tstark 2021-11-11 10:52 ` Cornelia Huck ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: tstark @ 2021-11-10 18:55 UTC (permalink / raw) To: virtio-comment; +Cc: grahamwo, benhill, tstark, pankaj.gupta.linux From: Taylor Stark <tstark@microsoft.com> Update the virtio-pmem 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 | 14 ++++++++++++-- virtio-pmem.tex | 40 +++++++++++++++++++++++++++++++++------- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/conformance.tex b/conformance.tex index 80547db..d9f2b45 100644 --- a/conformance.tex +++ b/conformance.tex @@ -31,8 +31,9 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} \ref{sec:Conformance / Driver Conformance / Sound Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Memory Driver Conformance}, \ref{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance}, -\ref{sec:Conformance / Driver Conformance / SCMI Driver Conformance} or -\ref{sec:Conformance / Driver Conformance / GPIO Driver Conformance}. +\ref{sec:Conformance / Driver Conformance / SCMI Driver Conformance}, +\ref{sec:Conformance / Driver Conformance / GPIO Driver Conformance} or +\ref{sec:Conformance / Driver Conformance / PMEM Driver Conformance}. \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}. \end{itemize} @@ -314,6 +315,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} \item \ref{drivernormative:Device Types / GPIO Device / eventq Operation} \end{itemize} +\conformance{\subsection}{PMEM Driver Conformance}\label{sec:Conformance / Driver Conformance / PMEM Driver Conformance} + +A PMEM driver MUST conform to the following normative statements: + +\begin{itemize} +\item \ref{drivernormative:Device Types / PMEM Device / Device Initialization} +\end{itemize} + \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance} A device MUST conform to the following normative statements: @@ -578,6 +587,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} A PMEM device MUST conform to the following normative statements: \begin{itemize} +\item \ref{devicenormative:Device Types / PMEM Device / Device Initialization} \item \ref{devicenormative:Device Types / PMEM Device / Device Operation / Virtqueue flush} \item \ref{devicenormative:Device Types / PMEM Device / Device Operation / Virtqueue return} \end{itemize} diff --git a/virtio-pmem.tex b/virtio-pmem.tex index 93ab3c1..384eaa3 100644 --- a/virtio-pmem.tex +++ b/virtio-pmem.tex @@ -24,7 +24,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} @@ -36,22 +39,45 @@ \subsection{Device configuration layout}\label{sec:Device Types / PMEM Device / \end{lstlisting} \begin{description} -\item[\field{start}] contains the physical address of the first byte of the persistent memory region. +\item[\field{start}] contains the physical address of the first byte of the +persistent memory region, if VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. -\item[\field{size}] contains the length of this address range. +\item[\field{size}] contains the length of this address range, if +VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. \end{description} +\subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device Initialization} + +The device indicates the guest physical address to the driver in one of two ways: \begin{enumerate} -\item Driver vpmem start is read from \field{start}. -\item Driver vpmem end is read from \field{size}. +\item As a guest absolute address, using virtio_pmem_config. +\item As a shared memory region. \end{enumerate} -\subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization} - The driver determines the start address and size of the persistent memory region in preparation for reading or writing data. The driver initializes req_vq in preparation for making flush requests. +\devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization} + +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. + +\drivernormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization} + +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}. + \subsection{Driver Operations}\label{sec:Device Types / PMEM Driver / Driver Operation / Request Queues} Requests have the following format: -- 2.33.0.vfs.0.0 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/ ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [virtio-comment] [PATCH v5 1/1] virtio-pmem: Support describing pmem as shared memory region 2021-11-10 18:55 ` [virtio-comment] [PATCH v5 1/1] " tstark @ 2021-11-11 10:52 ` Cornelia Huck 2021-11-11 18:17 ` Taylor Stark 2021-11-23 16:42 ` Stefan Hajnoczi 2021-11-16 0:31 ` [virtio-comment] " Pankaj Gupta 2021-11-18 17:37 ` [virtio-comment] " Halil Pasic 2 siblings, 2 replies; 13+ messages in thread From: Cornelia Huck @ 2021-11-11 10:52 UTC (permalink / raw) To: tstark, virtio-comment; +Cc: grahamwo, benhill, tstark, pankaj.gupta.linux On Wed, Nov 10 2021, tstark@linux.microsoft.com wrote: > From: Taylor Stark <tstark@microsoft.com> > > Update the virtio-pmem 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 | 14 ++++++++++++-- > virtio-pmem.tex | 40 +++++++++++++++++++++++++++++++++------- > 2 files changed, 45 insertions(+), 9 deletions(-) This looks good to me now. Could you please open an issue at https://github.com/oasis-tcs/virtio-spec/issues to get this included? If nobody has any further comments, I think we could start the voting process soon. 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/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [virtio-comment] [PATCH v5 1/1] virtio-pmem: Support describing pmem as shared memory region 2021-11-11 10:52 ` Cornelia Huck @ 2021-11-11 18:17 ` Taylor Stark 2021-11-23 16:42 ` Stefan Hajnoczi 1 sibling, 0 replies; 13+ messages in thread From: Taylor Stark @ 2021-11-11 18:17 UTC (permalink / raw) To: Cornelia Huck Cc: virtio-comment, grahamwo, benhill, tstark, pankaj.gupta.linux On Thu, Nov 11, 2021 at 11:52:42AM +0100, Cornelia Huck wrote: > On Wed, Nov 10 2021, tstark@linux.microsoft.com wrote: > > > From: Taylor Stark <tstark@microsoft.com> > > > > Update the virtio-pmem 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 | 14 ++++++++++++-- > > virtio-pmem.tex | 40 +++++++++++++++++++++++++++++++++------- > > 2 files changed, 45 insertions(+), 9 deletions(-) > > This looks good to me now. > > Could you please open an issue at > https://github.com/oasis-tcs/virtio-spec/issues to get this included? If > nobody has any further comments, I think we could start the voting > process soon. Sounds good, thanks Cornelia! Fixes: https://github.com/oasis-tcs/virtio-spec/issues/121 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [virtio-comment] [PATCH v5 1/1] virtio-pmem: Support describing pmem as shared memory region 2021-11-11 10:52 ` Cornelia Huck 2021-11-11 18:17 ` Taylor Stark @ 2021-11-23 16:42 ` Stefan Hajnoczi 2021-11-23 16:49 ` Cornelia Huck 1 sibling, 1 reply; 13+ messages in thread From: Stefan Hajnoczi @ 2021-11-23 16:42 UTC (permalink / raw) To: Cornelia Huck Cc: tstark, virtio-comment, grahamwo, benhill, tstark, pankaj.gupta.linux [-- Attachment #1: Type: text/plain, Size: 1025 bytes --] On Thu, Nov 11, 2021 at 11:52:42AM +0100, Cornelia Huck wrote: > On Wed, Nov 10 2021, tstark@linux.microsoft.com wrote: > > > From: Taylor Stark <tstark@microsoft.com> > > > > Update the virtio-pmem 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 | 14 ++++++++++++-- > > virtio-pmem.tex | 40 +++++++++++++++++++++++++++++++++------- > > 2 files changed, 45 insertions(+), 9 deletions(-) > > This looks good to me now. I noticed the patch uses "guest absolute address" instead of "physical address". This terminology was fixed in the original pmem patch and I guess the shared memory patch wasn't updated. Please don't use "guest". Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [virtio-comment] [PATCH v5 1/1] virtio-pmem: Support describing pmem as shared memory region 2021-11-23 16:42 ` Stefan Hajnoczi @ 2021-11-23 16:49 ` Cornelia Huck 2021-11-24 15:36 ` Stefan Hajnoczi 0 siblings, 1 reply; 13+ messages in thread From: Cornelia Huck @ 2021-11-23 16:49 UTC (permalink / raw) To: Stefan Hajnoczi Cc: tstark, virtio-comment, grahamwo, benhill, tstark, pankaj.gupta.linux On Tue, Nov 23 2021, Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Thu, Nov 11, 2021 at 11:52:42AM +0100, Cornelia Huck wrote: >> On Wed, Nov 10 2021, tstark@linux.microsoft.com wrote: >> >> > From: Taylor Stark <tstark@microsoft.com> >> > >> > Update the virtio-pmem 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 | 14 ++++++++++++-- >> > virtio-pmem.tex | 40 +++++++++++++++++++++++++++++++++------- >> > 2 files changed, 45 insertions(+), 9 deletions(-) >> >> This looks good to me now. > > I noticed the patch uses "guest absolute address" instead of "physical > address". This terminology was fixed in the original pmem patch and I > guess the shared memory patch wasn't updated. Please don't use "guest". So s/guest absolute/physical/ ? That can be fixed as an editorial update on top. 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/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [virtio-comment] [PATCH v5 1/1] virtio-pmem: Support describing pmem as shared memory region 2021-11-23 16:49 ` Cornelia Huck @ 2021-11-24 15:36 ` Stefan Hajnoczi 0 siblings, 0 replies; 13+ messages in thread From: Stefan Hajnoczi @ 2021-11-24 15:36 UTC (permalink / raw) To: Cornelia Huck Cc: tstark, virtio-comment, grahamwo, benhill, tstark, pankaj.gupta.linux [-- Attachment #1: Type: text/plain, Size: 1343 bytes --] On Tue, Nov 23, 2021 at 05:49:58PM +0100, Cornelia Huck wrote: > On Tue, Nov 23 2021, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > On Thu, Nov 11, 2021 at 11:52:42AM +0100, Cornelia Huck wrote: > >> On Wed, Nov 10 2021, tstark@linux.microsoft.com wrote: > >> > >> > From: Taylor Stark <tstark@microsoft.com> > >> > > >> > Update the virtio-pmem 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 | 14 ++++++++++++-- > >> > virtio-pmem.tex | 40 +++++++++++++++++++++++++++++++++------- > >> > 2 files changed, 45 insertions(+), 9 deletions(-) > >> > >> This looks good to me now. > > > > I noticed the patch uses "guest absolute address" instead of "physical > > address". This terminology was fixed in the original pmem patch and I > > guess the shared memory patch wasn't updated. Please don't use "guest". > > So s/guest absolute/physical/ ? That can be fixed as an editorial update > on top. > Yes, please. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* [virtio-comment] Re: [PATCH v5 1/1] virtio-pmem: Support describing pmem as shared memory region 2021-11-10 18:55 ` [virtio-comment] [PATCH v5 1/1] " tstark 2021-11-11 10:52 ` Cornelia Huck @ 2021-11-16 0:31 ` Pankaj Gupta 2021-11-18 17:37 ` [virtio-comment] " Halil Pasic 2 siblings, 0 replies; 13+ messages in thread From: Pankaj Gupta @ 2021-11-16 0:31 UTC (permalink / raw) To: Taylor Stark; +Cc: virtio-comment, grahamwo, benhill, tstark > Update the virtio-pmem 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 | 14 ++++++++++++-- > virtio-pmem.tex | 40 +++++++++++++++++++++++++++++++++------- > 2 files changed, 45 insertions(+), 9 deletions(-) > > diff --git a/conformance.tex b/conformance.tex > index 80547db..d9f2b45 100644 > --- a/conformance.tex > +++ b/conformance.tex > @@ -31,8 +31,9 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} > \ref{sec:Conformance / Driver Conformance / Sound Driver Conformance}, > \ref{sec:Conformance / Driver Conformance / Memory Driver Conformance}, > \ref{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance}, > -\ref{sec:Conformance / Driver Conformance / SCMI Driver Conformance} or > -\ref{sec:Conformance / Driver Conformance / GPIO Driver Conformance}. > +\ref{sec:Conformance / Driver Conformance / SCMI Driver Conformance}, > +\ref{sec:Conformance / Driver Conformance / GPIO Driver Conformance} or > +\ref{sec:Conformance / Driver Conformance / PMEM Driver Conformance}. > > \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}. > \end{itemize} > @@ -314,6 +315,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} > \item \ref{drivernormative:Device Types / GPIO Device / eventq Operation} > \end{itemize} > > +\conformance{\subsection}{PMEM Driver Conformance}\label{sec:Conformance / Driver Conformance / PMEM Driver Conformance} > + > +A PMEM driver MUST conform to the following normative statements: > + > +\begin{itemize} > +\item \ref{drivernormative:Device Types / PMEM Device / Device Initialization} > +\end{itemize} > + > \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance} > > A device MUST conform to the following normative statements: > @@ -578,6 +587,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets} > A PMEM device MUST conform to the following normative statements: > > \begin{itemize} > +\item \ref{devicenormative:Device Types / PMEM Device / Device Initialization} > \item \ref{devicenormative:Device Types / PMEM Device / Device Operation / Virtqueue flush} > \item \ref{devicenormative:Device Types / PMEM Device / Device Operation / Virtqueue return} > \end{itemize} > diff --git a/virtio-pmem.tex b/virtio-pmem.tex > index 93ab3c1..384eaa3 100644 > --- a/virtio-pmem.tex > +++ b/virtio-pmem.tex > @@ -24,7 +24,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} > > @@ -36,22 +39,45 @@ \subsection{Device configuration layout}\label{sec:Device Types / PMEM Device / > \end{lstlisting} > > \begin{description} > -\item[\field{start}] contains the physical address of the first byte of the persistent memory region. > +\item[\field{start}] contains the physical address of the first byte of the > +persistent memory region, if VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. > > -\item[\field{size}] contains the length of this address range. > +\item[\field{size}] contains the length of this address range, if > +VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. > \end{description} > > +\subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device Initialization} > + > +The device indicates the guest physical address to the driver in one of two ways: > \begin{enumerate} > -\item Driver vpmem start is read from \field{start}. > -\item Driver vpmem end is read from \field{size}. > +\item As a guest absolute address, using virtio_pmem_config. > +\item As a shared memory region. > \end{enumerate} > > -\subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization} > - > The driver determines the start address and size of the persistent memory region in preparation for reading or writing data. > > The driver initializes req_vq in preparation for making flush requests. > > +\devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization} > + > +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. > + > +\drivernormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization} > + > +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}. > + > \subsection{Driver Operations}\label{sec:Device Types / PMEM Driver / Driver Operation / Request Queues} > > Requests have the following format: Looks good to me. Reviewed-by: Pankaj Gupta <pankaj.gupta.linux@gmail.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/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [virtio-comment] [PATCH v5 1/1] virtio-pmem: Support describing pmem as shared memory region 2021-11-10 18:55 ` [virtio-comment] [PATCH v5 1/1] " tstark 2021-11-11 10:52 ` Cornelia Huck 2021-11-16 0:31 ` [virtio-comment] " Pankaj Gupta @ 2021-11-18 17:37 ` Halil Pasic 2021-11-21 8:08 ` Taylor Stark 2 siblings, 1 reply; 13+ messages in thread From: Halil Pasic @ 2021-11-18 17:37 UTC (permalink / raw) To: tstark Cc: virtio-comment, grahamwo, benhill, tstark, pankaj.gupta.linux, Halil Pasic On Wed, 10 Nov 2021 10:55:55 -0800 tstark@linux.microsoft.com wrote: [..] > @@ -36,22 +39,45 @@ \subsection{Device configuration layout}\label{sec:Device Types / PMEM Device / > \end{lstlisting} > > \begin{description} > -\item[\field{start}] contains the physical address of the first byte of the persistent memory region. > +\item[\field{start}] contains the physical address of the first byte of the > +persistent memory region, if VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. > > -\item[\field{size}] contains the length of this address range. > +\item[\field{size}] contains the length of this address range, if > +VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. > \end{description} > > +\subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device Initialization} > + > +The device indicates the guest physical address to the driver in one of two ways: > \begin{enumerate} > -\item Driver vpmem start is read from \field{start}. > -\item Driver vpmem end is read from \field{size}. > +\item As a guest absolute address, using virtio_pmem_config. > +\item As a shared memory region. > \end{enumerate} > > -\subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization} > - > The driver determines the start address and size of the persistent memory region in preparation for reading or writing data. > > The driver initializes req_vq in preparation for making flush requests. > > +\devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization} > + > +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. Sorry for joining in this late. I'm wondering if the quoted parts implies that the device SHOULD change its config space (fields start and size) when the driver sets FEATURES_OK, and the feature is VIRTIO_PMEM_F_SHMEM_REGION set at the device (it was offered, and got set). My train of thought is: initially no feature is considered negotiated, and config space access is allowed before feature negotiation is completed. That means the at this stage the device must indicate via the config space, and thus \field{size} != 0. But as a part of the transition 'features not negotiated' -> 'features negotiated' the device needs to go '\field{size} != 0' -> '\field{size} == 0' Is that what we want? Also I understand that Hyper-V would make the device cease operation if the feature VIRTIO_PMEM_F_SHMEM_REGION was not accepted by the driver. I believe I've read that in some previous email. Do we need a 'MAY fail to operate further if' statement? Anything that ain't guarded by feature bits ain't optional, and anything that is guarded by feature bits is optional unless explicitly stated otherwise. Regards, Halil 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/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [virtio-comment] [PATCH v5 1/1] virtio-pmem: Support describing pmem as shared memory region 2021-11-18 17:37 ` [virtio-comment] " Halil Pasic @ 2021-11-21 8:08 ` Taylor Stark 2021-11-22 11:58 ` Cornelia Huck 0 siblings, 1 reply; 13+ messages in thread From: Taylor Stark @ 2021-11-21 8:08 UTC (permalink / raw) To: Halil Pasic; +Cc: virtio-comment, grahamwo, benhill, tstark, pankaj.gupta.linux On Thu, Nov 18, 2021 at 06:37:52PM +0100, Halil Pasic wrote: > On Wed, 10 Nov 2021 10:55:55 -0800 > tstark@linux.microsoft.com wrote: > [..] > > @@ -36,22 +39,45 @@ \subsection{Device configuration layout}\label{sec:Device Types / PMEM Device / > > \end{lstlisting} > > > > \begin{description} > > -\item[\field{start}] contains the physical address of the first byte of the persistent memory region. > > +\item[\field{start}] contains the physical address of the first byte of the > > +persistent memory region, if VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. > > > > -\item[\field{size}] contains the length of this address range. > > +\item[\field{size}] contains the length of this address range, if > > +VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. > > \end{description} > > > > +\subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device Initialization} > > + > > +The device indicates the guest physical address to the driver in one of two ways: > > \begin{enumerate} > > -\item Driver vpmem start is read from \field{start}. > > -\item Driver vpmem end is read from \field{size}. > > +\item As a guest absolute address, using virtio_pmem_config. > > +\item As a shared memory region. > > \end{enumerate} > > > > -\subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization} > > - > > The driver determines the start address and size of the persistent memory region in preparation for reading or writing data. > > > > The driver initializes req_vq in preparation for making flush requests. > > > > +\devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization} > > + > > +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. > > Sorry for joining in this late. > > I'm wondering if the quoted parts implies that the device SHOULD change > its config space (fields start and size) when the driver sets > FEATURES_OK, and the feature is VIRTIO_PMEM_F_SHMEM_REGION set at the > device (it was offered, and got set). > > My train of thought is: initially no feature is considered negotiated, > and config space access is allowed before feature negotiation is > completed. That means the at this stage the device must indicate via > the config space, and thus \field{size} != 0. But as a part of the > transition 'features not negotiated' -> 'features negotiated' the device > needs to go '\field{size} != 0' -> '\field{size} == 0' > > Is that what we want? > > Also I understand that Hyper-V would make the device cease operation if > the feature VIRTIO_PMEM_F_SHMEM_REGION was not accepted by the driver. > I believe I've read that in some previous email. > > Do we need a 'MAY fail to operate further if' statement? Anything that > ain't guarded by feature bits ain't optional, and anything that is > guarded by feature bits is optional unless explicitly stated otherwise. > > Regards, > Halil No problem, thanks for joining in! :) I think your train of thought makes sense. You're right, for Hyper-V we can't support any driver that doesn't support VIRTIO_PMEM_F_SHMEM_REGION. So we have no choice but to set the config fields to zero from the start. There's no meaningful value we can place in those fields. I'm trying to figure out how best to word this. Saying that devices should set config fields prior to FEATURE_OK is redundant. Should I add a section saying that config fields MAY be set to 0 if VIRTIO_PMEM_F_SHMEM_REGION is offerred (such as in the Hyper-V case)? And that if it's set to 0 and the feature isn't negotiated, the device must reject the driver? I'd also have to add in a section saying that the pmem region shouldn't be touched by the driver prior to DRIVER_OK being set, to save drivers from accessing the zeroed values placed in the config. Additionally, encouraging devices to set the config fields to 0 once the feature has been negotiated seems like the right thing to do, to catch bugs? I'm not sure about the 'MAY fail to operate further statement'. Couldn't there be lots of reasons why a device rejects a driver? And wouldn't those be implementation details of each device? Is it common to document those? Thoughts? Thanks! Taylor 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/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [virtio-comment] [PATCH v5 1/1] virtio-pmem: Support describing pmem as shared memory region 2021-11-21 8:08 ` Taylor Stark @ 2021-11-22 11:58 ` Cornelia Huck 2021-11-22 19:56 ` Halil Pasic 0 siblings, 1 reply; 13+ messages in thread From: Cornelia Huck @ 2021-11-22 11:58 UTC (permalink / raw) To: Taylor Stark, Halil Pasic Cc: virtio-comment, grahamwo, benhill, tstark, pankaj.gupta.linux On Sun, Nov 21 2021, Taylor Stark <tstark@linux.microsoft.com> wrote: > On Thu, Nov 18, 2021 at 06:37:52PM +0100, Halil Pasic wrote: >> On Wed, 10 Nov 2021 10:55:55 -0800 >> tstark@linux.microsoft.com wrote: >> [..] >> > @@ -36,22 +39,45 @@ \subsection{Device configuration layout}\label{sec:Device Types / PMEM Device / >> > \end{lstlisting} >> > >> > \begin{description} >> > -\item[\field{start}] contains the physical address of the first byte of the persistent memory region. >> > +\item[\field{start}] contains the physical address of the first byte of the >> > +persistent memory region, if VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. >> > >> > -\item[\field{size}] contains the length of this address range. >> > +\item[\field{size}] contains the length of this address range, if >> > +VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. >> > \end{description} >> > >> > +\subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device Initialization} >> > + >> > +The device indicates the guest physical address to the driver in one of two ways: >> > \begin{enumerate} >> > -\item Driver vpmem start is read from \field{start}. >> > -\item Driver vpmem end is read from \field{size}. >> > +\item As a guest absolute address, using virtio_pmem_config. >> > +\item As a shared memory region. >> > \end{enumerate} >> > >> > -\subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization} >> > - >> > The driver determines the start address and size of the persistent memory region in preparation for reading or writing data. >> > >> > The driver initializes req_vq in preparation for making flush requests. >> > >> > +\devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization} >> > + >> > +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 [for the sake of the argument made below, note the 'has not been negotiated' here] >> > +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. >> >> Sorry for joining in this late. >> >> I'm wondering if the quoted parts implies that the device SHOULD change >> its config space (fields start and size) when the driver sets >> FEATURES_OK, and the feature is VIRTIO_PMEM_F_SHMEM_REGION set at the >> device (it was offered, and got set). >> >> My train of thought is: initially no feature is considered negotiated, >> and config space access is allowed before feature negotiation is >> completed. That means the at this stage the device must indicate via >> the config space, and thus \field{size} != 0. But as a part of the >> transition 'features not negotiated' -> 'features negotiated' the device >> needs to go '\field{size} != 0' -> '\field{size} == 0' It has been advised that it SHOULD put 0 there, but I don't consider that a must, just a good idea. >> >> Is that what we want? >> >> Also I understand that Hyper-V would make the device cease operation if >> the feature VIRTIO_PMEM_F_SHMEM_REGION was not accepted by the driver. >> I believe I've read that in some previous email. >> >> Do we need a 'MAY fail to operate further if' statement? Anything that >> ain't guarded by feature bits ain't optional, and anything that is >> guarded by feature bits is optional unless explicitly stated otherwise. The device can always fail the setting of FEATURES_OK, see 2.2.2: "The device SHOULD accept any valid subset of features the driver accepts, otherwise it MUST fail to set the FEATURES_OK device status bit when the driver writes it." I'd argue that "SHMEM_REGION not set" can be considered a non-valid subset by the device. (Also, there's a SHOULD in there.) >> >> Regards, >> Halil > > No problem, thanks for joining in! :) I think your train of thought makes sense. > You're right, for Hyper-V we can't support any driver that doesn't support > VIRTIO_PMEM_F_SHMEM_REGION. So we have no choice but to set the config fields > to zero from the start. There's no meaningful value we can place in those fields. I think this is fine: we only require the fields to have meaningful contents if SHMEM_REGION has not been negotiated -- as long as the device has not accepted FEATURES_OK from the driver, nothing has been negotiated respectively not been negotiated. > > I'm trying to figure out how best to word this. Saying that devices should set > config fields prior to FEATURE_OK is redundant. Should I add a section saying that > config fields MAY be set to 0 if VIRTIO_PMEM_F_SHMEM_REGION is offerred (such as > in the Hyper-V case)? And that if it's set to 0 and the feature isn't negotiated, > the device must reject the driver? I'd also have to add in a section saying that > the pmem region shouldn't be touched by the driver prior to DRIVER_OK being set, > to save drivers from accessing the zeroed values placed in the config. Additionally, > encouraging devices to set the config fields to 0 once the feature has been > negotiated seems like the right thing to do, to catch bugs? I don't think we can prohibit the driver from reading the config space, that is explicitly allowed in the general case. We probably can add a non-normative extra statement, though, that a device may present zeroes in the config space before feature negotiation has finished if it doesn't support !SHMEM_REGION. That would also give the driver a way to discover that it doesn't make sense to continue without negotiating SHMEM_REGION. No extra normative statement, but should make it a bit more clear to those trying to implement this. > > I'm not sure about the 'MAY fail to operate further statement'. Couldn't there > be lots of reasons why a device rejects a driver? And wouldn't those be > implementation details of each device? Is it common to document those? We probably don't need such a statement (see above). As voting for this is open until tomorrow, we should quickly decide what to do. My preference would be to merge it as-is and do a clarification on top. 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/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [virtio-comment] [PATCH v5 1/1] virtio-pmem: Support describing pmem as shared memory region 2021-11-22 11:58 ` Cornelia Huck @ 2021-11-22 19:56 ` Halil Pasic 2021-11-23 11:10 ` Cornelia Huck 0 siblings, 1 reply; 13+ messages in thread From: Halil Pasic @ 2021-11-22 19:56 UTC (permalink / raw) To: Cornelia Huck Cc: Taylor Stark, virtio-comment, grahamwo, benhill, tstark, pankaj.gupta.linux, Halil Pasic On Mon, 22 Nov 2021 12:58:01 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On Sun, Nov 21 2021, Taylor Stark <tstark@linux.microsoft.com> wrote: > > > On Thu, Nov 18, 2021 at 06:37:52PM +0100, Halil Pasic wrote: > >> On Wed, 10 Nov 2021 10:55:55 -0800 > >> tstark@linux.microsoft.com wrote: > >> [..] > >> > @@ -36,22 +39,45 @@ \subsection{Device configuration layout}\label{sec:Device Types / PMEM Device / > >> > \end{lstlisting} > >> > > >> > \begin{description} > >> > -\item[\field{start}] contains the physical address of the first byte of the persistent memory region. > >> > +\item[\field{start}] contains the physical address of the first byte of the > >> > +persistent memory region, if VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. > >> > > >> > -\item[\field{size}] contains the length of this address range. > >> > +\item[\field{size}] contains the length of this address range, if > >> > +VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. > >> > \end{description} > >> > > >> > +\subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device Initialization} > >> > + > >> > +The device indicates the guest physical address to the driver in one of two ways: > >> > \begin{enumerate} > >> > -\item Driver vpmem start is read from \field{start}. > >> > -\item Driver vpmem end is read from \field{size}. > >> > +\item As a guest absolute address, using virtio_pmem_config. > >> > +\item As a shared memory region. > >> > \end{enumerate} > >> > > >> > -\subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization} > >> > - > >> > The driver determines the start address and size of the persistent memory region in preparation for reading or writing data. > >> > > >> > The driver initializes req_vq in preparation for making flush requests. > >> > > >> > +\devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization} > >> > + > >> > +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 > > [for the sake of the argument made below, note the 'has not been > negotiated' here] > > >> > +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. > >> > >> Sorry for joining in this late. > >> > >> I'm wondering if the quoted parts implies that the device SHOULD change > >> its config space (fields start and size) when the driver sets > >> FEATURES_OK, and the feature is VIRTIO_PMEM_F_SHMEM_REGION set at the > >> device (it was offered, and got set). > >> > >> My train of thought is: initially no feature is considered negotiated, > >> and config space access is allowed before feature negotiation is > >> completed. That means the at this stage the device must indicate via > >> the config space, and thus \field{size} != 0. But as a part of the > >> transition 'features not negotiated' -> 'features negotiated' the device > >> needs to go '\field{size} != 0' -> '\field{size} == 0' > > It has been advised that it SHOULD put 0 there, but I don't consider > that a must, just a good idea. Right I agree with that. So we say that the config state update is a good idea as well? BTW the definition of SHOULD: SHOULD This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course. Maybe I should have used 'should go' instead of 'needs to go' but that doesn't change the fact, that the spec currently regards this config space update as RECOMMENDED. Conny, is that what we want? > > >> > >> Is that what we want? > >> > >> Also I understand that Hyper-V would make the device cease operation if > >> the feature VIRTIO_PMEM_F_SHMEM_REGION was not accepted by the driver. > >> I believe I've read that in some previous email. > >> > >> Do we need a 'MAY fail to operate further if' statement? Anything that > >> ain't guarded by feature bits ain't optional, and anything that is > >> guarded by feature bits is optional unless explicitly stated otherwise. > > The device can always fail the setting of FEATURES_OK, see 2.2.2: > > "The device SHOULD accept any valid subset of features the driver > accepts, otherwise it MUST fail to set the FEATURES_OK device status bit > when the driver writes it." > > I'd argue that "SHMEM_REGION not set" can be considered a non-valid > subset by the device. (Also, there's a SHOULD in there.) Yes, but that is not very helpful for somebody reading the spec without having the context of this discussion. To put it differently, why do we use the "MAY fail to operate further if" statement for: * VIRTIO_F_ACCESS_PLATFORM not accepted * VIRTIO_F_ORDER_PLATFORM not accepted * VIRTIO_F_VERSION_1 not offered If "SHMEM_REGION not accepted" is covered by 2.2.2 and thus superfluous, then so is "ACCESS_PLATFORM not accepted". IMHO in the sentence quoted by you "valid subset" should be interpreted as a subset that is closed to the "depends on" relationship between feature sets. I.e. a subset is invalid, if it contains at least one feature such that some feature required by it is not in the subset. If we say that any device implementation can proclaim any subset of features invalid, than the the first part of the sentence does not have any meaning to it. You are definitively right about the SHOULD, it is not a MUST. I believe the whole idea behind features is to facilitate extensibility and compatibility. Usually a driver can safely ignore features it does not know or care about,and the device SHOULD still work according to the expectations of the driver. But some feature bits are not like that. For example it ain't safe for the driver to ignore ACCESS_PLATFORM, for obvious reasons. And we express that with the "MAY fail to operate further if" statement. > > >> > >> Regards, > >> Halil > > > > No problem, thanks for joining in! :) I think your train of thought makes sense. > > You're right, for Hyper-V we can't support any driver that doesn't support > > VIRTIO_PMEM_F_SHMEM_REGION. So we have no choice but to set the config fields > > to zero from the start. There's no meaningful value we can place in those fields. > > I think this is fine: we only require the fields to have meaningful > contents if SHMEM_REGION has not been negotiated -- as long as the > device has not accepted FEATURES_OK from the driver, nothing has been > negotiated respectively not been negotiated. I have issues accepting the tristate nature of 'has been negotiated'. I don't think that is intuitive. If this really what we want we should be very clear about it. In my opinion feature negotiation is usually an opt-in. Before the opt-in the device is to give us the "compatible" or "old" behavior. > > > > > I'm trying to figure out how best to word this. Saying that devices should set > > config fields prior to FEATURE_OK is redundant. Should I add a section saying that > > config fields MAY be set to 0 if VIRTIO_PMEM_F_SHMEM_REGION is offerred (such as > > in the Hyper-V case)? And that if it's set to 0 and the feature isn't negotiated, > > the device must reject the driver? I'd also have to add in a section saying that > > the pmem region shouldn't be touched by the driver prior to DRIVER_OK being set, > > to save drivers from accessing the zeroed values placed in the config. Additionally, > > encouraging devices to set the config fields to 0 once the feature has been > > negotiated seems like the right thing to do, to catch bugs? > > I don't think we can prohibit the driver from reading the config space, > that is explicitly allowed in the general case. I agree. I believe the specification should tell us which features are essential by design, and which features are optional by design. We clearly have both kinds of features, right? > > We probably can add a non-normative extra statement, though, that a > device may present zeroes in the config space before feature negotiation > has finished if it doesn't support !SHMEM_REGION. I would not tie that to 'before feature negotiation' or 'does not support SHMEM'. We can say something like: If the value of the field \filed{size} is 0, then the \field{start} does not contain the physical address of the persistent memory region, and the persistent memory region is advertised by means different than via the config space. > That would also give > the driver a way to discover that it doesn't make sense to continue > without negotiating SHMEM_REGION. No extra normative statement, but > should make it a bit more clear to those trying to implement this. > > > > > I'm not sure about the 'MAY fail to operate further statement'. Couldn't there > > be lots of reasons why a device rejects a driver? And wouldn't those be > > implementation details of each device? Is it common to document those? > > We probably don't need such a statement (see above). I disagree. > > As voting for this is open until tomorrow, we should quickly decide what > to do. My preference would be to merge it as-is and do a clarification > on top. > I'm fine with merging as is. > > 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/ > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [virtio-comment] [PATCH v5 1/1] virtio-pmem: Support describing pmem as shared memory region 2021-11-22 19:56 ` Halil Pasic @ 2021-11-23 11:10 ` Cornelia Huck 0 siblings, 0 replies; 13+ messages in thread From: Cornelia Huck @ 2021-11-23 11:10 UTC (permalink / raw) To: Halil Pasic Cc: Taylor Stark, virtio-comment, grahamwo, benhill, tstark, pankaj.gupta.linux, Halil Pasic On Mon, Nov 22 2021, Halil Pasic <pasic@linux.ibm.com> wrote: > On Mon, 22 Nov 2021 12:58:01 +0100 > Cornelia Huck <cohuck@redhat.com> wrote: > >> On Sun, Nov 21 2021, Taylor Stark <tstark@linux.microsoft.com> wrote: >> >> > On Thu, Nov 18, 2021 at 06:37:52PM +0100, Halil Pasic wrote: >> >> On Wed, 10 Nov 2021 10:55:55 -0800 >> >> tstark@linux.microsoft.com wrote: >> >> [..] >> >> > @@ -36,22 +39,45 @@ \subsection{Device configuration layout}\label{sec:Device Types / PMEM Device / >> >> > \end{lstlisting} >> >> > >> >> > \begin{description} >> >> > -\item[\field{start}] contains the physical address of the first byte of the persistent memory region. >> >> > +\item[\field{start}] contains the physical address of the first byte of the >> >> > +persistent memory region, if VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. >> >> > >> >> > -\item[\field{size}] contains the length of this address range. >> >> > +\item[\field{size}] contains the length of this address range, if >> >> > +VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated. >> >> > \end{description} >> >> > >> >> > +\subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device Initialization} >> >> > + >> >> > +The device indicates the guest physical address to the driver in one of two ways: >> >> > \begin{enumerate} >> >> > -\item Driver vpmem start is read from \field{start}. >> >> > -\item Driver vpmem end is read from \field{size}. >> >> > +\item As a guest absolute address, using virtio_pmem_config. >> >> > +\item As a shared memory region. >> >> > \end{enumerate} >> >> > >> >> > -\subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization} >> >> > - >> >> > The driver determines the start address and size of the persistent memory region in preparation for reading or writing data. >> >> > >> >> > The driver initializes req_vq in preparation for making flush requests. >> >> > >> >> > +\devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization} >> >> > + >> >> > +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 >> >> [for the sake of the argument made below, note the 'has not been >> negotiated' here] >> >> >> > +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. >> >> >> >> Sorry for joining in this late. >> >> >> >> I'm wondering if the quoted parts implies that the device SHOULD change >> >> its config space (fields start and size) when the driver sets >> >> FEATURES_OK, and the feature is VIRTIO_PMEM_F_SHMEM_REGION set at the >> >> device (it was offered, and got set). >> >> >> >> My train of thought is: initially no feature is considered negotiated, >> >> and config space access is allowed before feature negotiation is >> >> completed. That means the at this stage the device must indicate via >> >> the config space, and thus \field{size} != 0. But as a part of the >> >> transition 'features not negotiated' -> 'features negotiated' the device >> >> needs to go '\field{size} != 0' -> '\field{size} == 0' >> >> It has been advised that it SHOULD put 0 there, but I don't consider >> that a must, just a good idea. > > Right I agree with that. So we say that the config state update is a > good idea as well? > > BTW the definition of SHOULD: > SHOULD This word, or the adjective "RECOMMENDED", mean that there > may exist valid reasons in particular circumstances to ignore a > particular item, but the full implications must be understood and > carefully weighed before choosing a different course. > > Maybe I should have used 'should go' instead of 'needs to go' but that > doesn't change the fact, that the spec currently regards this > config space update as RECOMMENDED. > > Conny, is that what we want? > I really don't understand your problem with this? Yes, we recommend that the device does this, but if there's anything that makes it difficult for the device, it can omit it. > >> >> >> >> >> Is that what we want? >> >> >> >> Also I understand that Hyper-V would make the device cease operation if >> >> the feature VIRTIO_PMEM_F_SHMEM_REGION was not accepted by the driver. >> >> I believe I've read that in some previous email. >> >> >> >> Do we need a 'MAY fail to operate further if' statement? Anything that >> >> ain't guarded by feature bits ain't optional, and anything that is >> >> guarded by feature bits is optional unless explicitly stated otherwise. >> >> The device can always fail the setting of FEATURES_OK, see 2.2.2: >> >> "The device SHOULD accept any valid subset of features the driver >> accepts, otherwise it MUST fail to set the FEATURES_OK device status bit >> when the driver writes it." >> >> I'd argue that "SHMEM_REGION not set" can be considered a non-valid >> subset by the device. (Also, there's a SHOULD in there.) > > Yes, but that is not very helpful for somebody reading the spec without > having the context of this discussion. > > To put it differently, why do we use the "MAY fail to operate further > if" statement for: > * VIRTIO_F_ACCESS_PLATFORM not accepted > * VIRTIO_F_ORDER_PLATFORM not accepted > * VIRTIO_F_VERSION_1 not offered > > If "SHMEM_REGION not accepted" is covered by 2.2.2 and thus superfluous, > then so is "ACCESS_PLATFORM not accepted". I'd say it is indeed not really needed. The driver must be able to deal with the device rejecting FEATURES_OK for a certain feature set. > > IMHO in the sentence quoted by you "valid subset" should be interpreted > as a subset that is closed to the "depends on" relationship between > feature sets. I.e. a subset is invalid, if it contains at least one > feature such that some feature required by it is not in the subset. > > If we say that any device implementation can proclaim any subset of > features invalid, than the the first part of the sentence does > not have any meaning to it. > > You are definitively right about the SHOULD, it is not a MUST. > > I believe the whole idea behind features is to facilitate extensibility > and compatibility. > > Usually a driver can safely ignore features it does not know or care > about,and the device SHOULD still work according to the expectations of > the driver. > > But some feature bits are not like that. For example it ain't safe for > the driver to ignore ACCESS_PLATFORM, for obvious reasons. And we > express that with the "MAY fail to operate further if" statement. Well if you want to highlight a bit, highlight it. But I don't think this is really needed here. Things like ACCESS_PLATFORM are not as obvious. > > >> >> >> >> >> Regards, >> >> Halil >> > >> > No problem, thanks for joining in! :) I think your train of thought makes sense. >> > You're right, for Hyper-V we can't support any driver that doesn't support >> > VIRTIO_PMEM_F_SHMEM_REGION. So we have no choice but to set the config fields >> > to zero from the start. There's no meaningful value we can place in those fields. >> >> I think this is fine: we only require the fields to have meaningful >> contents if SHMEM_REGION has not been negotiated -- as long as the >> device has not accepted FEATURES_OK from the driver, nothing has been >> negotiated respectively not been negotiated. > > I have issues accepting the tristate nature of 'has been negotiated'. I > don't think that is intuitive. If this really what we want we should be > very clear about it. > > In my opinion feature negotiation is usually an opt-in. Before the opt-in > the device is to give us the "compatible" or "old" behavior. But a device is always free to require the "new" behaviour; otherwise, we could never ditch old interfaces. > >> >> > >> > I'm trying to figure out how best to word this. Saying that devices should set >> > config fields prior to FEATURE_OK is redundant. Should I add a section saying that >> > config fields MAY be set to 0 if VIRTIO_PMEM_F_SHMEM_REGION is offerred (such as >> > in the Hyper-V case)? And that if it's set to 0 and the feature isn't negotiated, >> > the device must reject the driver? I'd also have to add in a section saying that >> > the pmem region shouldn't be touched by the driver prior to DRIVER_OK being set, >> > to save drivers from accessing the zeroed values placed in the config. Additionally, >> > encouraging devices to set the config fields to 0 once the feature has been >> > negotiated seems like the right thing to do, to catch bugs? >> >> I don't think we can prohibit the driver from reading the config space, >> that is explicitly allowed in the general case. > > I agree. I believe the specification should tell us which features are > essential by design, and which features are optional by design. We > clearly have both kinds of features, right? It also depends on the individual device which features are essential. > >> >> We probably can add a non-normative extra statement, though, that a >> device may present zeroes in the config space before feature negotiation >> has finished if it doesn't support !SHMEM_REGION. > > I would not tie that to 'before feature negotiation' or 'does not support > SHMEM'. We can say something like: > > If the value of the field \filed{size} is 0, then the \field{start} does > not contain the physical address of the persistent memory region, and > the persistent memory region is advertised by means different than > via the config space. Isn't that self-evident? If the device does not provide the information in the config space, it uses a different mechanism. > >> That would also give >> the driver a way to discover that it doesn't make sense to continue >> without negotiating SHMEM_REGION. No extra normative statement, but >> should make it a bit more clear to those trying to implement this. >> >> > >> > I'm not sure about the 'MAY fail to operate further statement'. Couldn't there >> > be lots of reasons why a device rejects a driver? And wouldn't those be >> > implementation details of each device? Is it common to document those? >> >> We probably don't need such a statement (see above). > > I disagree. I still stand by my opinion. > >> >> As voting for this is open until tomorrow, we should quickly decide what >> to do. My preference would be to merge it as-is and do a clarification >> on top. >> > > I'm fine with merging as is. OK; if everyone agrees we need changes on top, we can add them later. 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/ ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-11-24 15:36 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-10 18:55 [virtio-comment] [PATCH v5 0/1] virtio-pmem: Support describing pmem as shared memory region tstark 2021-11-10 18:55 ` [virtio-comment] [PATCH v5 1/1] " tstark 2021-11-11 10:52 ` Cornelia Huck 2021-11-11 18:17 ` Taylor Stark 2021-11-23 16:42 ` Stefan Hajnoczi 2021-11-23 16:49 ` Cornelia Huck 2021-11-24 15:36 ` Stefan Hajnoczi 2021-11-16 0:31 ` [virtio-comment] " Pankaj Gupta 2021-11-18 17:37 ` [virtio-comment] " Halil Pasic 2021-11-21 8:08 ` Taylor Stark 2021-11-22 11:58 ` Cornelia Huck 2021-11-22 19:56 ` Halil Pasic 2021-11-23 11:10 ` Cornelia Huck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.