Re: [PATCH] docs: fusa: Add requirements for Device Passthrough

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 7 Oct 2024, at 20:55, Oleksandr Tyshchenko <olekstysh@xxxxxxxxx> wrote:
> 
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> Add common requirements for a physical device assignment to Arm64
> and AMD64 PVH domains.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> ---
> Based on:
> [PATCH] docs: fusa: Replace VM with domain
> https://patchew.org/Xen/20241007182603.826807-1-ayan.kumar.halder@xxxxxxx/
> ---
> ---
> .../reqs/design-reqs/common/passthrough.rst   | 365 ++++++++++++++++++
> docs/fusa/reqs/index.rst                      |   1 +
> docs/fusa/reqs/market-reqs/reqs.rst           |  33 ++
> docs/fusa/reqs/product-reqs/common/reqs.rst   |  29 ++
> 4 files changed, 428 insertions(+)
> create mode 100644 docs/fusa/reqs/design-reqs/common/passthrough.rst
> create mode 100644 docs/fusa/reqs/product-reqs/common/reqs.rst
> 
> diff --git a/docs/fusa/reqs/design-reqs/common/passthrough.rst 
> b/docs/fusa/reqs/design-reqs/common/passthrough.rst
> new file mode 100644
> index 0000000000..a1d6676f65
> --- /dev/null
> +++ b/docs/fusa/reqs/design-reqs/common/passthrough.rst
> @@ -0,0 +1,365 @@
> +.. SPDX-License-Identifier: CC-BY-4.0
> +
> +Device Passthrough
> +==================
> +
> +The following are the requirements related to a physical device assignment
> +[1], [2] to Arm64 and AMD64 PVH domains.
> +
> +Requirements for both Arm64 and AMD64 PVH
> +=========================================
> +
> +Hide IOMMU from a domain
> +------------------------
> +
> +`XenSwdgn~passthrough_hide_iommu_from_domain~1`
> +
> +Description:
> +Xen shall not expose the IOMMU device to the domain even if I/O 
> virtualization
> +is disabled. The IOMMU shall be under hypervisor control only.
> +
> +Rationale:

I think there should be a rationale here to explain why we do not want the IOMMU
in particular to be assigned to a domain.

Added to that, I am not completely sure that there is a clear way to test this 
one
as for example one could assign registers not known by Xen.

Shouldn't this requirement in fact be an assumption of use ?

> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Discover PCI devices from hardware domain
> +-----------------------------------------
> +
> +`XenSwdgn~passthrough_discover_pci_devices_from_hwdom~1`
> +
> +Description:
> +The hardware domain shall enumerate and discover PCI devices and inform Xen
> +about their appearance and disappearance.

Again this is a design requirement telling what should be done by a domain.
This is an interface or an assumption of use but not a Xen design req.

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Discover PCI devices from Xen
> +-----------------------------
> +
> +`XenSwdgn~passthrough_discover_pci_devices_from_xen~1`
> +
> +Description:
> +Xen shall discover PCI devices (enumerated by the firmware beforehand) during
> +boot if the hardware domain is not present.

I am a bit wondering here why we would not want Xen to always do it if we have
the code to do it in Xen anyway.

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Assign PCI device to domain (with IOMMU)
> +----------------------------------------
> +
> +`XenSwdgn~passthrough_assign_pci_device_with_iommu~1`
> +
> +Description:
> +Xen shall assign a specified PCI device (always implied as DMA-capable) to
> +a domain during its creation using passthrough (partial) device tree on Arm64
> +and Hyperlaunch device tree on AMD-x86. The physical device to be assigned is
> +protected by the IOMMU.

This is a very long and complex requirement.
I would suggest to split it in 3:
- generic: Xen shall support assign PCI devices to domains.
- arm64 one: Xen shall assign PCI devices based on device tree (explain how 
this is configured in dts)
- amd: xxxx based on hyperlaunch
- Xen shall use the IOMMU to enforce DMA operations done by a PCI device 
assigned to a domain to be restricted to the memory of the given domain.

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Deassign PCI device from domain (with IOMMU)
> +--------------------------------------------
> +
> +`XenSwdgn~passthrough_deassign_pci_device_with_iommu~1`
> +
> +Description:
> +Xen shall deassign a specified PCI device from a domain during its 
> destruction.
> +The physical device to be deassigned is protected by the IOMMU.

Remove second sentence or turn it into a req to say that the PCI device shall 
not be allowed to do DMA anymore somehow.

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Forbid the same PCI device assignment to multiple domains
> +---------------------------------------------------------
> +
> +`XenSwdgn~passthrough_forbid_same_pci_device_assignment~1`
> +
> +Description:
> +Xen shall not assign the same PCI device to multiple domains by failing to
> +create a new domain if the device to be passed through is already assigned
> +to the existing domain. Also different PCI devices which share some resources
> +(interrupts, IOMMU connections) can be assigned only to the same domain.

