[virtio-dev] [PATCH] split-ring: Demand that a device must not change descriptor entries

[virtio-dev] [PATCH] split-ring: Demand that a device must not change descriptor entries

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

So far the spec only indirectly says that a descriptor table entry is
not modified by a device when processing it. Make this explicit by
adding it as normative requirement. Existing drivers already depend on
this.

See also https://lists.oasis-open.org/archives/virtio-dev/201910/msg00057.html.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 split-ring.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/split-ring.tex b/split-ring.tex
index 123ac9f..bfef62d 100644
--- a/split-ring.tex
+++ b/split-ring.tex
@@ -217,7 +217,7 @@ \subsection{The Virtqueue Descriptor Table}\label{sec:Basic Facilities of a Virt
 \devicenormative{\subsubsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
 A device MUST NOT write to a device-readable buffer, and a device SHOULD NOT
 read a device-writable buffer (it MAY do so for debugging or diagnostic
-purposes).
+purposes). A device MUST NOT write to any descriptor table entry.
 
 \drivernormative{\subsubsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
 Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes in total;
-- 
2.16.4

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH] split-ring: Demand that a device must not change descriptor entries

[Rob Miller]

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

what is trying to be solved here? There is a reason why this is allowed as
some vendors update the table when using RX_MERABLE_BUFFERs & F_IN_ORDER
features

Rob Miller
rob.miller@broadcom.com
(919)721-3339


On Mon, Nov 11, 2019 at 12:13 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> So far the spec only indirectly says that a descriptor table entry is
> not modified by a device when processing it. Make this explicit by
> adding it as normative requirement. Existing drivers already depend on
> this.
>
> See also
> https://lists.oasis-open.org/archives/virtio-dev/201910/msg00057.html.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  split-ring.tex | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/split-ring.tex b/split-ring.tex
> index 123ac9f..bfef62d 100644
> --- a/split-ring.tex
> +++ b/split-ring.tex
> @@ -217,7 +217,7 @@ \subsection{The Virtqueue Descriptor
> Table}\label{sec:Basic Facilities of a Virt
>  \devicenormative{\subsubsection}{The Virtqueue Descriptor Table}{Basic
> Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
>  A device MUST NOT write to a device-readable buffer, and a device SHOULD
> NOT
>  read a device-writable buffer (it MAY do so for debugging or diagnostic
> -purposes).
> +purposes). A device MUST NOT write to any descriptor table entry.
>
>  \drivernormative{\subsubsection}{The Virtqueue Descriptor Table}{Basic
> Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
>  Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes in
> total;
> --
> 2.16.4
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
>

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

Re: [virtio-dev] [PATCH] split-ring: Demand that a device must not change descriptor entries

[Jan Kiszka]

On 11.11.19 18:17, Rob Miller wrote:
> what is trying to be solved here? There is a reason why this is allowed 
> as some vendors update the table when using RX_MERABLE_BUFFERs & 
> F_IN_ORDER features

OK, good to know. Which fields are updated then? Under which conditions 
(ie. in which states of the device)?

The background is in the link below. My goal was to find a simple way 
declaring all fields read-only for the device to avoid problems with new 
implementations. But it looks like it's more complicated...

Jan

> 
> Rob Miller
> rob.miller@broadcom.com <mailto:rob.miller@broadcom.com>
> (919)721-3339
> 
> 
> On Mon, Nov 11, 2019 at 12:13 PM Jan Kiszka <jan.kiszka@siemens.com 
> <mailto:jan.kiszka@siemens.com>> wrote:
> 
>     From: Jan Kiszka <jan.kiszka@siemens.com
>     <mailto:jan.kiszka@siemens.com>>
> 
>     So far the spec only indirectly says that a descriptor table entry is
>     not modified by a device when processing it. Make this explicit by
>     adding it as normative requirement. Existing drivers already depend on
>     this.
> 
>     See also
>     https://lists.oasis-open.org/archives/virtio-dev/201910/msg00057.html.
> 
>     Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com
>     <mailto:jan.kiszka@siemens.com>>
>     ---
>       split-ring.tex | 2 +-
>       1 file changed, 1 insertion(+), 1 deletion(-)
> 
>     diff --git a/split-ring.tex b/split-ring.tex
>     index 123ac9f..bfef62d 100644
>     --- a/split-ring.tex
>     +++ b/split-ring.tex
>     @@ -217,7 +217,7 @@ \subsection{The Virtqueue Descriptor
>     Table}\label{sec:Basic Facilities of a Virt
>       \devicenormative{\subsubsection}{The Virtqueue Descriptor
>     Table}{Basic Facilities of a Virtio Device / Virtqueues / The
>     Virtqueue Descriptor Table}
>       A device MUST NOT write to a device-readable buffer, and a device
>     SHOULD NOT
>       read a device-writable buffer (it MAY do so for debugging or
>     diagnostic
>     -purposes).
>     +purposes). A device MUST NOT write to any descriptor table entry.
> 
>       \drivernormative{\subsubsection}{The Virtqueue Descriptor
>     Table}{Basic Facilities of a Virtio Device / Virtqueues / The
>     Virtqueue Descriptor Table}
>       Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes
>     in total;
>     -- 
>     2.16.4
> 
>     ---------------------------------------------------------------------
>     To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>     <mailto:virtio-dev-unsubscribe@lists.oasis-open.org>
>     For additional commands, e-mail:
>     virtio-dev-help@lists.oasis-open.org
>     <mailto:virtio-dev-help@lists.oasis-open.org>
> 

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH] split-ring: Demand that a device must not change descriptor entries

[Michael S. Tsirkin]

On Mon, Nov 11, 2019 at 12:17:51PM -0500, Rob Miller wrote:
> On Mon, Nov 11, 2019 at 12:13 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>     From: Jan Kiszka <jan.kiszka@siemens.com>
> 
>     So far the spec only indirectly says that a descriptor table entry is
>     not modified by a device when processing it. Make this explicit by
>     adding it as normative requirement. Existing drivers already depend on
>     this.
> 
>     See also https://lists.oasis-open.org/archives/virtio-dev/201910/
>     msg00057.html.
> 
>     Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>     ---
>      split-ring.tex | 2 +-
>      1 file changed, 1 insertion(+), 1 deletion(-)
> 
>     diff --git a/split-ring.tex b/split-ring.tex
>     index 123ac9f..bfef62d 100644
>     --- a/split-ring.tex
>     +++ b/split-ring.tex
>     @@ -217,7 +217,7 @@ \subsection{The Virtqueue Descriptor Table}\label
>     {sec:Basic Facilities of a Virt
>      \devicenormative{\subsubsection}{The Virtqueue Descriptor Table}{Basic
>     Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor
>     Table}
>      A device MUST NOT write to a device-readable buffer, and a device SHOULD
>     NOT
>      read a device-writable buffer (it MAY do so for debugging or diagnostic
>     -purposes).
>     +purposes). A device MUST NOT write to any descriptor table entry.
> 
>      \drivernormative{\subsubsection}{The Virtqueue Descriptor Table}{Basic
>     Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor
>     Table}
>      Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes in
>     total;
>     --
>     2.16.4
> 
> 
> what is trying to be solved here? There is a reason why this is allowed as some
> vendors update the table when using RX_MERABLE_BUFFERs & F_IN_ORDER features
> 
> Rob Miller
> rob.miller@broadcom.com
> (919)721-3339
> 
> 


Going back to this, I don't see how this can work with current Linux
guests at least when VIRTIO_F_IOMMU_PLATFORM is specified, since
the data is mapped to devices as follows:

                        dma_addr_t addr = vring_map_one_sg(vq, sg, DMA_TO_DEVICE);

and I guess we all agreed physical devices all set this flag?
Could you explain a bit more about how writing into descriptors
is useful with RX_MERABLE_BUFFERs?


-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH] split-ring: Demand that a device must not change descriptor entries

[Michael S. Tsirkin]

On Mon, Nov 11, 2019 at 06:26:58PM +0100, Jan Kiszka wrote:
> On 11.11.19 18:17, Rob Miller wrote:
> > what is trying to be solved here? There is a reason why this is allowed
> > as some vendors update the table when using RX_MERABLE_BUFFERs &
> > F_IN_ORDER features
> 
> OK, good to know. Which fields are updated then? Under which conditions (ie.
> in which states of the device)?
> 
> The background is in the link below. My goal was to find a simple way
> declaring all fields read-only for the device to avoid problems with new
> implementations. But it looks like it's more complicated...
> 
> Jan

No answer yet. Rob I see the following options:
- proceed with the ballot as is ignoring your comment. Maybe it's all a
  misunderstanding? It could be the comment was referring to the
  packed ring, where this patch refers to the split ring.
- make it weaker: MUST if PLATFORM_ACCESS is negotiated, and
  SHOULD if it isn't.

> > 
> > Rob Miller
> > rob.miller@broadcom.com <mailto:rob.miller@broadcom.com>
> > (919)721-3339
> > 
> > 
> > On Mon, Nov 11, 2019 at 12:13 PM Jan Kiszka <jan.kiszka@siemens.com
> > <mailto:jan.kiszka@siemens.com>> wrote:
> > 
> >     From: Jan Kiszka <jan.kiszka@siemens.com
> >     <mailto:jan.kiszka@siemens.com>>
> > 
> >     So far the spec only indirectly says that a descriptor table entry is
> >     not modified by a device when processing it. Make this explicit by
> >     adding it as normative requirement. Existing drivers already depend on
> >     this.
> > 
> >     See also
> >     https://lists.oasis-open.org/archives/virtio-dev/201910/msg00057.html.
> > 
> >     Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com
> >     <mailto:jan.kiszka@siemens.com>>
> >     ---
> >       split-ring.tex | 2 +-
> >       1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> >     diff --git a/split-ring.tex b/split-ring.tex
> >     index 123ac9f..bfef62d 100644
> >     --- a/split-ring.tex
> >     +++ b/split-ring.tex
> >     @@ -217,7 +217,7 @@ \subsection{The Virtqueue Descriptor
> >     Table}\label{sec:Basic Facilities of a Virt
> >       \devicenormative{\subsubsection}{The Virtqueue Descriptor
> >     Table}{Basic Facilities of a Virtio Device / Virtqueues / The
> >     Virtqueue Descriptor Table}
> >       A device MUST NOT write to a device-readable buffer, and a device
> >     SHOULD NOT
> >       read a device-writable buffer (it MAY do so for debugging or
> >     diagnostic
> >     -purposes).
> >     +purposes). A device MUST NOT write to any descriptor table entry.
> > 
> >       \drivernormative{\subsubsection}{The Virtqueue Descriptor
> >     Table}{Basic Facilities of a Virtio Device / Virtqueues / The
> >     Virtqueue Descriptor Table}
> >       Drivers MUST NOT add a descriptor chain longer than $2^{32}$ bytes
> >     in total;
> >     --     2.16.4
> > 
> >     ---------------------------------------------------------------------
> >     To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> >     <mailto:virtio-dev-unsubscribe@lists.oasis-open.org>
> >     For additional commands, e-mail:
> >     virtio-dev-help@lists.oasis-open.org
> >     <mailto:virtio-dev-help@lists.oasis-open.org>
> > 
> 
> -- 
> Siemens AG, Corporate Technology, CT RDA IOT SES-DE
> Corporate Competence Center Embedded Linux
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH] split-ring: Demand that a device must not change descriptor entries

