[PATCH] virtio-iommu: Rework the bypass feature

[PATCH] virtio-iommu: Rework the bypass feature

[Jean-Philippe Brucker]

The VIRTIO_IOMMU_F_BYPASS feature is awkward to use and incomplete.
Although it is implemented by QEMU, it is not supported by any driver as
far as I know. Replace it with a new VIRTIO_IOMMU_F_BYPASS_CONFIG
feature. The old feature bit is deprecated and will be recycled once
versions of QEMU that implement it are not distributed anymore.

Two features are missing from virtio-iommu:

* The ability for an hypervisor to start the device in bypass mode. The
  wording for VIRTIO_IOMMU_F_BYPASS is not clear enough to allow it at
  the moment.

* The ability for a guest to set individual endpoints in bypass mode
  when bypass is globally disabled. An OS should have the ability to
  allow only endpoints it trusts to bypass the IOMMU, while keeping DMA
  disabled for endpoints it isn't even aware of. At the moment this can
  only be emulated by creating identity mappings.

The VIRTIO_IOMMU_F_BYPASS_CONFIG feature adds a 'bypass' config field
that allows to enable and disable bypass globally. It also adds a new
flag for the ATTACH request.

* The hypervisor can start the VM with bypass enabled or, if it knows
  that the software stack supports it, disabled. The 'bypass' config
  fields resets to 0 or 1.

* Generally the firmware won't have an IOMMU driver and will need to be
  started in bypass mode, so the bootloader and kernel can be loaded
  from storage endpoint.

  For more security, the firmware could implement a minimal virtio-iommu
  driver that reuses existing virtio support and only touches the config
  space. It could enable PCI bus mastering in bridges only for the
  endpoints that need it, enable global IOMMU bypass by flipping a bit,
  then tear everything down before handing control over to the OS. This
  prevents vulnerability windows where a malicious endpoint reprograms
  the IOMMU while the OS is configuring it [1].

  The isolation provided by vIOMMUs has mainly been used for securely
  assigning endpoints to untrusted applications so far, while kernel DMA
  bypasses the IOMMU. But we can expect boot security to become as
  important in virtualization as it presently is on bare-metal systems,
  where some devices are untrusted and must never be able to access
  memory that wasn't assigned to them.

* The OS can enable and disable bypass globally. It can then enable
  bypass for individual endpoints by attaching them to bypass domains,
  using the new VIRTIO_IOMMU_ATTACH_F_BYPASS flag. It can disable bypass
  by attaching them to normal domains.

[1] IOMMU protection against I/O attacks: a vulnerability and a proof of concept
    Morgan, B., Alata, É., Nicomette, V. et al.
    https://link.springer.com/article/10.1186/s13173-017-0066-7

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
The virtio-iommu spec with colored diff is available at
https://jpbrucker.net/virtio-iommu/spec-bypass/virtio-iommu-f-bypass-config-v1-diff.pdf

Apologies for the poorly thought out VIRTIO_IOMMU_F_BYPASS. I didn't
spend enough time on it and ignored valuable suggestions.
---
 virtio-iommu.tex | 69 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 53 insertions(+), 16 deletions(-)

diff --git a/virtio-iommu.tex b/virtio-iommu.tex
index efa735b..a2c90ea 100644
--- a/virtio-iommu.tex
+++ b/virtio-iommu.tex
@@ -59,14 +59,19 @@ \subsection{Feature bits}\label{sec:Device Types / IOMMU Device / Feature bits}
   VIRTIO_IOMMU_F_MAP_UNMAP is supported.}
 
 \item[VIRTIO_IOMMU_F_BYPASS (3)]
-  When not attached to a domain, endpoints downstream of the IOMMU
-  can access the guest-physical address space.
+  This feature is deprecated.
 
 \item[VIRTIO_IOMMU_F_PROBE (4)]
   The PROBE request is available.
 
 \item[VIRTIO_IOMMU_F_MMIO (5)]
   The VIRTIO_IOMMU_MAP_F_MMIO flag is available.
+
+\item[VIRTIO_IOMMU_F_BYPASS_CONFIG (6)]
+  The global bypass state is described in \field{bypass} of
+  struct virtio_iommu_config. The domain bypass state is
+  described by VIRTIO_IOMMU_ATTACH_F_BYPASS.
+
 \end{description}
 
 \drivernormative{\subsubsection}{Feature bits}{Device Types / IOMMU Device / Feature bits}
@@ -97,12 +102,19 @@ \subsection{Device configuration layout}\label{sec:Device Types / IOMMU Device /
     le32 end;
   } domain_range;
   le32 probe_size;
+  u8 bypass;
+  u8 reserved[3];
 };
 \end{lstlisting}
 
 \drivernormative{\subsubsection}{Device configuration layout}{Device Types / IOMMU Device / Device configuration layout}
 
-The driver MUST NOT write to device configuration fields.
+When the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is negotiated, the
+driver MAY write to \field{bypass}. The driver MUST NOT write to
+any other device configuration field.
+
+If field \field{bypass} contains a value different than 0 or 1,
+the driver SHOULD treat it as 0.
 
 \devicenormative{\subsubsection}{Device configuration layout}{Device Types / IOMMU Device / Device configuration layout}
 
