[PATCH] iommu: offered -> negotiated

[PATCH] iommu: offered -> negotiated

[Cornelia Huck]

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

[virtio-comment] Re: [PATCH] iommu: offered -> negotiated

[Cornelia Huck]

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.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio] [PATCH] iommu: offered -> negotiated

[Halil Pasic]

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

Re: [virtio-comment] [PATCH] iommu: offered -> negotiated

[Harald Mommer]

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


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio] Re: [PATCH] iommu: offered -> negotiated

[Cornelia Huck]

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


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [virtio] Re: [PATCH] iommu: offered -> negotiated

[Halil Pasic]

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

Re: [virtio] Re: [PATCH] iommu: offered -> negotiated

[Cornelia Huck]

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.


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php