[Rob Miller]

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

In reviewing my comments, I agree that I misunderstood what was being
requested. Please ignore my comments and continue as needed.

Rob Miller
rob.miller@broadcom.com
(919)721-3339


On Thu, Feb 27, 2020 at 5:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:

> On Mon, Nov 11, 2019 at 06:26:58PM +0100, Jan Kiszka wrote:
> > On 11.11.19 18:17, Rob Miller wrote:
> > > what is trying to be solved here? There is a reason why this is allowed
> > > as some vendors update the table when using RX_MERABLE_BUFFERs &
> > > F_IN_ORDER features
> >
> > OK, good to know. Which fields are updated then? Under which conditions
> (ie.
> > in which states of the device)?
> >
> > The background is in the link below. My goal was to find a simple way
> > declaring all fields read-only for the device to avoid problems with new
> > implementations. But it looks like it's more complicated...
> >
> > Jan
>
> No answer yet. Rob I see the following options:
> - proceed with the ballot as is ignoring your comment. Maybe it's all a
>   misunderstanding? It could be the comment was referring to the
>   packed ring, where this patch refers to the split ring.
> - make it weaker: MUST if PLATFORM_ACCESS is negotiated, and
>   SHOULD if it isn't.
>
> > >
> > > Rob Miller
> > > rob.miller@broadcom.com <mailto:rob.miller@broadcom.com>
> > > (919)721-3339
> > >
> > >
> > > On Mon, Nov 11, 2019 at 12:13 PM Jan Kiszka <jan.kiszka@siemens.com
> > > <mailto:jan.kiszka@siemens.com>> wrote:
> > >
> > >     From: Jan Kiszka <jan.kiszka@siemens.com
> > >     <mailto:jan.kiszka@siemens.com>>
> > >
> > >     So far the spec only indirectly says that a descriptor table entry
> is
> > >     not modified by a device when processing it. Make this explicit by
> > >     adding it as normative requirement. Existing drivers already
> depend on
> > >     this.
> > >
> > >     See also
> > >
> https://lists.oasis-open.org/archives/virtio-dev/201910/msg00057.html.
> > >
> > >     Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com
> > >     <mailto:jan.kiszka@siemens.com>>
> > >     ---
> > >       split-ring.tex | 2 +-
> > >       1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > >     diff --git a/split-ring.tex b/split-ring.tex
> > >     index 123ac9f..bfef62d 100644
> > >     --- a/split-ring.tex
> > >     +++ b/split-ring.tex
> > >     @@ -217,7 +217,7 @@ \subsection{The Virtqueue Descriptor
> > >     Table}\label{sec:Basic Facilities of a Virt
> > >       \devicenormative{\subsubsection}{The Virtqueue Descriptor
> > >     Table}{Basic Facilities of a Virtio Device / Virtqueues / The
> > >     Virtqueue Descriptor Table}
> > >       A device MUST NOT write to a device-readable buffer, and a device
> > >     SHOULD NOT
> > >       read a device-writable buffer (it MAY do so for debugging or
> > >     diagnostic
> > >     -purposes).
> > >     +purposes). A device MUST NOT write to any descriptor table entry.
> > >
> > >       \drivernormative{\subsubsection}{The Virtqueue Descriptor
> > >     Table}{Basic Facilities of a Virtio Device / Virtqueues / The
> > >     Virtqueue Descriptor Table}
> > >       Drivers MUST NOT add a descriptor chain longer than $2^{32}$
> bytes
> > >     in total;
> > >     --     2.16.4
> > >
> > >
>  ---------------------------------------------------------------------
> > >     To unsubscribe, e-mail:
> virtio-dev-unsubscribe@lists.oasis-open.org
> > >     <mailto:virtio-dev-unsubscribe@lists.oasis-open.org>
> > >     For additional commands, e-mail:
> > >     virtio-dev-help@lists.oasis-open.org
> > >     <mailto:virtio-dev-help@lists.oasis-open.org>
> > >
> >
> > --
> > Siemens AG, Corporate Technology, CT RDA IOT SES-DE
> > Corporate Competence Center Embedded Linux
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
>

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

Re: [virtio-dev] [PATCH] split-ring: Demand that a device must not change descriptor entries

[Michael S. Tsirkin]

On Mon, Mar 02, 2020 at 07:01:37AM -0500, Rob Miller wrote:
> In reviewing my comments, I agree that I misunderstood what was being
> requested. Please ignore my comments and continue as needed.

OK then. Jan should we start the vote?


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH] split-ring: Demand that a device must not change descriptor entries

[Jan Kiszka]

On 02.03.20 13:13, Michael S. Tsirkin wrote:
> On Mon, Mar 02, 2020 at 07:01:37AM -0500, Rob Miller wrote:
>> In reviewing my comments, I agree that I misunderstood what was being
>> requested. Please ignore my comments and continue as needed.
> 
> OK then. Jan should we start the vote?
> 

Fine with me, yes.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org