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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Oleksandr,

> On 9 Oct 2024, at 19:20, Oleksandr Tyshchenko <olekstysh@xxxxxxxxx> wrote:
> 
> 
> 
> On 09.10.24 09:36, Bertrand Marquis wrote:
>> Hi Oleksandr,
> 
> Hello Bertrand
> 
> 
>>> 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).
> 
> 
> ok to both. However, I am a bit lost ...
> 
> 
> 
>>> 
>>> 
>>>> 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.
> 
> 
> ... yes, I saw his answer and completely agree with him as well, my question 
> is what I should do with the rationale?
> 
> The rationale I proposed above explains why we do not want the IOMMU
> in particular to be assigned to a domain. But, it belongs to the initial 
> "Hide IOMMU from a domain" requirement. Now, with turning it into "Xen shall 
> configure the IOMMU at boot according to the stage 2 translation
> tables" requirement should the rationale still be present?

No this would now be part of integrator role or something like that but should 
not be a rationale here anymore.

> 
> 
>>> 
>>>>> +
>>>>> +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.
> 
> yes, I agree
> 
> 
>>> 
>>>>> +
>>>>> +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.
> 
> 
> I agree regarding DMA engine handling. As for register assignments and 
> interrupt handling I am not quite sure. I am afraid we will need to 
> differentiate between platform and PCI devices.

Once a PCI device has been detected it becomes some registers and interrupts.
There might be some PCI ways to configure some stuff but appart from that i do 
not see any difference with a platform device.
Do i miss something ?

> 
> 
>>> 
>>> 
>>>>> +
>>>>> +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.
> 
> Yes, what I got from the text above is that we won't need a separate set of 
> requirements for interrupt-less device, I will drop them. The same goes for 
> "Assign/Deassign *non-DMA-capable* platform device to domain". Please let me 
> know if I got this wrong.


No sounds good.

> 
> 
> [snip]
> 
> 
>>>>> +
>>>>> +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.
> 
> ok, thanks for the clarification.

Cheers
Bertrand

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