Please split and simplify
- Xen shall assign a single device to a single domain
- Xen shall assign PCI devices sharing resources to the same domain.

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Requirements for Arm64 only
> +===========================
> +
> +Assign interrupt-less platform device to domain
> +-----------------------------------------------
> +
> +`XenSwdgn~passthrough_assign_interrupt_less_platform_device~1`
> +
> +Description:
> +Xen shall assign a specified platform device that has only a MMIO region
> +(does not have any interrupts) to a domain during its creation using 
> passthrough
> +device tree.
> +The example of interrupt-less device is PWM or clock controller.

I am a bit puzzled by this req. Why making a specific case for interrupt less ?

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Deassign interrupt-less platform device from domain
> +---------------------------------------------------
> +
> +`XenSwdgn~passthrough_deassign_interrupt_less_platform_device~1`
> +
> +Description:
> +Xen shall deassign a specified platform device that has only a MMIO region
> +(does not have any interrupts) from a domain during its destruction.
> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Assign non-DMA-capable platform device to domain
> +------------------------------------------------
> +
> +`XenSwdgn~passthrough_assign_non_dma_platform_device~1`
> +
> +Description:
> +Xen shall assign a specified non-DMA-capable platform device to a domain 
> during
> +its creation using passthrough device tree.
> +The example of non-DMA-capable device is Timer.

Again why making a specific case here ?

Wouldn't it me more logic to describe device passthrough and then what Xen 
should do for interrupts and for DMA ?

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Deassign non-DMA-capable platform device from domain
> +----------------------------------------------------
> +
> +`XenSwdgn~passthrough_deassign_non_dma_platform_device~1`
> +
> +Description:
> +Xen shall deassign a specified non-DMA-capable platform device from a domain
> +during its destruction.
> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Assign DMA-capable platform device to domain (with IOMMU)
> +---------------------------------------------------------
> +
> +`XenSwdgn~passthrough_assign_dma_platform_device_with_iommu~1`
> +
> +Description:
> +Xen shall assign a specified DMA-capable platform device to a domain during
> +its creation using passthrough device tree. The physical device to be 
> assigned
> +is protected by the IOMMU.

This requirement is not a design but an higher level as it does not tell 
anything about implementation.

> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Deassign DMA-capable platform device from domain (with IOMMU)
> +-------------------------------------------------------------
> +
> +`XenSwdgn~passthrough_deassign_dma_platform_device_with_iommu~1`
> +
> +Description:
> +Xen shall deassign a specified DMA-capable platform device from a domain 
> during
> +its destruction. The physical device to be deassigned is protected by the 
> IOMMU.
> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Assign DMA-capable platform device to domain (without IOMMU)
> +------------------------------------------------------------
> +
> +`XenSwdgn~passthrough_assign_dma_platform_device_without_iommu~1`
> +
> +Description:
> +Xen shall assign a specified DMA-capable platform device to a domain during
> +its creation using passthrough device tree. The physical device to be 
> assigned
> +is not protected by the IOMMU.
> +The DMA-capable device assignment which is not behind an IOMMU is allowed for
> +the direct mapped domains only. The direct mapped domain must be either safe 
> or
> +additional security mechanisms for protecting against possible malicious or
> +buggy DMA devices must be in place, e.g. Xilinx memory protection unit (XMPU)
> +and Xilinx peripheral protection unit (XPPU).


Please split in several reqs.


Stopping here my review for now.

Cheers
Bertrand

