[virtio-dev] [PATCH 0/4] virtio-iommu: Minor specification fixes

[virtio-dev] [PATCH 0/4] virtio-iommu: Minor specification fixes

[Jean-Philippe Brucker]

Patches 1 and 2 fix typos in the IOMMU device type. Patch 3 removes a
contradictory statement, and a useless one. Patch 4 adds some
information about removal of endpoints managed by the IOMMU, following a
recent fix in the Linux driver.

Jean-Philippe Brucker (4):
  virtio-iommu: Fix typo in label
  virtio-iommu: Fix RESV_MEM typo
  virtio-iommu: Fix contradictory and unnecessary statements
  virtio-iommu: Clarify hot-unplug behavior

 device-types/iommu/description.tex | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

-- 
2.41.0


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

[virtio-dev] [PATCH 1/4] virtio-iommu: Fix typo in label

[Jean-Philippe Brucker]

Use the 'sec:' prefix like all other labels

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 device-types/iommu/description.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/device-types/iommu/description.tex b/device-types/iommu/description.tex
index f8cbe88..d13a880 100644
--- a/device-types/iommu/description.tex
+++ b/device-types/iommu/description.tex
@@ -796,7 +796,7 @@ \subsubsection{PROBE properties}\label{sec:Device Types / IOMMU Device / Device
 The device SHOULD NOT allow accesses from the endpoint to RESV_MEM regions
 to affect any other component than the endpoint and the driver.
 
-\subsubsection{Fault reporting}\label{sev:Device Types / IOMMU Device / Device operations / Fault reporting}
+\subsubsection{Fault reporting}\label{sec:Device Types / IOMMU Device / Device operations / Fault reporting}
 
 The device can report translation faults and other significant
 asynchronous events on the event virtqueue. The driver initially populates
-- 
2.41.0


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

[virtio-dev] [PATCH 2/4] virtio-iommu: Fix RESV_MEM typo

[Jean-Philippe Brucker]

Remove an extra word in the RESV_MEM description.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 device-types/iommu/description.tex | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/device-types/iommu/description.tex b/device-types/iommu/description.tex
index d13a880..c9b43db 100644
--- a/device-types/iommu/description.tex
+++ b/device-types/iommu/description.tex
@@ -758,8 +758,8 @@ \subsubsection{PROBE properties}\label{sec:Device Types / IOMMU Device / Device
 \begin{description}
   \item[VIRTIO_IOMMU_RESV_MEM_T_RESERVED (0)]
     These virtual addresses cannot be used in a MAP requests. The region
-    is be reserved by the device, for example, if the platform needs to
-    setup DMA mappings of its own.
+    is reserved by the device, for example if the platform needs
+    to setup DMA mappings of its own.
 
   \item[VIRTIO_IOMMU_RESV_MEM_T_MSI (1)]
     This region is a doorbell for Message Signaled Interrupts (MSIs). It
-- 
2.41.0


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

[virtio-dev] [PATCH 3/4] virtio-iommu: Fix contradictory and unnecessary statements

[Jean-Philippe Brucker]

In the device-normative statement for the ATTACH request, we say:

  If another endpoint is already attached to the domain identified by
  \field{domain}, then the device MAY attach the endpoint identified by
  \field{endpoint} to the domain.

and later:

  If properties of the endpoint (obtained with a PROBE request) are
  compatible with properties of other endpoints already attached to the
  requested domain, then the device SHOULD attach the endpoint.

These statements contradict each others. Attaching multiple endpoints to
one domain is a convenience that allows using fewer page table
resources. The device is always allowed to reject a second attach
request, in which case the driver simply creates a separate domain for
the second endpoint. Remove the "SHOULD" statement and keep the "MAY"
one.

Also remove the following statement which doesn't add anything useful.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 device-types/iommu/description.tex | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/device-types/iommu/description.tex b/device-types/iommu/description.tex
index c9b43db..fb1b840 100644
--- a/device-types/iommu/description.tex
+++ b/device-types/iommu/description.tex
@@ -383,12 +383,9 @@ \subsubsection{ATTACH request}\label{sec:Device Types / IOMMU Device / Device op
 VIRTIO_IOMMU_S_UNSUPP.
 
 If properties of the endpoint (obtained with a PROBE request) are
-compatible with properties of other endpoints already attached to the
-requested domain, then the device SHOULD attach the endpoint. Otherwise
-the device SHOULD reject the request and set \field{status} to
-VIRTIO_IOMMU_S_UNSUPP.
-
-A device that does not reject the request MUST attach the endpoint.
+not compatible with properties of other endpoints already
+attached to the domain, the device SHOULD reject the request and
+set \field{status} to VIRTIO_IOMMU_S_UNSUPP.
 
 \subsubsection{DETACH request}
 
-- 
2.41.0


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

[virtio-dev] [PATCH 4/4] virtio-iommu: Clarify hot-unplug behavior

[Jean-Philippe Brucker]

Since it is not obvious what the virtio-iommu device or driver should do
when an endpoint is removed [1], add some guidance in the specification.
Follow the same principle as other (hardware) IOMMU devices: clearing
endpoint ID state on endpoint removal is the responsibility of the
driver, not the IOMMU device.

On most hardware IOMMU devices, the endpoint ID state is kept in tables
managed by the driver, so intuitively the driver should be updating the
tables on hot-unplug, so that a new endpoint plugged later with the same
ID does not inherit stale translations. Besides, hardware IOMMUs have no
knowledge of endpoint removal. It is less obvious for virtio-iommu
because endpoint states are managed with ATTACH/DETACH requests, and a
virtual platform could in theory update the endpoint state when it is
removed. Existing VMMs like QEMU don't do it, and rightly expect the
driver to manage endpoint ID state like with other IOMMUs. It is not
invalid for a VMM to clean up state on endpoint removal, but a driver
shouldn't count on it.

[1] https://lore.kernel.org/all/15bf1b00-3aa0-973a-3a86-3fa5c4d41d2c@daynix.com/

Reported-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 device-types/iommu/description.tex | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/device-types/iommu/description.tex b/device-types/iommu/description.tex
index fb1b840..63ccd41 100644
--- a/device-types/iommu/description.tex
+++ b/device-types/iommu/description.tex
@@ -407,6 +407,11 @@ \subsubsection{DETACH request}
 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.
 
+When an endpoint is removed from the platform (for example
+unplugged, or disabled with PCIe SR-IOV), the device is not
+required to detach the endpoint ID from its domain. It is the
+driver's responsibility to detach the endpoint before removal.
+
 \drivernormative{\paragraph}{DETACH request}{Device Types / IOMMU Device / Device operations / DETACH request}
 
 The driver SHOULD set \field{reserved} to zero.
-- 
2.41.0


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

[virtio-dev] Re: [PATCH 4/4] virtio-iommu: Clarify hot-unplug behavior

[Akihiko Odaki]

On 2023/08/04 0:32, Jean-Philippe Brucker wrote:
> Since it is not obvious what the virtio-iommu device or driver should do
> when an endpoint is removed [1], add some guidance in the specification.
> Follow the same principle as other (hardware) IOMMU devices: clearing
> endpoint ID state on endpoint removal is the responsibility of the
> driver, not the IOMMU device.
> 
> On most hardware IOMMU devices, the endpoint ID state is kept in tables
> managed by the driver, so intuitively the driver should be updating the
> tables on hot-unplug, so that a new endpoint plugged later with the same
> ID does not inherit stale translations. Besides, hardware IOMMUs have no
> knowledge of endpoint removal. It is less obvious for virtio-iommu
> because endpoint states are managed with ATTACH/DETACH requests, and a
> virtual platform could in theory update the endpoint state when it is
> removed. Existing VMMs like QEMU don't do it, and rightly expect the
> driver to manage endpoint ID state like with other IOMMUs. It is not
> invalid for a VMM to clean up state on endpoint removal, but a driver
> shouldn't count on it.

I think it's better to say it's invalid to detach on endpoint removal. 
Otherwise, the DETACH request made by the driver on endpoint removal 
will result in an error, which may make the driver emit an spurious 
warning. In fact, the Linux driver emits a warning if a DETACH request 
fails*

* 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/iommu/virtio-iommu.c?id=809d0810e3520da669d231303608cdf5fe5c1a70

> 
> [1] https://lore.kernel.org/all/15bf1b00-3aa0-973a-3a86-3fa5c4d41d2c@daynix.com/
> 
> Reported-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>   device-types/iommu/description.tex | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/device-types/iommu/description.tex b/device-types/iommu/description.tex
> index fb1b840..63ccd41 100644
> --- a/device-types/iommu/description.tex
> +++ b/device-types/iommu/description.tex
> @@ -407,6 +407,11 @@ \subsubsection{DETACH request}
>   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.
>   
> +When an endpoint is removed from the platform (for example
> +unplugged, or disabled with PCIe SR-IOV), the device is not
> +required to detach the endpoint ID from its domain. It is the
> +driver's responsibility to detach the endpoint before removal.
> +
>   \drivernormative{\paragraph}{DETACH request}{Device Types / IOMMU Device / Device operations / DETACH request}
>   
>   The driver SHOULD set \field{reserved} to zero.

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

[virtio-comment] Re: [PATCH 4/4] virtio-iommu: Clarify hot-unplug behavior

[Jean-Philippe Brucker]

On Fri, Aug 04, 2023 at 03:19:27PM +0900, Akihiko Odaki wrote:
> On 2023/08/04 0:32, Jean-Philippe Brucker wrote:
> > Since it is not obvious what the virtio-iommu device or driver should do
> > when an endpoint is removed [1], add some guidance in the specification.
> > Follow the same principle as other (hardware) IOMMU devices: clearing
> > endpoint ID state on endpoint removal is the responsibility of the
> > driver, not the IOMMU device.
> > 
> > On most hardware IOMMU devices, the endpoint ID state is kept in tables
> > managed by the driver, so intuitively the driver should be updating the
> > tables on hot-unplug, so that a new endpoint plugged later with the same
> > ID does not inherit stale translations. Besides, hardware IOMMUs have no
> > knowledge of endpoint removal. It is less obvious for virtio-iommu
> > because endpoint states are managed with ATTACH/DETACH requests, and a
> > virtual platform could in theory update the endpoint state when it is
> > removed. Existing VMMs like QEMU don't do it, and rightly expect the
> > driver to manage endpoint ID state like with other IOMMUs. It is not
> > invalid for a VMM to clean up state on endpoint removal, but a driver
> > shouldn't count on it.
> 
> I think it's better to say it's invalid to detach on endpoint removal.

It's certainly not a clear cut choice and I'm still hesitating whether we
should allow, deny or let the VMM choose what to do.

However, it looks like crosvm already detaches the domain when an endpoint
is unplugged:

https://github.com/google/crosvm/blob/e8b2cd080e8174948563567d395c2a42416f2807/devices/src/virtio/iommu/sys/unix.rs#L68

If I understood correctly, this function is called on PCIe hot-unplug, and
endpoint_map represents the domain attached to an endpoint ID. So the
domain is detached on endpoint removal. If a DETACH request is sent
afterwards, it will still succeed (detach_endpoint() in
devices/src/virtio/iommu.rs returns detached=true, which causes DETACH to
succeed, even if the endpoint is not in endpoint_map). But I could be
wrong as I'm not familiar with crosvm or rust.

That does make the decision a little easier, because if we did
retroactively specify that detaching on removal is invalid, this code
would become a bug. But in my opinion crosvm isn't doing anything wrong
here. It feels valid and even good practice to clean up all state
associated to an endpoint when it is removed, to avoid leaks and stale
objects.

> Otherwise, the DETACH request made by the driver on endpoint removal will
> result in an error, which may make the driver emit an spurious warning. In
> fact, the Linux driver emits a warning if a DETACH request fails*
> 
> * https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/iommu/virtio-iommu.c?id=809d0810e3520da669d231303608cdf5fe5c1a70
> 

I agree that this WARN_ON is problematic, we may have to demote it to
dev_warn() or remove it entirely.

PCIe supports async removal, where a device is physically removed without
the user first pressing a button to notify the OS that the device is going
away. And I'm currently playing with a PCIe thunderbolt drive where I just
yank the device out like any USB device, without first warning the OS
(apart from unmounting partitions). If a VMM wanted to emulate that, I
guess it would remove the device, possibly detach its IOMMU domain, and
notify the OS afterwards. So the driver needs to accept a failed DETACH
request more quietly.

Thanks,
Jean

> > 
> > [1] https://lore.kernel.org/all/15bf1b00-3aa0-973a-3a86-3fa5c4d41d2c@daynix.com/
> > 
> > Reported-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >   device-types/iommu/description.tex | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/device-types/iommu/description.tex b/device-types/iommu/description.tex
> > index fb1b840..63ccd41 100644
> > --- a/device-types/iommu/description.tex
> > +++ b/device-types/iommu/description.tex
> > @@ -407,6 +407,11 @@ \subsubsection{DETACH request}
> >   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.
> > +When an endpoint is removed from the platform (for example
> > +unplugged, or disabled with PCIe SR-IOV), the device is not
> > +required to detach the endpoint ID from its domain. It is the
> > +driver's responsibility to detach the endpoint before removal.
> > +
> >   \drivernormative{\paragraph}{DETACH request}{Device Types / IOMMU Device / Device operations / DETACH request}
> >   The driver SHOULD set \field{reserved} to zero.

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-dev] Re: [PATCH 4/4] virtio-iommu: Clarify hot-unplug behavior

[Jean-Philippe Brucker]

On Fri, Aug 04, 2023 at 03:19:27PM +0900, Akihiko Odaki wrote:
> On 2023/08/04 0:32, Jean-Philippe Brucker wrote:
> > Since it is not obvious what the virtio-iommu device or driver should do
> > when an endpoint is removed [1], add some guidance in the specification.
> > Follow the same principle as other (hardware) IOMMU devices: clearing
> > endpoint ID state on endpoint removal is the responsibility of the
> > driver, not the IOMMU device.
> > 
> > On most hardware IOMMU devices, the endpoint ID state is kept in tables
> > managed by the driver, so intuitively the driver should be updating the
> > tables on hot-unplug, so that a new endpoint plugged later with the same
> > ID does not inherit stale translations. Besides, hardware IOMMUs have no
> > knowledge of endpoint removal. It is less obvious for virtio-iommu
> > because endpoint states are managed with ATTACH/DETACH requests, and a
> > virtual platform could in theory update the endpoint state when it is
> > removed. Existing VMMs like QEMU don't do it, and rightly expect the
> > driver to manage endpoint ID state like with other IOMMUs. It is not
> > invalid for a VMM to clean up state on endpoint removal, but a driver
> > shouldn't count on it.
> 
> I think it's better to say it's invalid to detach on endpoint removal.

It's certainly not a clear cut choice and I'm still hesitating whether we
should allow, deny or let the VMM choose what to do.

However, it looks like crosvm already detaches the domain when an endpoint
is unplugged:

https://github.com/google/crosvm/blob/e8b2cd080e8174948563567d395c2a42416f2807/devices/src/virtio/iommu/sys/unix.rs#L68

If I understood correctly, this function is called on PCIe hot-unplug, and
endpoint_map represents the domain attached to an endpoint ID. So the
domain is detached on endpoint removal. If a DETACH request is sent
afterwards, it will still succeed (detach_endpoint() in
devices/src/virtio/iommu.rs returns detached=true, which causes DETACH to
succeed, even if the endpoint is not in endpoint_map). But I could be
wrong as I'm not familiar with crosvm or rust.

That does make the decision a little easier, because if we did
retroactively specify that detaching on removal is invalid, this code
would become a bug. But in my opinion crosvm isn't doing anything wrong
here. It feels valid and even good practice to clean up all state
associated to an endpoint when it is removed, to avoid leaks and stale
objects.

> Otherwise, the DETACH request made by the driver on endpoint removal will
> result in an error, which may make the driver emit an spurious warning. In
> fact, the Linux driver emits a warning if a DETACH request fails*
> 
> * https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/iommu/virtio-iommu.c?id=809d0810e3520da669d231303608cdf5fe5c1a70
> 

I agree that this WARN_ON is problematic, we may have to demote it to
dev_warn() or remove it entirely.

PCIe supports async removal, where a device is physically removed without
the user first pressing a button to notify the OS that the device is going
away. And I'm currently playing with a PCIe thunderbolt drive where I just
yank the device out like any USB device, without first warning the OS
(apart from unmounting partitions). If a VMM wanted to emulate that, I
guess it would remove the device, possibly detach its IOMMU domain, and
notify the OS afterwards. So the driver needs to accept a failed DETACH
request more quietly.

Thanks,
Jean

> > 
> > [1] https://lore.kernel.org/all/15bf1b00-3aa0-973a-3a86-3fa5c4d41d2c@daynix.com/
> > 
> > Reported-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >   device-types/iommu/description.tex | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/device-types/iommu/description.tex b/device-types/iommu/description.tex
> > index fb1b840..63ccd41 100644
> > --- a/device-types/iommu/description.tex
> > +++ b/device-types/iommu/description.tex
> > @@ -407,6 +407,11 @@ \subsubsection{DETACH request}
> >   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.
> > +When an endpoint is removed from the platform (for example
> > +unplugged, or disabled with PCIe SR-IOV), the device is not
> > +required to detach the endpoint ID from its domain. It is the
> > +driver's responsibility to detach the endpoint before removal.
> > +
> >   \drivernormative{\paragraph}{DETACH request}{Device Types / IOMMU Device / Device operations / DETACH request}
> >   The driver SHOULD set \field{reserved} to zero.

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

[virtio-dev] Re: [PATCH 4/4] virtio-iommu: Clarify hot-unplug behavior

[Akihiko Odaki]

On 2023/08/11 0:10, Jean-Philippe Brucker wrote:
> On Fri, Aug 04, 2023 at 03:19:27PM +0900, Akihiko Odaki wrote:
>> On 2023/08/04 0:32, Jean-Philippe Brucker wrote:
>>> Since it is not obvious what the virtio-iommu device or driver should do
>>> when an endpoint is removed [1], add some guidance in the specification.
>>> Follow the same principle as other (hardware) IOMMU devices: clearing
>>> endpoint ID state on endpoint removal is the responsibility of the
>>> driver, not the IOMMU device.
>>>
>>> On most hardware IOMMU devices, the endpoint ID state is kept in tables
>>> managed by the driver, so intuitively the driver should be updating the
>>> tables on hot-unplug, so that a new endpoint plugged later with the same
>>> ID does not inherit stale translations. Besides, hardware IOMMUs have no
>>> knowledge of endpoint removal. It is less obvious for virtio-iommu
>>> because endpoint states are managed with ATTACH/DETACH requests, and a
>>> virtual platform could in theory update the endpoint state when it is
>>> removed. Existing VMMs like QEMU don't do it, and rightly expect the
>>> driver to manage endpoint ID state like with other IOMMUs. It is not
>>> invalid for a VMM to clean up state on endpoint removal, but a driver
>>> shouldn't count on it.
>>
>> I think it's better to say it's invalid to detach on endpoint removal.
> 
> It's certainly not a clear cut choice and I'm still hesitating whether we
> should allow, deny or let the VMM choose what to do.
> 
> However, it looks like crosvm already detaches the domain when an endpoint
> is unplugged:
> 
> https://github.com/google/crosvm/blob/e8b2cd080e8174948563567d395c2a42416f2807/devices/src/virtio/iommu/sys/unix.rs#L68
> 
> If I understood correctly, this function is called on PCIe hot-unplug, and
> endpoint_map represents the domain attached to an endpoint ID. So the
> domain is detached on endpoint removal. If a DETACH request is sent
> afterwards, it will still succeed (detach_endpoint() in
> devices/src/virtio/iommu.rs returns detached=true, which causes DETACH to
> succeed, even if the endpoint is not in endpoint_map). But I could be
> wrong as I'm not familiar with crosvm or rust.
> 
> That does make the decision a little easier, because if we did
> retroactively specify that detaching on removal is invalid, this code
> would become a bug. But in my opinion crosvm isn't doing anything wrong
> here. It feels valid and even good practice to clean up all state
> associated to an endpoint when it is removed, to avoid leaks and stale
> objects.

In this case I think it's creating use-after-free hazard. The guest 
believes it is managing domains and assume a domain is available until 
it makes a DETACH request for the last endpoint attached to the domain.

However, if the host automatically detaches an endpoint from a domain 
and if the domain has no other endpoints, the domain will be gone and 
the domain ID may be reused for another domain. If the guest makes a 
request with the stale domain ID before it becomes aware that the device 
is unplugged, the request will be performed on some arbitrary domain and 
may break things.

By the way, crosvm's logic to detach endpoint on removal looks incorrect 
for me. A domain may have several endpoints attached, but the code looks 
like it's always destroying a domain whether there are other endpoints 
attached to the domain. I'm adding Zide Chen, who wrote the code 
according to git blame, and crosvm-dev@chromium.org to CC.

Regards,
Akihiko Odaki

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

[virtio-comment] Re: [PATCH 4/4] virtio-iommu: Clarify hot-unplug behavior

[Jean-Philippe Brucker]

On Fri, Aug 11, 2023 at 07:21:29AM +0900, Akihiko Odaki wrote:
> On 2023/08/11 0:10, Jean-Philippe Brucker wrote:
> > On Fri, Aug 04, 2023 at 03:19:27PM +0900, Akihiko Odaki wrote:
> > > On 2023/08/04 0:32, Jean-Philippe Brucker wrote:
> > > > Since it is not obvious what the virtio-iommu device or driver should do
> > > > when an endpoint is removed [1], add some guidance in the specification.
> > > > Follow the same principle as other (hardware) IOMMU devices: clearing
> > > > endpoint ID state on endpoint removal is the responsibility of the
> > > > driver, not the IOMMU device.
> > > > 
> > > > On most hardware IOMMU devices, the endpoint ID state is kept in tables
> > > > managed by the driver, so intuitively the driver should be updating the
> > > > tables on hot-unplug, so that a new endpoint plugged later with the same
> > > > ID does not inherit stale translations. Besides, hardware IOMMUs have no
> > > > knowledge of endpoint removal. It is less obvious for virtio-iommu
> > > > because endpoint states are managed with ATTACH/DETACH requests, and a
> > > > virtual platform could in theory update the endpoint state when it is
> > > > removed. Existing VMMs like QEMU don't do it, and rightly expect the
> > > > driver to manage endpoint ID state like with other IOMMUs. It is not
> > > > invalid for a VMM to clean up state on endpoint removal, but a driver
> > > > shouldn't count on it.
> > > 
> > > I think it's better to say it's invalid to detach on endpoint removal.
> > 
> > It's certainly not a clear cut choice and I'm still hesitating whether we
> > should allow, deny or let the VMM choose what to do.
> > 
> > However, it looks like crosvm already detaches the domain when an endpoint
> > is unplugged:
> > 
> > https://github.com/google/crosvm/blob/e8b2cd080e8174948563567d395c2a42416f2807/devices/src/virtio/iommu/sys/unix.rs#L68
> > 
> > If I understood correctly, this function is called on PCIe hot-unplug, and
> > endpoint_map represents the domain attached to an endpoint ID. So the
> > domain is detached on endpoint removal. If a DETACH request is sent
> > afterwards, it will still succeed (detach_endpoint() in
> > devices/src/virtio/iommu.rs returns detached=true, which causes DETACH to
> > succeed, even if the endpoint is not in endpoint_map). But I could be
> > wrong as I'm not familiar with crosvm or rust.
> > 
> > That does make the decision a little easier, because if we did
> > retroactively specify that detaching on removal is invalid, this code
> > would become a bug. But in my opinion crosvm isn't doing anything wrong
> > here. It feels valid and even good practice to clean up all state
> > associated to an endpoint when it is removed, to avoid leaks and stale
> > objects.
> 
> In this case I think it's creating use-after-free hazard. The guest believes
> it is managing domains and assume a domain is available until it makes a
> DETACH request for the last endpoint attached to the domain.
> 
> However, if the host automatically detaches an endpoint from a domain and if
> the domain has no other endpoints, the domain will be gone and the domain ID
> may be reused for another domain. If the guest makes a request with the
> stale domain ID before it becomes aware that the device is unplugged, the
> request will be performed on some arbitrary domain and may break things.

That's an excellent point, there is a race:

        VMM                        GUEST
     Remove ep 1
       detach domain 1
       destroy domain 1
                                ATTACH ep 2 to domain 1
     Handle ATTACH
       create domain 1

     Notify that ep 1 is gone
                                Handle notification, DETACH ep 1

The guest tries to attach ep 2 to domain 1, which succeeds. So the
guest expects that ep 2 is now able to access the mappings that were on
domain 1. But instead it's a new empty domain.

I think that's a good justification for adding a device normative
statement, something like:

  The device SHOULD only detach endpoints from domains when requested by
  the driver; either when handling ATTACH and DETACH requests or when
  performing a device reset.

> 
> By the way, crosvm's logic to detach endpoint on removal looks incorrect for
> me. A domain may have several endpoints attached, but the code looks like
> it's always destroying a domain whether there are other endpoints attached
> to the domain. I'm adding Zide Chen, who wrote the code according to git
> blame, and crosvm-dev@chromium.org to CC.

Link to this thread for more context:
https://lore.kernel.org/virtio-dev/20230803153238.541803-5-jean-philippe@linaro.org/

I thought crosvm rejected attaching multiple endpoints to one domain but I
think I misread. Rejecting multiple attach would be a straightforward fix
(it's allowed by the spec), though it would prevent assigning endpoints
that cannot be isolated from each others by the hardware (the driver won't
attach those to different domains, if it's made aware that they should be
in the same IOMMU group, for example if they are on a conventional PCI
bus).

Thanks,
Jean

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-dev] Re: [PATCH 4/4] virtio-iommu: Clarify hot-unplug behavior

[Jean-Philippe Brucker]

On Fri, Aug 11, 2023 at 07:21:29AM +0900, Akihiko Odaki wrote:
> On 2023/08/11 0:10, Jean-Philippe Brucker wrote:
> > On Fri, Aug 04, 2023 at 03:19:27PM +0900, Akihiko Odaki wrote:
> > > On 2023/08/04 0:32, Jean-Philippe Brucker wrote:
> > > > Since it is not obvious what the virtio-iommu device or driver should do
> > > > when an endpoint is removed [1], add some guidance in the specification.
> > > > Follow the same principle as other (hardware) IOMMU devices: clearing
> > > > endpoint ID state on endpoint removal is the responsibility of the
> > > > driver, not the IOMMU device.
> > > > 
> > > > On most hardware IOMMU devices, the endpoint ID state is kept in tables
> > > > managed by the driver, so intuitively the driver should be updating the
> > > > tables on hot-unplug, so that a new endpoint plugged later with the same
> > > > ID does not inherit stale translations. Besides, hardware IOMMUs have no
> > > > knowledge of endpoint removal. It is less obvious for virtio-iommu
> > > > because endpoint states are managed with ATTACH/DETACH requests, and a
> > > > virtual platform could in theory update the endpoint state when it is
> > > > removed. Existing VMMs like QEMU don't do it, and rightly expect the
> > > > driver to manage endpoint ID state like with other IOMMUs. It is not
> > > > invalid for a VMM to clean up state on endpoint removal, but a driver
> > > > shouldn't count on it.
> > > 
> > > I think it's better to say it's invalid to detach on endpoint removal.
> > 
> > It's certainly not a clear cut choice and I'm still hesitating whether we
> > should allow, deny or let the VMM choose what to do.
> > 
> > However, it looks like crosvm already detaches the domain when an endpoint
> > is unplugged:
> > 
> > https://github.com/google/crosvm/blob/e8b2cd080e8174948563567d395c2a42416f2807/devices/src/virtio/iommu/sys/unix.rs#L68
> > 
> > If I understood correctly, this function is called on PCIe hot-unplug, and
> > endpoint_map represents the domain attached to an endpoint ID. So the
> > domain is detached on endpoint removal. If a DETACH request is sent
> > afterwards, it will still succeed (detach_endpoint() in
> > devices/src/virtio/iommu.rs returns detached=true, which causes DETACH to
> > succeed, even if the endpoint is not in endpoint_map). But I could be
> > wrong as I'm not familiar with crosvm or rust.
> > 
> > That does make the decision a little easier, because if we did
> > retroactively specify that detaching on removal is invalid, this code
> > would become a bug. But in my opinion crosvm isn't doing anything wrong
> > here. It feels valid and even good practice to clean up all state
> > associated to an endpoint when it is removed, to avoid leaks and stale
> > objects.
> 
> In this case I think it's creating use-after-free hazard. The guest believes
> it is managing domains and assume a domain is available until it makes a
> DETACH request for the last endpoint attached to the domain.
> 
> However, if the host automatically detaches an endpoint from a domain and if
> the domain has no other endpoints, the domain will be gone and the domain ID
> may be reused for another domain. If the guest makes a request with the
> stale domain ID before it becomes aware that the device is unplugged, the
> request will be performed on some arbitrary domain and may break things.

That's an excellent point, there is a race:

        VMM                        GUEST
     Remove ep 1
       detach domain 1
       destroy domain 1
                                ATTACH ep 2 to domain 1
     Handle ATTACH
       create domain 1

     Notify that ep 1 is gone
                                Handle notification, DETACH ep 1

The guest tries to attach ep 2 to domain 1, which succeeds. So the
guest expects that ep 2 is now able to access the mappings that were on
domain 1. But instead it's a new empty domain.

I think that's a good justification for adding a device normative
statement, something like:

  The device SHOULD only detach endpoints from domains when requested by
  the driver; either when handling ATTACH and DETACH requests or when
  performing a device reset.

> 
> By the way, crosvm's logic to detach endpoint on removal looks incorrect for
> me. A domain may have several endpoints attached, but the code looks like
> it's always destroying a domain whether there are other endpoints attached
> to the domain. I'm adding Zide Chen, who wrote the code according to git
> blame, and crosvm-dev@chromium.org to CC.

Link to this thread for more context:
https://lore.kernel.org/virtio-dev/20230803153238.541803-5-jean-philippe@linaro.org/

I thought crosvm rejected attaching multiple endpoints to one domain but I
think I misread. Rejecting multiple attach would be a straightforward fix
(it's allowed by the spec), though it would prevent assigning endpoints
that cannot be isolated from each others by the hardware (the driver won't
attach those to different domains, if it's made aware that they should be
in the same IOMMU group, for example if they are on a conventional PCI
bus).

Thanks,
Jean

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

[virtio-dev] Re: [PATCH 4/4] virtio-iommu: Clarify hot-unplug behavior

[Akihiko Odaki]

On 2023/08/11 23:20, Jean-Philippe Brucker wrote:
> On Fri, Aug 11, 2023 at 07:21:29AM +0900, Akihiko Odaki wrote:
>> On 2023/08/11 0:10, Jean-Philippe Brucker wrote:
>>> On Fri, Aug 04, 2023 at 03:19:27PM +0900, Akihiko Odaki wrote:
>>>> On 2023/08/04 0:32, Jean-Philippe Brucker wrote:
>>>>> Since it is not obvious what the virtio-iommu device or driver should do
>>>>> when an endpoint is removed [1], add some guidance in the specification.
>>>>> Follow the same principle as other (hardware) IOMMU devices: clearing
>>>>> endpoint ID state on endpoint removal is the responsibility of the
>>>>> driver, not the IOMMU device.
>>>>>
>>>>> On most hardware IOMMU devices, the endpoint ID state is kept in tables
>>>>> managed by the driver, so intuitively the driver should be updating the
>>>>> tables on hot-unplug, so that a new endpoint plugged later with the same
>>>>> ID does not inherit stale translations. Besides, hardware IOMMUs have no
>>>>> knowledge of endpoint removal. It is less obvious for virtio-iommu
>>>>> because endpoint states are managed with ATTACH/DETACH requests, and a
>>>>> virtual platform could in theory update the endpoint state when it is
>>>>> removed. Existing VMMs like QEMU don't do it, and rightly expect the
>>>>> driver to manage endpoint ID state like with other IOMMUs. It is not
>>>>> invalid for a VMM to clean up state on endpoint removal, but a driver
>>>>> shouldn't count on it.
>>>>
>>>> I think it's better to say it's invalid to detach on endpoint removal.
>>>
>>> It's certainly not a clear cut choice and I'm still hesitating whether we
>>> should allow, deny or let the VMM choose what to do.
>>>
>>> However, it looks like crosvm already detaches the domain when an endpoint
>>> is unplugged:
>>>
>>> https://github.com/google/crosvm/blob/e8b2cd080e8174948563567d395c2a42416f2807/devices/src/virtio/iommu/sys/unix.rs#L68
>>>
>>> If I understood correctly, this function is called on PCIe hot-unplug, and
>>> endpoint_map represents the domain attached to an endpoint ID. So the
>>> domain is detached on endpoint removal. If a DETACH request is sent
>>> afterwards, it will still succeed (detach_endpoint() in
>>> devices/src/virtio/iommu.rs returns detached=true, which causes DETACH to
>>> succeed, even if the endpoint is not in endpoint_map). But I could be
>>> wrong as I'm not familiar with crosvm or rust.
>>>
>>> That does make the decision a little easier, because if we did
>>> retroactively specify that detaching on removal is invalid, this code
>>> would become a bug. But in my opinion crosvm isn't doing anything wrong
>>> here. It feels valid and even good practice to clean up all state
>>> associated to an endpoint when it is removed, to avoid leaks and stale
>>> objects.
>>
>> In this case I think it's creating use-after-free hazard. The guest believes
>> it is managing domains and assume a domain is available until it makes a
>> DETACH request for the last endpoint attached to the domain.
>>
>> However, if the host automatically detaches an endpoint from a domain and if
>> the domain has no other endpoints, the domain will be gone and the domain ID
>> may be reused for another domain. If the guest makes a request with the
>> stale domain ID before it becomes aware that the device is unplugged, the
>> request will be performed on some arbitrary domain and may break things.
> 
> That's an excellent point, there is a race:
> 
>          VMM                        GUEST
>       Remove ep 1
>         detach domain 1
>         destroy domain 1
>                                  ATTACH ep 2 to domain 1
>       Handle ATTACH
>         create domain 1
> 
>       Notify that ep 1 is gone
>                                  Handle notification, DETACH ep 1
> 
> The guest tries to attach ep 2 to domain 1, which succeeds. So the
> guest expects that ep 2 is now able to access the mappings that were on
> domain 1. But instead it's a new empty domain.
> 
> I think that's a good justification for adding a device normative
> statement, something like:
> 
>    The device SHOULD only detach endpoints from domains when requested by
>    the driver; either when handling ATTACH and DETACH requests or when
>    performing a device reset.
> 
>>
>> By the way, crosvm's logic to detach endpoint on removal looks incorrect for
>> me. A domain may have several endpoints attached, but the code looks like
>> it's always destroying a domain whether there are other endpoints attached
>> to the domain. I'm adding Zide Chen, who wrote the code according to git
>> blame, and crosvm-dev@chromium.org to CC.
> 
> Link to this thread for more context:
> https://lore.kernel.org/virtio-dev/20230803153238.541803-5-jean-philippe@linaro.org/
> 
> I thought crosvm rejected attaching multiple endpoints to one domain but I
> think I misread. Rejecting multiple attach would be a straightforward fix
> (it's allowed by the spec), though it would prevent assigning endpoints
> that cannot be isolated from each others by the hardware (the driver won't
> attach those to different domains, if it's made aware that they should be
> in the same IOMMU group, for example if they are on a conventional PCI
> bus).

Now we figured out an endpoint should not be detached from a domain 
without a request from the driver anyway so the code to detach an 
endpoint can be simply removed.

Regards,
Akihiko Odaki

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

[virtio-comment] Re: [PATCH 4/4] virtio-iommu: Clarify hot-unplug behavior

[Jean-Philippe Brucker]

On Sat, Aug 12, 2023 at 03:25:10PM +0900, Akihiko Odaki wrote:
> > > By the way, crosvm's logic to detach endpoint on removal looks incorrect for
> > > me. A domain may have several endpoints attached, but the code looks like
> > > it's always destroying a domain whether there are other endpoints attached
> > > to the domain. I'm adding Zide Chen, who wrote the code according to git
> > > blame, and crosvm-dev@chromium.org to CC.
> > 
> > Link to this thread for more context:
> > https://lore.kernel.org/virtio-dev/20230803153238.541803-5-jean-philippe@linaro.org/
> > 
> > I thought crosvm rejected attaching multiple endpoints to one domain but I
> > think I misread. Rejecting multiple attach would be a straightforward fix
> > (it's allowed by the spec), though it would prevent assigning endpoints
> > that cannot be isolated from each others by the hardware (the driver won't
> > attach those to different domains, if it's made aware that they should be
> > in the same IOMMU group, for example if they are on a conventional PCI
> > bus).
> 
> Now we figured out an endpoint should not be detached from a domain without
> a request from the driver anyway so the code to detach an endpoint can be
> simply removed.

Yes, but I think the other detach path, when handling ATTACH or DETACH
requests, doesn't support domains with multiple endpoints attached either:

    // Currently, we only support detaching an endpoint if it is the only endpoint attached
    // to its domain.

But the ATTACH handler seems to accept attaching multiple endpoints to the
same domain?

Thanks,
Jean

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-dev] Re: [PATCH 4/4] virtio-iommu: Clarify hot-unplug behavior

[Jean-Philippe Brucker]

On Sat, Aug 12, 2023 at 03:25:10PM +0900, Akihiko Odaki wrote:
> > > By the way, crosvm's logic to detach endpoint on removal looks incorrect for
> > > me. A domain may have several endpoints attached, but the code looks like
> > > it's always destroying a domain whether there are other endpoints attached
> > > to the domain. I'm adding Zide Chen, who wrote the code according to git
> > > blame, and crosvm-dev@chromium.org to CC.
> > 
> > Link to this thread for more context:
> > https://lore.kernel.org/virtio-dev/20230803153238.541803-5-jean-philippe@linaro.org/
> > 
> > I thought crosvm rejected attaching multiple endpoints to one domain but I
> > think I misread. Rejecting multiple attach would be a straightforward fix
> > (it's allowed by the spec), though it would prevent assigning endpoints
> > that cannot be isolated from each others by the hardware (the driver won't
> > attach those to different domains, if it's made aware that they should be
> > in the same IOMMU group, for example if they are on a conventional PCI
> > bus).
> 
> Now we figured out an endpoint should not be detached from a domain without
> a request from the driver anyway so the code to detach an endpoint can be
> simply removed.

Yes, but I think the other detach path, when handling ATTACH or DETACH
requests, doesn't support domains with multiple endpoints attached either:

    // Currently, we only support detaching an endpoint if it is the only endpoint attached
    // to its domain.

But the ATTACH handler seems to accept attaching multiple endpoints to the
same domain?

Thanks,
Jean

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

[virtio-dev] Re: [PATCH 4/4] virtio-iommu: Clarify hot-unplug behavior

[Akihiko Odaki]

On 2023/08/14 20:25, Jean-Philippe Brucker wrote:
> On Sat, Aug 12, 2023 at 03:25:10PM +0900, Akihiko Odaki wrote:
>>>> By the way, crosvm's logic to detach endpoint on removal looks incorrect for
>>>> me. A domain may have several endpoints attached, but the code looks like
>>>> it's always destroying a domain whether there are other endpoints attached
>>>> to the domain. I'm adding Zide Chen, who wrote the code according to git
>>>> blame, and crosvm-dev@chromium.org to CC.
>>>
>>> Link to this thread for more context:
>>> https://lore.kernel.org/virtio-dev/20230803153238.541803-5-jean-philippe@linaro.org/
>>>
>>> I thought crosvm rejected attaching multiple endpoints to one domain but I
>>> think I misread. Rejecting multiple attach would be a straightforward fix
>>> (it's allowed by the spec), though it would prevent assigning endpoints
>>> that cannot be isolated from each others by the hardware (the driver won't
>>> attach those to different domains, if it's made aware that they should be
>>> in the same IOMMU group, for example if they are on a conventional PCI
>>> bus).
>>
>> Now we figured out an endpoint should not be detached from a domain without
>> a request from the driver anyway so the code to detach an endpoint can be
>> simply removed.
> 
> Yes, but I think the other detach path, when handling ATTACH or DETACH
> requests, doesn't support domains with multiple endpoints attached either:
> 
>      // Currently, we only support detaching an endpoint if it is the only endpoint attached
>      // to its domain.
> 
> But the ATTACH handler seems to accept attaching multiple endpoints to the
> same domain?
> 
> Thanks,
> Jean

We saw no response from crosvm people for a few weeks so I opened an 
issue on their bug tracker for heads-up:
https://issuetracker.google.com/u/1/issues/298297288

Regards,
Akihiko Odaki

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