OASIS Virtual I/O Device (VIRTIO) TC

All those clauses actually apply whenever the feature is negotiated, not merely offered. Rename to clarify things. Signed-off-by: Cornelia Huck <cohuck@redhat.com> --- virtio-iommu.tex 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/virtio-iommu.tex b/virtio-iommu.tex index f8cbe8895f0d..efe1000ac2d2 100644 --- a/virtio-iommu.tex +++ b/virtio-iommu.tex @@ -232,22 +232,22 @@ subsection{Device operations}label{sec:Device Types / IOMMU Device / Device op Creating mappings aligned on large page sizes can improve performance since they require fewer page table and TLB entries. -item If the VIRTIO_IOMMU_F_DOMAIN_RANGE feature is offered, +item If the VIRTIO_IOMMU_F_DOMAIN_RANGE feature is negotiated, field{domain_range} describes the values supported in a field{domain} - field. If the feature is not offered, any field{domain} value is valid. + field. If the feature is not negotiated, any field{domain} value is valid. -item If the VIRTIO_IOMMU_F_INPUT_RANGE feature is offered, +item If the VIRTIO_IOMMU_F_INPUT_RANGE feature is negotiated, field{input_range} contains the virtual address range that the IOMMU is able to translate. Any mapping request to virtual addresses outside of this range fails. - If the feature is not offered, virtual mappings span over the whole + If the feature is not negotiated, virtual mappings span over the whole 64-bit address space ( exttt{start = 0, end = 0xffffffff ffffffff}) end{itemize} An endpoint is in bypass mode if: egin{itemize} - item the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is offered and: + item the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is negotiated and: egin{itemize} item config field field{bypass} is 1 and the endpoint is not attached to a domain. This applies even if the driver -- 2.34.1

On Tue, Apr 12 2022, Cornelia Huck <cohuck@redhat.com> wrote:

> All those clauses actually apply whenever the feature is negotiated,
> not merely offered. Rename to clarify things.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> virtio-iommu.tex | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)

Pushed as editorial update.

On Tue, Apr 12 2022, Cornelia Huck <cohuck@redhat.com> wrote: > All those clauses actually apply whenever the feature is negotiated, > not merely offered. Rename to clarify things. > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > virtio-iommu.tex 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) Pushed as editorial update.

On Tue, 12 Apr 2022 10:30:59 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> All those clauses actually apply whenever the feature is negotiated,

Not sure "only apply" or "also apply". If it is also apply, then
we are actually making a change that imposes further requirements
on the driver.

> not merely offered. Rename to clarify things.

I'm not sure this is a mere clarification. I'm pretty sure, to
me it was not clear that 'offered' actually means 'negotiated'
in this context.

