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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Oleksandr,

> On 8 Oct 2024, at 20:53, Oleksandr Tyshchenko <olekstysh@xxxxxxxxx> wrote:
> 
> 
> 
> On 08.10.24 09:17, Bertrand Marquis wrote:
>> Hi,
> 
> Hello Bertrand
> 
> 
>>> 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.
> 
> 
> ok, will add. I propose the following text:
> 
> Xen having the whole picture of the host resources and device assignment 
> unlike the individual domain makes use of the IOMMU to:
> - perform DMA Remapping on Arm64 and AMD64 platforms, which is also known as 
> stage-2 (or 2nd stage) address translations for DMA devices passed through to 
> domains and Interrupt Remapping on AMD64 platforms.
remove arm64 or amd64, this is always true

> - provide access protection functionalities to prevent malicious or buggy DMA 
> devices from accessing arbitrary memory ranges (e.g. memory allocated to 
> other domains) or from generating interrupts that could affect other domains.

I would turn this in positive: restrict DMA devices to only have access to the 
memory of the Domain there are assigned to or no memory at all if not assigned 
(maybe 2 reqs here).

> 
> 
>> 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.
> 
> I am afraid you are right, valid point. For example, on Arm64, if there is no 
> corresponding driver in use, we will end up exposing IOMMU dt node to Dom0.
> 
> 
>> Shouldn't this requirement in fact be an assumption of use ?
> 
> Assumption of use on Xen? From my PoV sounds reasonable, will do.

As was suggested by stefano, i agree with him on turning it differently. Please 
see his answer.

> 
>>> +
>>> +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.
> 
> I agree, will convert to Assumption of use on domain.
> 
> 
>>> +
>>> +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.
> 
> Makes sense, will drop "if the hardware domain is not present".
> 
> 
>>> +
>>> +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
> 
> I agree, will split, but ...
> 
>> - 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.
> 
> 
> ... does this need to be a separate 4th requirement here (and for the similar 
> requirement for the platform device down the document) or this sentence is 
> meant to be added to all resulting generic/arm64/amd requirements?
> 
> I would like to clarify, there are two groups of requirements to cover 
> DMA-capable devices in this document:
> - for devices that are behind the IOMMU and IOMMU can be used for them, those 
> requirements description explicitly mention "device xxx is protected by the 
> IOMMU" in addition to "(with IOMMU)" in the subject.
> - for devices that are not behind the IOMMU or IOMMU cannot be used for them, 
> those requirements description explicitly mention "device xxx is not 
> protected by the IOMMU" in addition to "(without IOMMU)" in the subject.

I think you need to be more generic and any DMA engine that is not protected by 
an IOMMU shall not be assigned to a non trusted domain.
This is in fact a requirement on the integrator, Xen cannot do much about this.

> 
>>> +
>>> +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.
> 
> 
> I would like to clarify, the second sentence here is just to indicate what 
> type of device (in the context of IOMMU involvement) the requirement is 
> talking about, not about special care for denying any DMA from it after 
> deassigning.
> 
> If you still think that we need a new requirement to explicitly highlight 
> that, I will be ok to create, in that case, I assume, the platform device 
> will want to gain the similar requirement. Please let me know your preference.

As said in the mail to stefano, i think we should try to generalise more.
So i would say we should handle:
- register assignments
- DMA engine handling
- interrupt handling

A device is a just a logical construct which may or may not contain or use 
several of those elements.

> 
> 
>>> +
>>> +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.
> 
> Good point, will split.
> 
> 
>>> +
>>> +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 ?
> 
> 
> Those devices exist and can be assigned to a domain, they are configured 
> slightly differently in comparison with devices with interrupts ("xen,path" 
> is not needed for the former), other code paths are executed in Xen.
> 
> More technical details:
> The allowance of the platform device assignment which is not behind an IOMMU 
> (for both non-DMA-capable and DMA-capable devices) is specified using device 
> tree property ("xen,force-assign-without-iommu") in the device node described 
> in the passthrough device tree. The said property also allows the 
> interrupt-less platform device assignment (a device that has only a MMIO 
> region) without specifying the corresponding node in the host device via 
> device tree property ("xen,path").

Please see upper.

> 
> 
>>> +
>>> +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 ?
> 
> Almost the same answer as for interrupt-less device. Here "xen,path" is 
> needed.
> 
> 
>> Wouldn't it me more logic to describe device passthrough and then what Xen 
>> should do for interrupts and for DMA ?
> 
> I can add more details on how this is configured/what properties are used, 
> etc in rationale for each requirement mentioning device tree. Or do you mean 
> something else?
> 
> 
>>> +
>>> +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.
> 
> Ok, will add details regarding passthrough/host device trees.
> 
> 
>>> +
>>> +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.
> 
> 
> I agree, will do. I feel it should be split into the following requirements:
> - Assign DMA-capable platform device to domain (without IOMMU)
> - Create direct mapped domain
> - Enable additional security mechanisms in direct mapped domain
> 
> To be honest, I'm not quite sure whether it is worth creating the last 
> requirement ...

I do not think the last one is needed here.
It could be an integrator guidance at best.

Cheers
Bertrand

> 
> 
>> Stopping here my review for now
> 
> Thanks for the review.
> 
> 
>> Cheers
>> Bertrand
> 
> [snip]