> +
> +Rationale:
> +The IOMMU is absent from the system or it is disabled (status = "disabled"
> +in the host device tree).
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Deassign DMA-capable platform device from domain (without IOMMU)
> +----------------------------------------------------------------
> +
> +`XenSwdgn~passthrough_deassign_dma_platform_device_without_iommu~1`
> +
> +Description:
> +Xen shall deassign a specified DMA-capable platform device from a domain 
> during
> +its destruction. The physical device to be deassigned is not protected
> +by the IOMMU.
> +
> +Rationale:
> +The IOMMU is absent from the system or it is disabled (status = "disabled"
> +in the host device tree).
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Map platform device MMIO region identity
> +----------------------------------------
> +
> +`XenSwdgn~passthrough_map_platform_device_mmio_region_ident~1`
> +
> +Description:
> +Xen shall map platform device memory region identity (guest address ==
> +physical address) when assigning a specified platform device to a domain.
> +The device can be either non-DMA-capable or DMA-capable.
> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Map platform device MMIO region non-identity
> +--------------------------------------------
> +
> +`XenSwdgn~passthrough_map_platform_device_mmio_region_non_ident~1`
> +
> +Description:
> +Xen shall map platform device memory region non-identity (guest address !=
> +physical address) when assigning a specified platform device to a domain.
> +The device can be either non-DMA-capable or DMA-capable.
> +
> +Rationale:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Assign PCI device to domain (without IOMMU)
> +-------------------------------------------
> +
> +`XenSwdgn~passthrough_assign_pci_device_without_iommu~1`
> +
> +Description:
> +Xen shall assign a specified PCI device to a domain during its creation using
> +passthrough device tree. The physical device to be assigned is not protected
> +by the IOMMU.
> +The DMA-capable device assignment which is not behind an IOMMU is allowed for
> +the direct mapped domains only. The direct mapped domain must be either safe 
> or
> +additional security mechanisms for protecting against possible malicious or
> +buggy DMA devices must be in place, e.g. Xilinx memory protection unit (XMPU)
> +and Xilinx peripheral protection unit (XPPU).
> +
> +Rationale:
> +The IOMMU is absent from the system or it is disabled (status = "disabled"
> +in the host device tree).
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Deassign PCI device from domain (without IOMMU)
> +-----------------------------------------------
> +
> +`XenSwdgn~passthrough_deassign_pci_device_without_iommu~1`
> +
> +Description:
> +Xen shall deassign a specified PCI device from a domain during its 
> destruction.
> +The physical device to be deassigned is not protected by the IOMMU.
> +
> +Rationale:
> +The IOMMU is absent from the system or it is disabled (status = "disabled"
> +in the host device tree).
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Forbid the same platform device assignment to multiple domains
> +--------------------------------------------------------------
> +
> +`XenSwdgn~passthrough_forbid_same_platform_device_assignment~1`
> +
> +Description:
> +Xen shall not assign the same platform device to multiple domains by failing
> +to create a new domain if the device to be passed through is already assigned
> +to the existing domain. Also, different platform devices which share some
> +resources (interrupts, IOMMU connections) can be assigned only to the same
> +domain.
> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenProd~device_passthrough~1`
> +
> +Notes
> +=====
> +
> +The AMD64 PVH-specific requirements are written under the assumption that 
> once
> +the Hyperlaunch feature is completed, Xen shall assign a PCI device to boot
> +time domains. This is not the case today, where the PCI device can be passed
> +through only to domains launched by a control (toolstack) domain.
> +
> +The Arm64-specific requirements are written under the assumption that once
> +the dom0less PCI Passthrough feature is completed, Xen shall assign a PCI 
> device
> +to boot time domains. This is not the case today, where only the platform 
> device
> +Passthrough is supported.
> +
> +[1] 
> https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/passthrough.txt;hb=HEAD
> +[2] 
> https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=docs/misc/arm/passthrough-noiommu.txt;hb=HEAD
> diff --git a/docs/fusa/reqs/index.rst b/docs/fusa/reqs/index.rst
> index 183f183b1f..19c2f26b2b 100644
> --- a/docs/fusa/reqs/index.rst
> +++ b/docs/fusa/reqs/index.rst
> @@ -10,3 +10,4 @@ Requirements documentation
>    market-reqs
>    product-reqs
>    design-reqs/arm64
> +   design-reqs/common
> diff --git a/docs/fusa/reqs/market-reqs/reqs.rst 
> b/docs/fusa/reqs/market-reqs/reqs.rst
> index f456788d96..37a443395b 100644
> --- a/docs/fusa/reqs/market-reqs/reqs.rst
> +++ b/docs/fusa/reqs/market-reqs/reqs.rst
> @@ -47,3 +47,36 @@ Comments:
> 
> Needs:
>  - XenProd
> +
> +Run AMD-x86 domains
> +-------------------
> +
> +`XenMkt~run_x86_domains~1`
> +
> +Description:
> +Xen shall run AMD-x86 domains.
> +
> +Rationale:
> +
> +Comments:
> +
> +Needs:
> + - XenProd
> +
> +Domain device assignment
> +------------------------
> +
> +`XenMkt~domain_device_assignment~1`
> +
> +Description:
> +Xen shall assign device to each domain.
> +
> +For example, it shall assign GPU to domain A, MMC to domain B. Only the 
> domain
> +assigned to a device, shall have exclusive access to the device.
> +
> +Rationale:
> +
> +Comments:
> +
> +Needs:
> + - XenProd
> diff --git a/docs/fusa/reqs/product-reqs/common/reqs.rst 
> b/docs/fusa/reqs/product-reqs/common/reqs.rst
> new file mode 100644
> index 0000000000..9304399e4d
> --- /dev/null
> +++ b/docs/fusa/reqs/product-reqs/common/reqs.rst
> @@ -0,0 +1,29 @@
> +.. SPDX-License-Identifier: CC-BY-4.0
> +
> +Domain Creation And Runtime
> +===========================
> +
> +Device Passthrough
> +------------------
> +
> +`XenProd~device_passthrough~1`
> +
> +Description:
> +Xen shall provide mechanism for assigning a physical device to the domains.
> +
> +For example:
> +
> +- PCI passthrough
> +- MMC passthrough
> +
> +Rationale:
> +
> +Comments:
> +
> +Covers:
> + - `XenMkt~run_arm64_domains~1`
> + - `XenMkt~run_x86_domains~1`
> + - `XenMkt~domain_device_assignment~1`
> +
> +Needs:
> + - XenSwdgn
> -- 
> 2.34.1
>