>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> virtio-iommu.tex | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/virtio-iommu.tex b/virtio-iommu.tex
> index f8cbe8895f0d..efe1000ac2d2 100644
> --- a/virtio-iommu.tex
> +++ b/virtio-iommu.tex
> @@ -232,22 +232,22 @@ \subsection{Device operations}\label{sec:Device Types / IOMMU Device / Device op
> Creating mappings aligned on large page sizes can improve performance
> since they require fewer page table and TLB entries.
>
> -\item If the VIRTIO_IOMMU_F_DOMAIN_RANGE feature is offered,
> +\item If the VIRTIO_IOMMU_F_DOMAIN_RANGE feature is negotiated,

Here 'offered' might actually make sense. With offered, doing
an early config space access on domain_range is permissible, if not
then the driver should not read domain_range before features are fully
negotiated.

One could implement the device in a way, that when it offers the feature,
it just presents the value.

> \field{domain_range} describes the values supported in a \field{domain}
> - field. If the feature is not offered, any \field{domain} value is valid.
> + field. If the feature is not negotiated, any \field{domain} value is valid.

Let's be very careful here! The new version does not make sense to me
while the old one does. Here is why:

Whether the feature F_DOMAIN_RANGE is offered or not is under the control
of the device. So it is completely sane to say: if the device supports
any value it I guess can decide to not offer the feature, or to write
le32_min and le_32_max into the corresponding fields. BTW are those
fields really signed?

But the interesting case is, what if the device does not support
arbitrary values, for domain, but let us say just the values 0, 1, 2, 3?

With the new version the driver can just decide to not negotiate
F_DOMAIN_RANGE, and then because the feature is not negotiated, any
domain value is valid according to the above. The only way out of that
is, to say that the device should fail feature negotiation if the
feature was offered, but not accepted, and there are invalid domain
values.

What I'm trying to say is: the old version was more straight forward to
me, and if we do insist on sticking to the new version, we should
probably spell out that the device is expected to fail feature
negotiation if this feature is not accepted by the driver.

>
> -\item If the VIRTIO_IOMMU_F_INPUT_RANGE feature is offered,
> +\item If the VIRTIO_IOMMU_F_INPUT_RANGE feature is negotiated,

This one is similar I guess.

> \field{input_range} contains the virtual address range that the IOMMU is
> able to translate. Any mapping request to virtual addresses outside of
> this range fails.
>
> - If the feature is not offered, virtual mappings span over the whole
> + If the feature is not negotiated, virtual mappings span over the whole
> 64-bit address space (\texttt{start = 0, end = 0xffffffff ffffffff})
> \end{itemize}
>
> An endpoint is in bypass mode if:
> \begin{itemize}
> - \item the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is offered and:
> + \item the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is negotiated and:

Again makes no sense...

> \begin{itemize}
> \item config field \field{bypass} is 1 and the endpoint is
> not attached to a domain. This applies even if the driver

... the rest of the sentence goes "does not accept the
VIRTIO_IOMMU_F_BYPASS_CONFIG feature". If the feature was negotiated,
then the device has offered it and the driver has accepted it by
definition!

Regards,
Halil

On Tue, 12 Apr 2022 10:30:59 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > All those clauses actually apply whenever the feature is negotiated, Not sure "only apply" or "also apply". If it is also apply, then we are actually making a change that imposes further requirements on the driver. > not merely offered. Rename to clarify things. I'm not sure this is a mere clarification. I'm pretty sure, to me it was not clear that 'offered' actually means 'negotiated' in this context. > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > virtio-iommu.tex 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/virtio-iommu.tex b/virtio-iommu.tex > index f8cbe8895f0d..efe1000ac2d2 100644 > --- a/virtio-iommu.tex > +++ b/virtio-iommu.tex > @@ -232,22 +232,22 @@ subsection{Device operations}label{sec:Device Types / IOMMU Device / Device op > Creating mappings aligned on large page sizes can improve performance > since they require fewer page table and TLB entries. > > -item If the VIRTIO_IOMMU_F_DOMAIN_RANGE feature is offered, > +item If the VIRTIO_IOMMU_F_DOMAIN_RANGE feature is negotiated, Here 'offered' might actually make sense. With offered, doing an early config space access on domain_range is permissible, if not then the driver should not read domain_range before features are fully negotiated. One could implement the device in a way, that when it offers the feature, it just presents the value. > field{domain_range} describes the values supported in a field{domain} > - field. If the feature is not offered, any field{domain} value is valid. > + field. If the feature is not negotiated, any field{domain} value is valid. Let's be very careful here! The new version does not make sense to me while the old one does. Here is why: Whether the feature F_DOMAIN_RANGE is offered or not is under the control of the device. So it is completely sane to say: if the device supports any value it I guess can decide to not offer the feature, or to write le32_min and le_32_max into the corresponding fields. BTW are those fields really signed? But the interesting case is, what if the device does not support arbitrary values, for domain, but let us say just the values 0, 1, 2, 3? With the new version the driver can just decide to not negotiate F_DOMAIN_RANGE, and then because the feature is not negotiated, any domain value is valid according to the above. The only way out of that is, to say that the device should fail feature negotiation if the feature was offered, but not accepted, and there are invalid domain values. What I'm trying to say is: the old version was more straight forward to me, and if we do insist on sticking to the new version, we should probably spell out that the device is expected to fail feature negotiation if this feature is not accepted by the driver. > > -item If the VIRTIO_IOMMU_F_INPUT_RANGE feature is offered, > +item If the VIRTIO_IOMMU_F_INPUT_RANGE feature is negotiated, This one is similar I guess. > field{input_range} contains the virtual address range that the IOMMU is > able to translate. Any mapping request to virtual addresses outside of > this range fails. > > - If the feature is not offered, virtual mappings span over the whole > + If the feature is not negotiated, virtual mappings span over the whole > 64-bit address space ( exttt{start = 0, end = 0xffffffff ffffffff}) > end{itemize} > > An endpoint is in bypass mode if: > egin{itemize} > - item the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is offered and: > + item the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is negotiated and: Again makes no sense... > egin{itemize} > item config field field{bypass} is 1 and the endpoint is > not attached to a domain. This applies even if the driver ... the rest of the sentence goes "does not accept the VIRTIO_IOMMU_F_BYPASS_CONFIG feature". If the feature was negotiated, then the device has offered it and the driver has accepted it by definition! Regards, Halil

Hello,

I consider those changes not as clarifications but as requirements
changes which will cause software to be changed.

On 12.04.22 10:30, Cornelia Huck wrote:
> All those clauses actually apply whenever the feature is negotiated,
> not merely offered. Rename to clarify things.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> virtio-iommu.tex | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/virtio-iommu.tex b/virtio-iommu.tex
> index f8cbe8895f0d..efe1000ac2d2 100644
> --- a/virtio-iommu.tex
> +++ b/virtio-iommu.tex
> @@ -232,22 +232,22 @@ \subsection{Device operations}\label{sec:Device Types / IOMMU Device / Device op
> Creating mappings aligned on large page sizes can improve performance
> since they require fewer page table and TLB entries.
>
> -\item If the VIRTIO_IOMMU_F_DOMAIN_RANGE feature is offered,
> +\item If the VIRTIO_IOMMU_F_DOMAIN_RANGE feature is negotiated,
> \field{domain_range} describes the values supported in a \field{domain}
> - field. If the feature is not offered, any \field{domain} value is valid.
> + field. If the feature is not negotiated, any \field{domain} value is valid.
>
> -\item If the VIRTIO_IOMMU_F_INPUT_RANGE feature is offered,
> +\item If the VIRTIO_IOMMU_F_INPUT_RANGE feature is negotiated,
> \field{input_range} contains the virtual address range that the IOMMU is
> able to translate. Any mapping request to virtual addresses outside of
> this range fails.
>
> - If the feature is not offered, virtual mappings span over the whole
> + If the feature is not negotiated, virtual mappings span over the whole
> 64-bit address space (\texttt{start = 0, end = 0xffffffff ffffffff})
> \end{itemize}

Assume the following situation: Due to internal implementation there is
a device which can only work when the domain range from the driver is in
a certain value range (internal implementation: Array of N domains
indexed from 0 to N-1). And then there is a driver which behaves in a
compliant manner but has no idea about the feature
VIRTIO_IOMMU_F_DOMAIN_RANGE so does not negotiate it. With "negotiated"
the driver the device has to fail feature negotiation as it cannot
accept all possible domain values during live operation. With "offered"
it can succeed feature negotiation as it reserves the right to fail
later in the ATTACH/DETACH/MAP/UNMAP REQUEST with error
VIRTIO_IOMMU_S_RANGE when a domain value is received it cannot handle
internally.

Exactly the same applies for VIRTIO_IOMMU_F_INPUT_RANGE.

The implementation I'm doing currently cannot work with all possible
virtual address value ranges due to efficiency reasons. With the changed
text the implementation should also be changed now to fail feature
negotiation if the driver does not offer VIRTIO_IOMMU_F_INPUT_RANGE. As
the implementation can currently handle all possible domain values (not
in the future for efficiency reasons when the implementation is getting
optimized) I can now think about failing the feature negotiation if
VIRTIO_IOMMU_F_DOMAIN_RANGE was not negotiated or to remove the checks
for the allowed value range already implemented.

No big issue (for me). But causes a change in the implementation and may
therefore not be an editorial change only. The original wording may not
have been a mistake from the author. It may have been there for an
(unknown) purpose. Some old driver known to behave compliant to the
device implementation but having no idea about those features?

Regards
Harald

--
Dipl.-Ing. Harald Mommer
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin

Phone: +49 (30) 60 98 540-0 <== Zentrale
Fax: +49 (30) 60 98 540-99
E-Mail: harald.mommer@opensynergy.com

www.opensynergy.com

Handelsregister: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Regis Adjamah

On Tue, Apr 12 2022, Cornelia Huck <cohuck@redhat.com> wrote:

Hmm... it seems the original proposal had been too buried (or I actually
fat-fingered things in my patch).

What do we do now? Revert, and re-apply a new patch after perhaps some
further discussion? (Would likely be for 1.3, though. Maybe also vote,
as this does not seem to be as straightforward as I thought.)

> All those clauses actually apply whenever the feature is negotiated,
> not merely offered. Rename to clarify things.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> virtio-iommu.tex | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/virtio-iommu.tex b/virtio-iommu.tex
> index f8cbe8895f0d..efe1000ac2d2 100644
> --- a/virtio-iommu.tex
> +++ b/virtio-iommu.tex
> @@ -232,22 +232,22 @@ \subsection{Device operations}\label{sec:Device Types / IOMMU Device / Device op
> Creating mappings aligned on large page sizes can improve performance
> since they require fewer page table and TLB entries.
>
> -\item If the VIRTIO_IOMMU_F_DOMAIN_RANGE feature is offered,
> +\item If the VIRTIO_IOMMU_F_DOMAIN_RANGE feature is negotiated,
> \field{domain_range} describes the values supported in a \field{domain}
> - field. If the feature is not offered, any \field{domain} value is valid.
> + field. If the feature is not negotiated, any \field{domain} value is valid.
>
> -\item If the VIRTIO_IOMMU_F_INPUT_RANGE feature is offered,
> +\item If the VIRTIO_IOMMU_F_INPUT_RANGE feature is negotiated,
> \field{input_range} contains the virtual address range that the IOMMU is
> able to translate. Any mapping request to virtual addresses outside of
> this range fails.
>
> - If the feature is not offered, virtual mappings span over the whole
> + If the feature is not negotiated, virtual mappings span over the whole
> 64-bit address space (\texttt{start = 0, end = 0xffffffff ffffffff})
> \end{itemize}
>
> An endpoint is in bypass mode if:
> \begin{itemize}
> - \item the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is offered and:
> + \item the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is negotiated and:
> \begin{itemize}
> \item config field \field{bypass} is 1 and the endpoint is
> not attached to a domain. This applies even if the driver
> --
> 2.34.1

On Tue, Apr 12 2022, Cornelia Huck <cohuck@redhat.com> wrote: Hmm... it seems the original proposal had been too buried (or I actually fat-fingered things in my patch). What do we do now? Revert, and re-apply a new patch after perhaps some further discussion? (Would likely be for 1.3, though. Maybe also vote, as this does not seem to be as straightforward as I thought.) > All those clauses actually apply whenever the feature is negotiated, > not merely offered. Rename to clarify things. > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > virtio-iommu.tex 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/virtio-iommu.tex b/virtio-iommu.tex > index f8cbe8895f0d..efe1000ac2d2 100644 > --- a/virtio-iommu.tex > +++ b/virtio-iommu.tex > @@ -232,22 +232,22 @@ subsection{Device operations}label{sec:Device Types / IOMMU Device / Device op > Creating mappings aligned on large page sizes can improve performance > since they require fewer page table and TLB entries. > > -item If the VIRTIO_IOMMU_F_DOMAIN_RANGE feature is offered, > +item If the VIRTIO_IOMMU_F_DOMAIN_RANGE feature is negotiated, > field{domain_range} describes the values supported in a field{domain} > - field. If the feature is not offered, any field{domain} value is valid. > + field. If the feature is not negotiated, any field{domain} value is valid. > > -item If the VIRTIO_IOMMU_F_INPUT_RANGE feature is offered, > +item If the VIRTIO_IOMMU_F_INPUT_RANGE feature is negotiated, > field{input_range} contains the virtual address range that the IOMMU is > able to translate. Any mapping request to virtual addresses outside of > this range fails. > > - If the feature is not offered, virtual mappings span over the whole > + If the feature is not negotiated, virtual mappings span over the whole > 64-bit address space ( exttt{start = 0, end = 0xffffffff ffffffff}) > end{itemize} > > An endpoint is in bypass mode if: > egin{itemize} > - item the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is offered and: > + item the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is negotiated and: > egin{itemize} > item config field field{bypass} is 1 and the endpoint is > not attached to a domain. This applies even if the driver > -- > 2.34.1

On Thu, 14 Apr 2022 11:11:06 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> What do we do now? Revert, and re-apply a new patch after perhaps some
> further discussion? (Would likely be for 1.3, though. Maybe also vote,
> as this does not seem to be as straightforward as I thought.)

Sounds good to me. I'm not sure about the re-apply part, because I'm
not sure this patch is salvageable.

Regards,
Halil

On Thu, 14 Apr 2022 11:11:06 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > What do we do now? Revert, and re-apply a new patch after perhaps some > further discussion? (Would likely be for 1.3, though. Maybe also vote, > as this does not seem to be as straightforward as I thought.) Sounds good to me. I'm not sure about the re-apply part, because I'm not sure this patch is salvageable. Regards, Halil

On Thu, Apr 14 2022, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Thu, 14 Apr 2022 11:11:06 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> What do we do now? Revert, and re-apply a new patch after perhaps some
>> further discussion? (Would likely be for 1.3, though. Maybe also vote,
>> as this does not seem to be as straightforward as I thought.)
>
> Sounds good to me. I'm not sure about the re-apply part, because I'm
> not sure this patch is salvageable.

Reverted. Let's continue discussing this in an 1.3 context.

On Thu, Apr 14 2022, Halil Pasic <pasic@linux.ibm.com> wrote: > On Thu, 14 Apr 2022 11:11:06 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > >> What do we do now? Revert, and re-apply a new patch after perhaps some >> further discussion? (Would likely be for 1.3, though. Maybe also vote, >> as this does not seem to be as straightforward as I thought.) > > Sounds good to me. I'm not sure about the re-apply part, because I'm > not sure this patch is salvageable. Reverted. Let's continue discussing this in an 1.3 context.