@@ -110,16 +122,24 @@ \subsection{Device configuration layout}\label{sec:Device Types / IOMMU Device /
 the page granularity. The device MAY set more than one bit in
 \field{page_size_mask}.
 
+If the driver writes a value different than 0 or 1 in
+\field{bypass}, the device SHOULD treat it as 0.
+
 \subsection{Device initialization}\label{sec:Device Types / IOMMU Device / Device initialization}
 
 When the device is reset, endpoints are not attached to any domain.
 
-If the VIRTIO_IOMMU_F_BYPASS feature is negotiated, all accesses from
-unattached endpoints are allowed and translated by the IOMMU using the
-identity function. If the feature is not negotiated, any memory access
-from an unattached endpoint fails. Upon attaching an endpoint in
-bypass mode to a new domain, any memory access from the endpoint fails,
-since the domain does not contain any mapping.
+An endpoint is in bypass mode if the VIRTIO_IOMMU_F_BYPASS_CONFIG
+feature is offered and:
+\begin{itemize}
+  \item config field \field{bypass} is 1 and the endpoint is not
+    attached to a domain, or
+  \item the endpoint is attached to a domain with
+    VIRTIO_IOMMU_ATTACH_F_BYPASS.
+\end{itemize}
+
+All accesses from an endpoint in bypass mode are allowed and
+translated by the IOMMU using the identity function.
 
 Future devices might support more modes of operation besides MAP/UNMAP.
 Drivers verify that devices set VIRTIO_IOMMU_F_MAP_UNMAP and fail
@@ -136,8 +156,8 @@ \subsection{Device initialization}\label{sec:Device Types / IOMMU Device / Devic
 
 \devicenormative{\subsubsection}{Device Initialization}{Device Types / IOMMU Device / Device Initialization}
 
-If the driver does not accept the VIRTIO_IOMMU_F_BYPASS feature, the
-device SHOULD NOT let endpoints access the guest-physical address space.
+The device SHOULD NOT let unattached endpoints that are not in
+bypass mode access the guest-physical address space.
 
 \subsection{Device operations}\label{sec:Device Types / IOMMU Device / Device operations}
 
@@ -266,9 +286,12 @@ \subsubsection{ATTACH request}\label{sec:Device Types / IOMMU Device / Device op
   struct virtio_iommu_req_head head;
   le32 domain;
   le32 endpoint;
-  u8   reserved[8];
+  le32 flags;
+  u8   reserved[4];
   struct virtio_iommu_req_tail tail;
 };
+
+#define VIRTIO_IOMMU_ATTACH_F_BYPASS  (1 << 0)
 \end{lstlisting}
 
 Attach an endpoint to a domain. \field{domain} uniquely identifies a
@@ -294,6 +317,10 @@ \subsubsection{ATTACH request}\label{sec:Device Types / IOMMU Device / Device op
 attached to a single domain at a time. Endpoints attached to different
 domains are isolated from each other.
 
+When the VIRTIO_IOMMU_F_BYPASS_CONFIG is negotiated, the driver
+can set the VIRTIO_IOMMU_ATTACH_F_BYPASS flag to create a bypass
+domain. Endpoints attached to this domain are in bypass mode.
+
 \drivernormative{\paragraph}{ATTACH request}{Device Types / IOMMU Device / Device operations / ATTACH request}
 
 The driver SHOULD set \field{reserved} to zero.
@@ -301,11 +328,20 @@ \subsubsection{ATTACH request}\label{sec:Device Types / IOMMU Device / Device op
 The driver SHOULD ensure that endpoints that cannot be isolated from each
 other are attached to the same domain.
 
+If the domain already exists and is a bypass domain, the driver
+SHOULD set the VIRTIO_IOMMU_ATTACH_F_BYPASS flag. If the domain
+exists and is not a bypass domain, the driver SHOULD NOT set the
+VIRTIO_IOMMU_ATTACH_F_BYPASS flag.
+
 \devicenormative{\paragraph}{ATTACH request}{Device Types / IOMMU Device / Device operations / ATTACH request}
 
 If the \field{reserved} field of an ATTACH request is not zero, the device
 MUST reject the request and set \field{status} to VIRTIO_IOMMU_S_INVAL.
 
+If the device does not recognize a \field{flags} bit, it MUST
+reject the request and set \field{status} to
+VIRTIO_IOMMU_S_INVAL.
+
 If the endpoint identified by \field{endpoint} doesn't exist, the device
 MUST reject the request and set \field{status} to VIRTIO_IOMMU_S_NOENT.
 
@@ -343,10 +379,7 @@ \subsubsection{DETACH request}
 \end{lstlisting}
 
 Detach an endpoint from a domain. When this request completes, the
-endpoint cannot access any mapping from that domain anymore. If feature
-VIRTIO_IOMMU_F_BYPASS has been negotiated, then once this request
-completes all accesses from the endpoint are allowed and translated by the
-IOMMU using the identity function.
+endpoint cannot access any mapping from that domain anymore.
 
 After all endpoints have been successfully detached from a domain, it
 ceases to exist and its ID can be reused by the driver for another domain.
@@ -433,6 +466,8 @@ \subsubsection{MAP request}\label{sec:Device Types / IOMMU Device / Device opera
 
 The driver SHOULD set undefined \field{flags} bits to zero.
 
+The driver SHOULD NOT send MAP requests on a bypass domain.
+
 \field{virt_end} MUST be strictly greater than \field{virt_start}.
 
 The driver SHOULD set the VIRTIO_IOMMU_MAP_F_MMIO flag when the physical
@@ -538,6 +573,8 @@ \subsubsection{UNMAP request}\label{sec:Device Types / IOMMU Device / Device ope
 or be outside any mapping. The last address of a range MUST either be the
 last address of a mapping or be outside any mapping.
 
+The driver SHOULD NOT send UNMAP requests on a bypass domain.
+
 \devicenormative{\paragraph}{UNMAP request}{Device Types / IOMMU Device / Device operations / UNMAP request}
 
 If the \field{reserved} field of an UNMAP request is not zero, the device
-- 
2.33.0

[virtio-dev] Re: [PATCH] virtio-iommu: Rework the bypass feature

[Cornelia Huck]

On Thu, Sep 30 2021, Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:

> The VIRTIO_IOMMU_F_BYPASS feature is awkward to use and incomplete.
> Although it is implemented by QEMU, it is not supported by any driver as
> far as I know. Replace it with a new VIRTIO_IOMMU_F_BYPASS_CONFIG
> feature. The old feature bit is deprecated and will be recycled once
> versions of QEMU that implement it are not distributed anymore.

I'm not sure we can safely reuse old feature bits. I don't think we're
running out of free feature bits, so just keep this as reserved and
deprecated?

>
> Two features are missing from virtio-iommu:
>
> * The ability for an hypervisor to start the device in bypass mode. The
>   wording for VIRTIO_IOMMU_F_BYPASS is not clear enough to allow it at
>   the moment.
>
> * The ability for a guest to set individual endpoints in bypass mode
>   when bypass is globally disabled. An OS should have the ability to
>   allow only endpoints it trusts to bypass the IOMMU, while keeping DMA
>   disabled for endpoints it isn't even aware of. At the moment this can
>   only be emulated by creating identity mappings.
>
> The VIRTIO_IOMMU_F_BYPASS_CONFIG feature adds a 'bypass' config field
> that allows to enable and disable bypass globally. It also adds a new
> flag for the ATTACH request.
>
> * The hypervisor can start the VM with bypass enabled or, if it knows
>   that the software stack supports it, disabled. The 'bypass' config
>   fields resets to 0 or 1.
>
> * Generally the firmware won't have an IOMMU driver and will need to be
>   started in bypass mode, so the bootloader and kernel can be loaded
>   from storage endpoint.
>
>   For more security, the firmware could implement a minimal virtio-iommu
>   driver that reuses existing virtio support and only touches the config
>   space. It could enable PCI bus mastering in bridges only for the
>   endpoints that need it, enable global IOMMU bypass by flipping a bit,
>   then tear everything down before handing control over to the OS. This
>   prevents vulnerability windows where a malicious endpoint reprograms
>   the IOMMU while the OS is configuring it [1].
>
>   The isolation provided by vIOMMUs has mainly been used for securely
>   assigning endpoints to untrusted applications so far, while kernel DMA
>   bypasses the IOMMU. But we can expect boot security to become as
>   important in virtualization as it presently is on bare-metal systems,
>   where some devices are untrusted and must never be able to access
>   memory that wasn't assigned to them.
>
> * The OS can enable and disable bypass globally. It can then enable
>   bypass for individual endpoints by attaching them to bypass domains,
>   using the new VIRTIO_IOMMU_ATTACH_F_BYPASS flag. It can disable bypass
>   by attaching them to normal domains.
>
> [1] IOMMU protection against I/O attacks: a vulnerability and a proof of concept
>     Morgan, B., Alata, É., Nicomette, V. et al.
>     https://link.springer.com/article/10.1186/s13173-017-0066-7

I'm not really familiar with this topic, so I'll just point out some
spec things.

>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> The virtio-iommu spec with colored diff is available at
> https://jpbrucker.net/virtio-iommu/spec-bypass/virtio-iommu-f-bypass-config-v1-diff.pdf
>
> Apologies for the poorly thought out VIRTIO_IOMMU_F_BYPASS. I didn't
> spend enough time on it and ignored valuable suggestions.
> ---
>  virtio-iommu.tex | 69 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 53 insertions(+), 16 deletions(-)
>
> diff --git a/virtio-iommu.tex b/virtio-iommu.tex
> index efa735b..a2c90ea 100644
> --- a/virtio-iommu.tex
> +++ b/virtio-iommu.tex
> @@ -59,14 +59,19 @@ \subsection{Feature bits}\label{sec:Device Types / IOMMU Device / Feature bits}
>    VIRTIO_IOMMU_F_MAP_UNMAP is supported.}
>  
>  \item[VIRTIO_IOMMU_F_BYPASS (3)]
> -  When not attached to a domain, endpoints downstream of the IOMMU
> -  can access the guest-physical address space.
> +  This feature is deprecated.

"and must not be negotiated." ?

Not sure if we should add normative statements for that.

>  
>  \item[VIRTIO_IOMMU_F_PROBE (4)]
>    The PROBE request is available.
>  
>  \item[VIRTIO_IOMMU_F_MMIO (5)]
>    The VIRTIO_IOMMU_MAP_F_MMIO flag is available.
> +
> +\item[VIRTIO_IOMMU_F_BYPASS_CONFIG (6)]
> +  The global bypass state is described in \field{bypass} of
> +  struct virtio_iommu_config. The domain bypass state is
> +  described by VIRTIO_IOMMU_ATTACH_F_BYPASS.
> +
>  \end{description}
>  
>  \drivernormative{\subsubsection}{Feature bits}{Device Types / IOMMU Device / Feature bits}
> @@ -97,12 +102,19 @@ \subsection{Device configuration layout}\label{sec:Device Types / IOMMU Device /
>      le32 end;
>    } domain_range;
>    le32 probe_size;
> +  u8 bypass;
> +  u8 reserved[3];
>  };
>  \end{lstlisting}
>  
>  \drivernormative{\subsubsection}{Device configuration layout}{Device Types / IOMMU Device / Device configuration layout}
>  
> -The driver MUST NOT write to device configuration fields.
> +When the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is negotiated, the
> +driver MAY write to \field{bypass}. The driver MUST NOT write to
> +any other device configuration field.
> +
> +If field \field{bypass} contains a value different than 0 or 1,
> +the driver SHOULD treat it as 0.

"The driver MUST NOT write any value different than 0 or 1 to
\field{bypass}." ?

>  
>  \devicenormative{\subsubsection}{Device configuration layout}{Device Types / IOMMU Device / Device configuration layout}
>  
> @@ -110,16 +122,24 @@ \subsection{Device configuration layout}\label{sec:Device Types / IOMMU Device /
>  the page granularity. The device MAY set more than one bit in
>  \field{page_size_mask}.
>  
> +If the driver writes a value different than 0 or 1 in
> +\field{bypass}, the device SHOULD treat it as 0.

"The device MUST NOT present any value different than 0 or 1 in
\field{bypass}." ?


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

Re: [PATCH] virtio-iommu: Rework the bypass feature

[Michael S. Tsirkin]

On Tue, Oct 05, 2021 at 06:57:50PM +0200, Cornelia Huck wrote:
> On Thu, Sep 30 2021, Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> 
> > The VIRTIO_IOMMU_F_BYPASS feature is awkward to use and incomplete.
> > Although it is implemented by QEMU, it is not supported by any driver as
> > far as I know. Replace it with a new VIRTIO_IOMMU_F_BYPASS_CONFIG
> > feature. The old feature bit is deprecated and will be recycled once
> > versions of QEMU that implement it are not distributed anymore.
> 
> I'm not sure we can safely reuse old feature bits. I don't think we're
> running out of free feature bits, so just keep this as reserved and
> deprecated?
> 
> >
> > Two features are missing from virtio-iommu:
> >
> > * The ability for an hypervisor to start the device in bypass mode. The
> >   wording for VIRTIO_IOMMU_F_BYPASS is not clear enough to allow it at
> >   the moment.
> >
> > * The ability for a guest to set individual endpoints in bypass mode
> >   when bypass is globally disabled. An OS should have the ability to
> >   allow only endpoints it trusts to bypass the IOMMU, while keeping DMA
> >   disabled for endpoints it isn't even aware of. At the moment this can
> >   only be emulated by creating identity mappings.
> >
> > The VIRTIO_IOMMU_F_BYPASS_CONFIG feature adds a 'bypass' config field
> > that allows to enable and disable bypass globally. It also adds a new
> > flag for the ATTACH request.
> >
> > * The hypervisor can start the VM with bypass enabled or, if it knows
> >   that the software stack supports it, disabled. The 'bypass' config
> >   fields resets to 0 or 1.
> >
> > * Generally the firmware won't have an IOMMU driver and will need to be
> >   started in bypass mode, so the bootloader and kernel can be loaded
> >   from storage endpoint.
> >
> >   For more security, the firmware could implement a minimal virtio-iommu
> >   driver that reuses existing virtio support and only touches the config
> >   space. It could enable PCI bus mastering in bridges only for the
> >   endpoints that need it, enable global IOMMU bypass by flipping a bit,
> >   then tear everything down before handing control over to the OS. This
> >   prevents vulnerability windows where a malicious endpoint reprograms
> >   the IOMMU while the OS is configuring it [1].
> >
> >   The isolation provided by vIOMMUs has mainly been used for securely
> >   assigning endpoints to untrusted applications so far, while kernel DMA
> >   bypasses the IOMMU. But we can expect boot security to become as
> >   important in virtualization as it presently is on bare-metal systems,
> >   where some devices are untrusted and must never be able to access
> >   memory that wasn't assigned to them.
> >
> > * The OS can enable and disable bypass globally. It can then enable
> >   bypass for individual endpoints by attaching them to bypass domains,
> >   using the new VIRTIO_IOMMU_ATTACH_F_BYPASS flag. It can disable bypass
> >   by attaching them to normal domains.
> >
> > [1] IOMMU protection against I/O attacks: a vulnerability and a proof of concept
> >     Morgan, B., Alata, É., Nicomette, V. et al.
> >     https://link.springer.com/article/10.1186/s13173-017-0066-7
> 
> I'm not really familiar with this topic, so I'll just point out some
> spec things.
> 
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> > The virtio-iommu spec with colored diff is available at
> > https://jpbrucker.net/virtio-iommu/spec-bypass/virtio-iommu-f-bypass-config-v1-diff.pdf
> >
> > Apologies for the poorly thought out VIRTIO_IOMMU_F_BYPASS. I didn't
> > spend enough time on it and ignored valuable suggestions.
> > ---
> >  virtio-iommu.tex | 69 +++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 53 insertions(+), 16 deletions(-)
> >
> > diff --git a/virtio-iommu.tex b/virtio-iommu.tex
> > index efa735b..a2c90ea 100644
> > --- a/virtio-iommu.tex
> > +++ b/virtio-iommu.tex
> > @@ -59,14 +59,19 @@ \subsection{Feature bits}\label{sec:Device Types / IOMMU Device / Feature bits}
> >    VIRTIO_IOMMU_F_MAP_UNMAP is supported.}
> >  
> >  \item[VIRTIO_IOMMU_F_BYPASS (3)]
> > -  When not attached to a domain, endpoints downstream of the IOMMU
> > -  can access the guest-physical address space.
> > +  This feature is deprecated.
> 
> "and must not be negotiated." ?
> 
> Not sure if we should add normative statements for that.

We can't just make existing art de jure illegal.

So SHOULD NOT and yes needs confirmance statements.
Given documentation is already there why remove it?


> >  
> >  \item[VIRTIO_IOMMU_F_PROBE (4)]
> >    The PROBE request is available.
> >  
> >  \item[VIRTIO_IOMMU_F_MMIO (5)]
> >    The VIRTIO_IOMMU_MAP_F_MMIO flag is available.
> > +
> > +\item[VIRTIO_IOMMU_F_BYPASS_CONFIG (6)]
> > +  The global bypass state is described in \field{bypass} of
> > +  struct virtio_iommu_config. The domain bypass state is
> > +  described by VIRTIO_IOMMU_ATTACH_F_BYPASS.
> > +
> >  \end{description}
> >  
> >  \drivernormative{\subsubsection}{Feature bits}{Device Types / IOMMU Device / Feature bits}
> > @@ -97,12 +102,19 @@ \subsection{Device configuration layout}\label{sec:Device Types / IOMMU Device /
> >      le32 end;
> >    } domain_range;
> >    le32 probe_size;
> > +  u8 bypass;
> > +  u8 reserved[3];
> >  };
> >  \end{lstlisting}
> >  
> >  \drivernormative{\subsubsection}{Device configuration layout}{Device Types / IOMMU Device / Device configuration layout}
> >  
> > -The driver MUST NOT write to device configuration fields.
> > +When the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is negotiated, the
> > +driver MAY write to \field{bypass}. The driver MUST NOT write to
> > +any other device configuration field.
> > +
> > +If field \field{bypass} contains a value different than 0 or 1,
> > +the driver SHOULD treat it as 0.
> 
> "The driver MUST NOT write any value different than 0 or 1 to
> \field{bypass}." ?
> 
> >  
> >  \devicenormative{\subsubsection}{Device configuration layout}{Device Types / IOMMU Device / Device configuration layout}
> >  
> > @@ -110,16 +122,24 @@ \subsection{Device configuration layout}\label{sec:Device Types / IOMMU Device /
> >  the page granularity. The device MAY set more than one bit in
> >  \field{page_size_mask}.
> >  
> > +If the driver writes a value different than 0 or 1 in
> > +\field{bypass}, the device SHOULD treat it as 0.
> 
> "The device MUST NOT present any value different than 0 or 1 in
> \field{bypass}." ?

[virtio-dev] Re: [PATCH] virtio-iommu: Rework the bypass feature

[Cornelia Huck]

On Tue, Oct 05 2021, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Oct 05, 2021 at 06:57:50PM +0200, Cornelia Huck wrote:
>> On Thu, Sep 30 2021, Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
>> >  \item[VIRTIO_IOMMU_F_BYPASS (3)]
>> > -  When not attached to a domain, endpoints downstream of the IOMMU
>> > -  can access the guest-physical address space.
>> > +  This feature is deprecated.
>> 
>> "and must not be negotiated." ?
>> 
>> Not sure if we should add normative statements for that.
>
> We can't just make existing art de jure illegal.
>
> So SHOULD NOT and yes needs confirmance statements.
> Given documentation is already there why remove it?

Right, let's make that SHOULD NOT.


---------------------------------------------------------------------
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] virtio-iommu: Rework the bypass feature

[Eric Auger]

Hi Jean,

On 9/30/21 6:35 PM, Jean-Philippe Brucker wrote:
> The VIRTIO_IOMMU_F_BYPASS feature is awkward to use and incomplete.
> Although it is implemented by QEMU, it is not supported by any driver as
> far as I know. Replace it with a new VIRTIO_IOMMU_F_BYPASS_CONFIG
> feature. The old feature bit is deprecated and will be recycled once
> versions of QEMU that implement it are not distributed anymore.
>
> Two features are missing from virtio-iommu:
>
> * The ability for an hypervisor to start the device in bypass mode. The
>   wording for VIRTIO_IOMMU_F_BYPASS is not clear enough to allow it at
>   the moment.
>
> * The ability for a guest to set individual endpoints in bypass mode
>   when bypass is globally disabled. An OS should have the ability to
>   allow only endpoints it trusts to bypass the IOMMU, while keeping DMA
>   disabled for endpoints it isn't even aware of. At the moment this can
>   only be emulated by creating identity mappings.
>
> The VIRTIO_IOMMU_F_BYPASS_CONFIG feature adds a 'bypass' config field
> that allows to enable and disable bypass globally. It also adds a new
> flag for the ATTACH request.
>
> * The hypervisor can start the VM with bypass enabled or, if it knows
>   that the software stack supports it, disabled. The 'bypass' config
>   fields resets to 0 or 1.
I don't see the reset value documented in the actual spec while it looks
quite important for our boot bypass functionality. Or did I miss it?
> * Generally the firmware won't have an IOMMU driver and will need to be
>   started in bypass mode, so the bootloader and kernel can be loaded
>   from storage endpoint.
>
>   For more security, the firmware could implement a minimal virtio-iommu
>   driver that reuses existing virtio support and only touches the config
>   space. It could enable PCI bus mastering in bridges only for the
>   endpoints that need it, enable global IOMMU bypass by flipping a bit,
>   then tear everything down before handing control over to the OS. This
>   prevents vulnerability windows where a malicious endpoint reprograms
>   the IOMMU while the OS is configuring it [1].
>
>   The isolation provided by vIOMMUs has mainly been used for securely
>   assigning endpoints to untrusted applications so far, while kernel DMA
>   bypasses the IOMMU. But we can expect boot security to become as
>   important in virtualization as it presently is on bare-metal systems,
>   where some devices are untrusted and must never be able to access
>   memory that wasn't assigned to them.
>
> * The OS can enable and disable bypass globally. It can then enable
>   bypass for individual endpoints by attaching them to bypass domains,
>   using the new VIRTIO_IOMMU_ATTACH_F_BYPASS flag. It can disable bypass
>   by attaching them to normal domains.
>
> [1] IOMMU protection against I/O attacks: a vulnerability and a proof of concept
>     Morgan, B., Alata, É., Nicomette, V. et al.
>     https://link.springer.com/article/10.1186/s13173-017-0066-7
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> The virtio-iommu spec with colored diff is available at
> https://jpbrucker.net/virtio-iommu/spec-bypass/virtio-iommu-f-bypass-config-v1-diff.pdf
>
> Apologies for the poorly thought out VIRTIO_IOMMU_F_BYPASS. I didn't
> spend enough time on it and ignored valuable suggestions.
> ---
>  virtio-iommu.tex | 69 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 53 insertions(+), 16 deletions(-)
>
> diff --git a/virtio-iommu.tex b/virtio-iommu.tex
> index efa735b..a2c90ea 100644
> --- a/virtio-iommu.tex
> +++ b/virtio-iommu.tex
> @@ -59,14 +59,19 @@ \subsection{Feature bits}\label{sec:Device Types / IOMMU Device / Feature bits}
>    VIRTIO_IOMMU_F_MAP_UNMAP is supported.}
>  
>  \item[VIRTIO_IOMMU_F_BYPASS (3)]
> -  When not attached to a domain, endpoints downstream of the IOMMU
> -  can access the guest-physical address space.
> +  This feature is deprecated.
Why are you obliged to deprecate the feature. Can't you leave it and
impose that either

VIRTIO_IOMMU_F_BYPASS or VIRTIO_IOMMU_F_BYPASS_CONFIG gets set?

>  
>  \item[VIRTIO_IOMMU_F_PROBE (4)]
>    The PROBE request is available.
>  
>  \item[VIRTIO_IOMMU_F_MMIO (5)]
>    The VIRTIO_IOMMU_MAP_F_MMIO flag is available.
> +
> +\item[VIRTIO_IOMMU_F_BYPASS_CONFIG (6)]
> +  The global bypass state is described in \field{bypass} of
> +  struct virtio_iommu_config. The domain bypass state is
> +  described by VIRTIO_IOMMU_ATTACH_F_BYPASS.
I would re-emphasize the global bypass state relates to bypass state of
unattached devices whereas domain bypass state controls bypass for
devices attached to a given domain.
> +
>  \end{description}
>  
>  \drivernormative{\subsubsection}{Feature bits}{Device Types / IOMMU Device / Feature bits}
> @@ -97,12 +102,19 @@ \subsection{Device configuration layout}\label{sec:Device Types / IOMMU Device /
>      le32 end;
>    } domain_range;
>    le32 probe_size;
> +  u8 bypass;
> +  u8 reserved[3];
>  };
>  \end{lstlisting}
>  
>  \drivernormative{\subsubsection}{Device configuration layout}{Device Types / IOMMU Device / Device configuration layout}
>  
> -The driver MUST NOT write to device configuration fields.
> +When the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is negotiated, the
> +driver MAY write to \field{bypass}. The driver MUST NOT write to
> +any other device configuration field.
> +
> +If field \field{bypass} contains a value different than 0 or 1,
> +the driver SHOULD treat it as 0.
>  
>  \devicenormative{\subsubsection}{Device configuration layout}{Device Types / IOMMU Device / Device configuration layout}
>  
> @@ -110,16 +122,24 @@ \subsection{Device configuration layout}\label{sec:Device Types / IOMMU Device /
>  the page granularity. The device MAY set more than one bit in
>  \field{page_size_mask}.
>  
> +If the driver writes a value different than 0 or 1 in
> +\field{bypass}, the device SHOULD treat it as 0.
> +
>  \subsection{Device initialization}\label{sec:Device Types / IOMMU Device / Device initialization}
>  
>  When the device is reset, endpoints are not attached to any domain.
>  
> -If the VIRTIO_IOMMU_F_BYPASS feature is negotiated, all accesses from
> -unattached endpoints are allowed and translated by the IOMMU using the
> -identity function. If the feature is not negotiated, any memory access
> -from an unattached endpoint fails. Upon attaching an endpoint in
> -bypass mode to a new domain, any memory access from the endpoint fails,
> -since the domain does not contain any mapping.
> +An endpoint is in bypass mode if the VIRTIO_IOMMU_F_BYPASS_CONFIG
> +feature is offered and:
> +\begin{itemize}
> +  \item config field \field{bypass} is 1 and the endpoint is not
> +    attached to a domain, or
> +  \item the endpoint is attached to a domain with
> +    VIRTIO_IOMMU_ATTACH_F_BYPASS.
> +\end{itemize}
> +
> +All accesses from an endpoint in bypass mode are allowed and
> +translated by the IOMMU using the identity function.
>  
>  Future devices might support more modes of operation besides MAP/UNMAP.
>  Drivers verify that devices set VIRTIO_IOMMU_F_MAP_UNMAP and fail
> @@ -136,8 +156,8 @@ \subsection{Device initialization}\label{sec:Device Types / IOMMU Device / Devic
>  
>  \devicenormative{\subsubsection}{Device Initialization}{Device Types / IOMMU Device / Device Initialization}
>  
> -If the driver does not accept the VIRTIO_IOMMU_F_BYPASS feature, the
> -device SHOULD NOT let endpoints access the guest-physical address space.
> +The device SHOULD NOT let unattached endpoints that are not in
> +bypass mode access the guest-physical address space.
SHOULD NOT or MUST NOT. Can you remind me of the difference?
>  
>  \subsection{Device operations}\label{sec:Device Types / IOMMU Device / Device operations}
>  
> @@ -266,9 +286,12 @@ \subsubsection{ATTACH request}\label{sec:Device Types / IOMMU Device / Device op
>    struct virtio_iommu_req_head head;
>    le32 domain;
>    le32 endpoint;
> -  u8   reserved[8];
> +  le32 flags;
> +  u8   reserved[4];
>    struct virtio_iommu_req_tail tail;
>  };
> +
> +#define VIRTIO_IOMMU_ATTACH_F_BYPASS  (1 << 0)
>  \end{lstlisting}
>  
>  Attach an endpoint to a domain. \field{domain} uniquely identifies a
> @@ -294,6 +317,10 @@ \subsubsection{ATTACH request}\label{sec:Device Types / IOMMU Device / Device op
>  attached to a single domain at a time. Endpoints attached to different
>  domains are isolated from each other.
>  
> +When the VIRTIO_IOMMU_F_BYPASS_CONFIG is negotiated, the driver
> +can set the VIRTIO_IOMMU_ATTACH_F_BYPASS flag to create a bypass
> +domain. Endpoints attached to this domain are in bypass mode.
> +
>  \drivernormative{\paragraph}{ATTACH request}{Device Types / IOMMU Device / Device operations / ATTACH request}
>  
>  The driver SHOULD set \field{reserved} to zero.
> @@ -301,11 +328,20 @@ \subsubsection{ATTACH request}\label{sec:Device Types / IOMMU Device / Device op
>  The driver SHOULD ensure that endpoints that cannot be isolated from each
>  other are attached to the same domain.
>  
> +If the domain already exists and is a bypass domain, the driver
> +SHOULD set the VIRTIO_IOMMU_ATTACH_F_BYPASS flag. If the domain
> +exists and is not a bypass domain, the driver SHOULD NOT set the
> +VIRTIO_IOMMU_ATTACH_F_BYPASS flag.
> +
>  \devicenormative{\paragraph}{ATTACH request}{Device Types / IOMMU Device / Device operations / ATTACH request}
>  
>  If the \field{reserved} field of an ATTACH request is not zero, the device
>  MUST reject the request and set \field{status} to VIRTIO_IOMMU_S_INVAL.
>  
> +If the device does not recognize a \field{flags} bit, it MUST
> +reject the request and set \field{status} to
> +VIRTIO_IOMMU_S_INVAL.
> +
>  If the endpoint identified by \field{endpoint} doesn't exist, the device
>  MUST reject the request and set \field{status} to VIRTIO_IOMMU_S_NOENT.
>  
> @@ -343,10 +379,7 @@ \subsubsection{DETACH request}
>  \end{lstlisting}
>  
>  Detach an endpoint from a domain. When this request completes, the
> -endpoint cannot access any mapping from that domain anymore. If feature
> -VIRTIO_IOMMU_F_BYPASS has been negotiated, then once this request
> -completes all accesses from the endpoint are allowed and translated by the
> -IOMMU using the identity function.
> +endpoint cannot access any mapping from that domain anymore.
You could adapt the rest of the paragraph to the new

VIRTIO_IOMMU_F_BYPASS_CONFIG

>  
>  After all endpoints have been successfully detached from a domain, it
>  ceases to exist and its ID can be reused by the driver for another domain.
> @@ -433,6 +466,8 @@ \subsubsection{MAP request}\label{sec:Device Types / IOMMU Device / Device opera
>  
>  The driver SHOULD set undefined \field{flags} bits to zero.
>  
> +The driver SHOULD NOT send MAP requests on a bypass domain.
> +
>  \field{virt_end} MUST be strictly greater than \field{virt_start}.
>  
>  The driver SHOULD set the VIRTIO_IOMMU_MAP_F_MMIO flag when the physical
> @@ -538,6 +573,8 @@ \subsubsection{UNMAP request}\label{sec:Device Types / IOMMU Device / Device ope
>  or be outside any mapping. The last address of a range MUST either be the
>  last address of a mapping or be outside any mapping.
>  
> +The driver SHOULD NOT send UNMAP requests on a bypass domain.
> +
>  \devicenormative{\paragraph}{UNMAP request}{Device Types / IOMMU Device / Device operations / UNMAP request}
>  
>  If the \field{reserved} field of an UNMAP request is not zero, the device
Shouldn't we descrive the behavior of the device is MAP/UNMAP is called
on a bypass domain or if we try to attach another device to a domain
with a different ATTACH_F_BYPASS flag?

Thanks

Eric

Re: [virtio-dev] [PATCH] virtio-iommu: Rework the bypass feature

[Jean-Philippe Brucker]

Hi Eric,

On Wed, Oct 06, 2021 at 02:53:50PM +0200, Eric Auger wrote:
> > * The hypervisor can start the VM with bypass enabled or, if it knows
> >   that the software stack supports it, disabled. The 'bypass' config
> >   fields resets to 0 or 1.
> I don't see the reset value documented in the actual spec while it looks
> quite important for our boot bypass functionality. Or did I miss it?

No you're right, I'll add this. It's important to say that the device may
initialize the field to 1, but also that a reset (writing a 0 to device
status) doesn't re-initialize the field to its boot value (because that
may introduce a vulnerability during the FW->OS transition)

  If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
  initialize the \field{bypass} field to 1. When the driver resets the
  device, the \field{bypass} field SHOULD NOT change.

[...]
> > diff --git a/virtio-iommu.tex b/virtio-iommu.tex
> > index efa735b..a2c90ea 100644
> > --- a/virtio-iommu.tex
> > +++ b/virtio-iommu.tex
> > @@ -59,14 +59,19 @@ \subsection{Feature bits}\label{sec:Device Types / IOMMU Device / Feature bits}
> >    VIRTIO_IOMMU_F_MAP_UNMAP is supported.}
> >  
> >  \item[VIRTIO_IOMMU_F_BYPASS (3)]
> > -  When not attached to a domain, endpoints downstream of the IOMMU
> > -  can access the guest-physical address space.
> > +  This feature is deprecated.
> Why are you obliged to deprecate the feature.

We could leave the feature, but it adds complexity in the spec, and in
drivers if they have to support both. Maybe we can leave the feature,
merge its description with F_BYPASS_CONFIG and state something like:

  The VIRTIO_IOMMU_F_BYPASS_CONFIG feature supersedes
  VIRTIO_IOMMU_F_BYPASS. New devices SHOULD NOT offer
  VIRTIO_IOMMU_F_BYPASS.

> Can't you leave it and
> impose that either
> 
> VIRTIO_IOMMU_F_BYPASS or VIRTIO_IOMMU_F_BYPASS_CONFIG gets set?

Just to be clear, you mean devices must not implement both features at the
same time, right?  Not that devices must implement at least one of the
feature? (I don't think we can do that)

> 
> >  
> >  \item[VIRTIO_IOMMU_F_PROBE (4)]
> >    The PROBE request is available.
> >  
> >  \item[VIRTIO_IOMMU_F_MMIO (5)]
> >    The VIRTIO_IOMMU_MAP_F_MMIO flag is available.
> > +
> > +\item[VIRTIO_IOMMU_F_BYPASS_CONFIG (6)]
> > +  The global bypass state is described in \field{bypass} of
> > +  struct virtio_iommu_config. The domain bypass state is
> > +  described by VIRTIO_IOMMU_ATTACH_F_BYPASS.
> I would re-emphasize the global bypass state relates to bypass state of
> unattached devices whereas domain bypass state controls bypass for
> devices attached to a given domain.

Right, though I think I'll drop "bypass state" and describe everything
using "endpoint in bypass mode", so we can keep a single definition in one
place.

[...]
> >  \subsection{Device initialization}\label{sec:Device Types / IOMMU Device / Device initialization}
> >  
> >  When the device is reset, endpoints are not attached to any domain.
> >  
> > -If the VIRTIO_IOMMU_F_BYPASS feature is negotiated, all accesses from
> > -unattached endpoints are allowed and translated by the IOMMU using the
> > -identity function. If the feature is not negotiated, any memory access
> > -from an unattached endpoint fails. Upon attaching an endpoint in
> > -bypass mode to a new domain, any memory access from the endpoint fails,
> > -since the domain does not contain any mapping.
> > +An endpoint is in bypass mode if the VIRTIO_IOMMU_F_BYPASS_CONFIG
> > +feature is offered and:
> > +\begin{itemize}
> > +  \item config field \field{bypass} is 1 and the endpoint is not
> > +    attached to a domain, or
> > +  \item the endpoint is attached to a domain with
> > +    VIRTIO_IOMMU_ATTACH_F_BYPASS.
> > +\end{itemize}
> > +
> > +All accesses from an endpoint in bypass mode are allowed and
> > +translated by the IOMMU using the identity function.

I think I'll move this description to the Device operations section, not
sure why it was here in the first place.

> >  
> >  Future devices might support more modes of operation besides MAP/UNMAP.
> >  Drivers verify that devices set VIRTIO_IOMMU_F_MAP_UNMAP and fail
> > @@ -136,8 +156,8 @@ \subsection{Device initialization}\label{sec:Device Types / IOMMU Device / Devic
> >  
> >  \devicenormative{\subsubsection}{Device Initialization}{Device Types / IOMMU Device / Device Initialization}
> >  
> > -If the driver does not accept the VIRTIO_IOMMU_F_BYPASS feature, the
> > -device SHOULD NOT let endpoints access the guest-physical address space.
> > +The device SHOULD NOT let unattached endpoints that are not in
> > +bypass mode access the guest-physical address space.
> SHOULD NOT or MUST NOT. Can you remind me of the difference?

They are defined in rfc2119 https://datatracker.ietf.org/doc/html/rfc2119

SHOULD NOT
  This phrase, or the phrase "NOT RECOMMENDED" mean that there may exist
  valid reasons in particular circumstances when the particular behavior
  is acceptable or even useful, but the full implications should be
  understood and the case carefully weighed before implementing any
  behavior described with this label.

MUST NOT
  This phrase, or the phrase "SHALL NOT", mean that the definition is an
  absolute prohibition of the specification

So for this particular spec statement, I was reluctant to make it MUST
because there may be cases where the unattached endpoint has to access
some memory, even though I couldn't think of one. For example, maybe an
endpoint that should not perform DMA could still be allowed to send MSIs?

For the driver implementation, when the device has a "SHOULD NOT" and
doesn't follow the rule I try to handle it more gracefully, than when it
has a MUST NOT.

[...]
> >  Detach an endpoint from a domain. When this request completes, the
> > -endpoint cannot access any mapping from that domain anymore. If feature
> > -VIRTIO_IOMMU_F_BYPASS has been negotiated, then once this request
> > -completes all accesses from the endpoint are allowed and translated by the
> > -IOMMU using the identity function.
> > +endpoint cannot access any mapping from that domain anymore.
> You could adapt the rest of the paragraph to the new
> 
> VIRTIO_IOMMU_F_BYPASS_CONFIG
> 

I think the paragraph is redundant if we explain properly in "Device
operations" how bypass works.

> >  
> >  After all endpoints have been successfully detached from a domain, it
> >  ceases to exist and its ID can be reused by the driver for another domain.
> > @@ -433,6 +466,8 @@ \subsubsection{MAP request}\label{sec:Device Types / IOMMU Device / Device opera
> >  
> >  The driver SHOULD set undefined \field{flags} bits to zero.
> >  
> > +The driver SHOULD NOT send MAP requests on a bypass domain.
> > +
> >  \field{virt_end} MUST be strictly greater than \field{virt_start}.
> >  
> >  The driver SHOULD set the VIRTIO_IOMMU_MAP_F_MMIO flag when the physical
> > @@ -538,6 +573,8 @@ \subsubsection{UNMAP request}\label{sec:Device Types / IOMMU Device / Device ope
> >  or be outside any mapping. The last address of a range MUST either be the
> >  last address of a mapping or be outside any mapping.
> >  
> > +The driver SHOULD NOT send UNMAP requests on a bypass domain.
> > +
> >  \devicenormative{\paragraph}{UNMAP request}{Device Types / IOMMU Device / Device operations / UNMAP request}
> >  
> >  If the \field{reserved} field of an UNMAP request is not zero, the device
> Shouldn't we descrive the behavior of the device is MAP/UNMAP is called
> on a bypass domain or if we try to attach another device to a domain
> with a different ATTACH_F_BYPASS flag?

Ok

Thanks,
Jean

Re: [virtio-dev] [PATCH] virtio-iommu: Rework the bypass feature

[Eric Auger]

Hi Jean,

On 10/7/21 7:46 PM, Jean-Philippe Brucker wrote:
> Hi Eric,
>
> On Wed, Oct 06, 2021 at 02:53:50PM +0200, Eric Auger wrote:
>>> * The hypervisor can start the VM with bypass enabled or, if it knows
>>>   that the software stack supports it, disabled. The 'bypass' config
>>>   fields resets to 0 or 1.
>> I don't see the reset value documented in the actual spec while it looks
>> quite important for our boot bypass functionality. Or did I miss it?
> No you're right, I'll add this. It's important to say that the device may
> initialize the field to 1, but also that a reset (writing a 0 to device
> status) doesn't re-initialize the field to its boot value (because that
> may introduce a vulnerability during the FW->OS transition)
>
>   If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
>   initialize the \field{bypass} field to 1. When the driver resets the
>   device, the \field{bypass} field SHOULD NOT change.
>
> [...]
>>> diff --git a/virtio-iommu.tex b/virtio-iommu.tex
>>> index efa735b..a2c90ea 100644
>>> --- a/virtio-iommu.tex
>>> +++ b/virtio-iommu.tex
>>> @@ -59,14 +59,19 @@ \subsection{Feature bits}\label{sec:Device Types / IOMMU Device / Feature bits}
>>>    VIRTIO_IOMMU_F_MAP_UNMAP is supported.}
>>>  
>>>  \item[VIRTIO_IOMMU_F_BYPASS (3)]
>>> -  When not attached to a domain, endpoints downstream of the IOMMU
>>> -  can access the guest-physical address space.
>>> +  This feature is deprecated.
>> Why are you obliged to deprecate the feature.
> We could leave the feature, but it adds complexity in the spec, and in
> drivers if they have to support both. Maybe we can leave the feature,
> merge its description with F_BYPASS_CONFIG and state something like:
>
>   The VIRTIO_IOMMU_F_BYPASS_CONFIG feature supersedes
>   VIRTIO_IOMMU_F_BYPASS. New devices SHOULD NOT offer
>   VIRTIO_IOMMU_F_BYPASS.
>
>> Can't you leave it and
>> impose that either
>>
>> VIRTIO_IOMMU_F_BYPASS or VIRTIO_IOMMU_F_BYPASS_CONFIG gets set?
> Just to be clear, you mean devices must not implement both features at the
> same time, right? 

Yes that's what I meant.
>  Not that devices must implement at least one of the
> feature? (I don't think we can do that)
>
>>>  
>>>  \item[VIRTIO_IOMMU_F_PROBE (4)]
>>>    The PROBE request is available.
>>>  
>>>  \item[VIRTIO_IOMMU_F_MMIO (5)]
>>>    The VIRTIO_IOMMU_MAP_F_MMIO flag is available.
>>> +
>>> +\item[VIRTIO_IOMMU_F_BYPASS_CONFIG (6)]
>>> +  The global bypass state is described in \field{bypass} of
>>> +  struct virtio_iommu_config. The domain bypass state is
>>> +  described by VIRTIO_IOMMU_ATTACH_F_BYPASS.
>> I would re-emphasize the global bypass state relates to bypass state of
>> unattached devices whereas domain bypass state controls bypass for
>> devices attached to a given domain.
> Right, though I think I'll drop "bypass state" and describe everything
> using "endpoint in bypass mode", so we can keep a single definition in one
> place.
>
> [...]
>>>  \subsection{Device initialization}\label{sec:Device Types / IOMMU Device / Device initialization}
>>>  
>>>  When the device is reset, endpoints are not attached to any domain.
>>>  
>>> -If the VIRTIO_IOMMU_F_BYPASS feature is negotiated, all accesses from
>>> -unattached endpoints are allowed and translated by the IOMMU using the
>>> -identity function. If the feature is not negotiated, any memory access
>>> -from an unattached endpoint fails. Upon attaching an endpoint in
>>> -bypass mode to a new domain, any memory access from the endpoint fails,
>>> -since the domain does not contain any mapping.
>>> +An endpoint is in bypass mode if the VIRTIO_IOMMU_F_BYPASS_CONFIG
>>> +feature is offered and:
>>> +\begin{itemize}
>>> +  \item config field \field{bypass} is 1 and the endpoint is not
>>> +    attached to a domain, or
>>> +  \item the endpoint is attached to a domain with
>>> +    VIRTIO_IOMMU_ATTACH_F_BYPASS.
>>> +\end{itemize}
>>> +
>>> +All accesses from an endpoint in bypass mode are allowed and
>>> +translated by the IOMMU using the identity function.
> I think I'll move this description to the Device operations section, not
> sure why it was here in the first place.
>
>>>  
>>>  Future devices might support more modes of operation besides MAP/UNMAP.
>>>  Drivers verify that devices set VIRTIO_IOMMU_F_MAP_UNMAP and fail
>>> @@ -136,8 +156,8 @@ \subsection{Device initialization}\label{sec:Device Types / IOMMU Device / Devic
>>>  
>>>  \devicenormative{\subsubsection}{Device Initialization}{Device Types / IOMMU Device / Device Initialization}
>>>  
>>> -If the driver does not accept the VIRTIO_IOMMU_F_BYPASS feature, the
>>> -device SHOULD NOT let endpoints access the guest-physical address space.
>>> +The device SHOULD NOT let unattached endpoints that are not in
>>> +bypass mode access the guest-physical address space.
>> SHOULD NOT or MUST NOT. Can you remind me of the difference?
> They are defined in rfc2119 https://datatracker.ietf.org/doc/html/rfc2119
>
> SHOULD NOT
>   This phrase, or the phrase "NOT RECOMMENDED" mean that there may exist
>   valid reasons in particular circumstances when the particular behavior
>   is acceptable or even useful, but the full implications should be
>   understood and the case carefully weighed before implementing any
>   behavior described with this label.
>
> MUST NOT
>   This phrase, or the phrase "SHALL NOT", mean that the definition is an
>   absolute prohibition of the specification

Thank you for the reminder!
>
> So for this particular spec statement, I was reluctant to make it MUST
> because there may be cases where the unattached endpoint has to access
> some memory, even though I couldn't think of one. For example, maybe an
> endpoint that should not perform DMA could still be allowed to send MSIs?
>
> For the driver implementation, when the device has a "SHOULD NOT" and
> doesn't follow the rule I try to handle it more gracefully, than when it
> has a MUST NOT.

OK

Thanks

Eric
>
> [...]
>>>  Detach an endpoint from a domain. When this request completes, the
>>> -endpoint cannot access any mapping from that domain anymore. If feature
>>> -VIRTIO_IOMMU_F_BYPASS has been negotiated, then once this request
>>> -completes all accesses from the endpoint are allowed and translated by the
>>> -IOMMU using the identity function.
>>> +endpoint cannot access any mapping from that domain anymore.
>> You could adapt the rest of the paragraph to the new
>>
>> VIRTIO_IOMMU_F_BYPASS_CONFIG
>>
> I think the paragraph is redundant if we explain properly in "Device
> operations" how bypass works.
>
>>>  
>>>  After all endpoints have been successfully detached from a domain, it
>>>  ceases to exist and its ID can be reused by the driver for another domain.
>>> @@ -433,6 +466,8 @@ \subsubsection{MAP request}\label{sec:Device Types / IOMMU Device / Device opera
>>>  
>>>  The driver SHOULD set undefined \field{flags} bits to zero.
>>>  
>>> +The driver SHOULD NOT send MAP requests on a bypass domain.
>>> +
>>>  \field{virt_end} MUST be strictly greater than \field{virt_start}.
>>>  
>>>  The driver SHOULD set the VIRTIO_IOMMU_MAP_F_MMIO flag when the physical
>>> @@ -538,6 +573,8 @@ \subsubsection{UNMAP request}\label{sec:Device Types / IOMMU Device / Device ope
>>>  or be outside any mapping. The last address of a range MUST either be the
>>>  last address of a mapping or be outside any mapping.
>>>  
>>> +The driver SHOULD NOT send UNMAP requests on a bypass domain.
>>> +
>>>  \devicenormative{\paragraph}{UNMAP request}{Device Types / IOMMU Device / Device operations / UNMAP request}
>>>  
>>>  If the \field{reserved} field of an UNMAP request is not zero, the device
>> Shouldn't we descrive the behavior of the device is MAP/UNMAP is called
>> on a bypass domain or if we try to attach another device to a domain
>> with a different ATTACH_F_BYPASS flag?
> Ok
>
> Thanks,
> Jean
>