diff mbox series

[v2,1/3] xen/arm: pci: introduce PCI_PASSTHROUGH Kconfig option

Message ID 20230707014754.51333-2-stewart.hildebrand@amd.com (mailing list archive)
State New, archived
Headers show
Series Kconfig for PCI passthrough on ARM | expand

Commit Message

Stewart Hildebrand July 7, 2023, 1:47 a.m. UTC
From: Rahul Singh <rahul.singh@arm.com>

Setting CONFIG_PCI_PASSTHROUGH=y will enable PCI passthrough on ARM, even though
the feature is not yet complete in the current upstream codebase. The purpose of
this is to make it easier to enable the necessary configs (HAS_PCI, HAS_VPCI) for
testing and development of PCI passthrough on ARM.

Since PCI passthrough on ARM is still work in progress at this time, make it
depend on EXPERT.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
(cherry picked from commit 9a08f1f7ce28ec619640ba9ce11018bf443e9a0e from the
 downstream branch [1])

v1->v2:
* drop "ARM" naming since it is already in an ARM category
* depend on EXPERT instead of UNSUPPORTED

Changes from downstream to v1:
* depends on ARM_64 (Stefano)
* Don't select HAS_VPCI_GUEST_SUPPORT since this config option is not currently
  used in the upstream codebase. This will want to be re-added here once the
  vpci series [2] is merged.
* Don't select ARM_SMMU_V3 since this option can already be selected
  independently. While PCI passthrough on ARM depends on an SMMU, it does not
  depend on a particular version or variant of an SMMU.
* Don't select HAS_ITS since this option can already be selected independently.
  HAS_ITS may want to be added here once the MSI series [1] is merged.
* Don't select LATE_HWDOM since this option is unrelated to PCI passthrough.

[1] https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commits/poc/pci-passthrough
[2] https://lists.xenproject.org/archives/html/xen-devel/2023-06/msg00863.html
---
 xen/arch/arm/Kconfig | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Jan Beulich July 7, 2023, 6:55 a.m. UTC | #1
On 07.07.2023 03:47, Stewart Hildebrand wrote:
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -190,6 +190,15 @@ config STATIC_SHM
>  	help
>  	  This option enables statically shared memory on a dom0less system.
>  
> +config PCI_PASSTHROUGH
> +	bool "PCI passthrough" if EXPERT
> +	depends on ARM_64
> +	select HAS_PCI
> +	select HAS_VPCI
> +	default n

No need for this line - that's what the tool does by default anyway.

Jan
Julien Grall July 7, 2023, 8:10 a.m. UTC | #2
Hi Jan,

On 07/07/2023 07:55, Jan Beulich wrote:
> On 07.07.2023 03:47, Stewart Hildebrand wrote:
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -190,6 +190,15 @@ config STATIC_SHM
>>   	help
>>   	  This option enables statically shared memory on a dom0less system.
>>   
>> +config PCI_PASSTHROUGH
>> +	bool "PCI passthrough" if EXPERT
>> +	depends on ARM_64
>> +	select HAS_PCI
>> +	select HAS_VPCI
>> +	default n
> 
> No need for this line - that's what the tool does by default anyway.

I would rather keep. We are already using 'default n' and it is making 
more obvious the default value.

Cheers,
Julien Grall July 7, 2023, 8:38 a.m. UTC | #3
Hi,

On 07/07/2023 02:47, Stewart Hildebrand wrote:
> From: Rahul Singh <rahul.singh@arm.com>
> 
> Setting CONFIG_PCI_PASSTHROUGH=y will enable PCI passthrough on ARM, even though
> the feature is not yet complete in the current upstream codebase. The purpose of
> this is to make it easier to enable the necessary configs (HAS_PCI, HAS_VPCI) for
> testing and development of PCI passthrough on ARM.
> 
> Since PCI passthrough on ARM is still work in progress at this time, make it
> depend on EXPERT.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,
Julien Grall July 13, 2023, 6:40 p.m. UTC | #4
Hi Stewart,

On 07/07/2023 02:47, Stewart Hildebrand wrote:
> From: Rahul Singh <rahul.singh@arm.com>
> 
> Setting CONFIG_PCI_PASSTHROUGH=y will enable PCI passthrough on ARM, even though
> the feature is not yet complete in the current upstream codebase. The purpose of
> this is to make it easier to enable the necessary configs (HAS_PCI, HAS_VPCI) for
> testing and development of PCI passthrough on ARM.
> 
> Since PCI passthrough on ARM is still work in progress at this time, make it
> depend on EXPERT.

While preparing the patch for committing, I noticed that HAS_PASSTHROUGH 
will now allow the user to select one of the IOMMU quarantine options.

There are three of them right now:
   1. none
   2. basic (i.e. faulting)
   3. scratch page

The latter is unlikely to work on Arm because we don't setup the scratch 
page. AFAIU, for that, we would need to implement the callback 
quarantine_init().

I would expect 1 and 2 work. That said, I think 1. would behave like 2. 
because on Arm the device should not be automatically re-assigned to 
dom0. I know this is correct for platform device, but will it be valid 
for PCI as well?

Overall, before enabling HAS_PASSTHROUGH, I think there would be a bit 
of tweaking necessary to at least prevent a user to select the option 3 
(either via Kconfig or the command line). And possibly update the 
Kconfig for IOMMU_QUARANTINE to reflect the behavior on Arm.

Cheers,
Stewart Hildebrand July 18, 2023, 5:35 p.m. UTC | #5
On 7/13/23 14:40, Julien Grall wrote:
> Hi Stewart,
> 
> On 07/07/2023 02:47, Stewart Hildebrand wrote:
>> From: Rahul Singh <rahul.singh@arm.com>
>>
>> Setting CONFIG_PCI_PASSTHROUGH=y will enable PCI passthrough on ARM, even though
>> the feature is not yet complete in the current upstream codebase. The purpose of
>> this is to make it easier to enable the necessary configs (HAS_PCI, HAS_VPCI) for
>> testing and development of PCI passthrough on ARM.
>>
>> Since PCI passthrough on ARM is still work in progress at this time, make it
>> depend on EXPERT.
> 
> While preparing the patch for committing, I noticed that HAS_PASSTHROUGH
> will now allow the user to select one of the IOMMU quarantine options.
> 
> There are three of them right now:
>   1. none
>   2. basic (i.e. faulting)
>   3. scratch page
> 
> The latter is unlikely to work on Arm because we don't setup the scratch
> page. AFAIU, for that, we would need to implement the callback
> quarantine_init().
> 
> I would expect 1 and 2 work. That said, I think 1. would behave like 2.
> because on Arm the device should not be automatically re-assigned to
> dom0. I know this is correct for platform device, but will it be valid
> for PCI as well?

In a system with dom0 where the guest is created from the xl toolstack, we rely on "xl pci-assignable-add". Upon domain destruction, the device automatically gets assigned to domIO. However, there's nothing preventing a user from attempting to invoke "xl pci-assignable-remove", which should assign the device back to dom0, but it is not automatic.

With dom0less, PCI devices are not deassigned from the domU.

> Overall, before enabling HAS_PASSTHROUGH, I think there would be a bit
> of tweaking necessary to at least prevent a user to select the option 3
> (either via Kconfig or the command line). And possibly update the
> Kconfig for IOMMU_QUARANTINE to reflect the behavior on Arm.

OK
Julien Grall July 20, 2023, 9:20 a.m. UTC | #6
Hi,

On 18/07/2023 18:35, Stewart Hildebrand wrote:
> On 7/13/23 14:40, Julien Grall wrote:
>> Hi Stewart,
>>
>> On 07/07/2023 02:47, Stewart Hildebrand wrote:
>>> From: Rahul Singh <rahul.singh@arm.com>
>>>
>>> Setting CONFIG_PCI_PASSTHROUGH=y will enable PCI passthrough on ARM, even though
>>> the feature is not yet complete in the current upstream codebase. The purpose of
>>> this is to make it easier to enable the necessary configs (HAS_PCI, HAS_VPCI) for
>>> testing and development of PCI passthrough on ARM.
>>>
>>> Since PCI passthrough on ARM is still work in progress at this time, make it
>>> depend on EXPERT.
>>
>> While preparing the patch for committing, I noticed that HAS_PASSTHROUGH
>> will now allow the user to select one of the IOMMU quarantine options.
>>
>> There are three of them right now:
>>    1. none
>>    2. basic (i.e. faulting)
>>    3. scratch page
>>
>> The latter is unlikely to work on Arm because we don't setup the scratch
>> page. AFAIU, for that, we would need to implement the callback
>> quarantine_init().
>>
>> I would expect 1 and 2 work. That said, I think 1. would behave like 2.
>> because on Arm the device should not be automatically re-assigned to
>> dom0. I know this is correct for platform device, but will it be valid
>> for PCI as well?
> 
> In a system with dom0 where the guest is created from the xl toolstack, we rely on "xl pci-assignable-add". Upon domain destruction, the device automatically gets assigned to domIO.

Ok. To clarify, does this mean any DMA will fault, the same as for 
platform device?

> However, there's nothing preventing a user from attempting to invoke "xl pci-assignable-remove", which should assign the device back to dom0, but it is not automatic.

I don't think we want to fully prevent a user to re-assign a device to 
dom0. But we at least want to avoid re-assigning the device to dom0 by 
default. After that a user can reset the device before it gets 
re-assigned to dom0.

Cheers,
Stewart Hildebrand Oct. 4, 2023, 6:07 p.m. UTC | #7
On 7/20/23 05:20, Julien Grall wrote:
> Hi,
> 
> On 18/07/2023 18:35, Stewart Hildebrand wrote:
>> On 7/13/23 14:40, Julien Grall wrote:
>>> Hi Stewart,
>>>
>>> On 07/07/2023 02:47, Stewart Hildebrand wrote:
>>>> From: Rahul Singh <rahul.singh@arm.com>
>>>>
>>>> Setting CONFIG_PCI_PASSTHROUGH=y will enable PCI passthrough on ARM, even though
>>>> the feature is not yet complete in the current upstream codebase. The purpose of
>>>> this is to make it easier to enable the necessary configs (HAS_PCI, HAS_VPCI) for
>>>> testing and development of PCI passthrough on ARM.
>>>>
>>>> Since PCI passthrough on ARM is still work in progress at this time, make it
>>>> depend on EXPERT.
>>>
>>> While preparing the patch for committing, I noticed that HAS_PASSTHROUGH
>>> will now allow the user to select one of the IOMMU quarantine options.
>>>
>>> There are three of them right now:
>>>    1. none
>>>    2. basic (i.e. faulting)
>>>    3. scratch page
>>>
>>> The latter is unlikely to work on Arm because we don't setup the scratch
>>> page. AFAIU, for that, we would need to implement the callback
>>> quarantine_init().
>>>
>>> I would expect 1 and 2 work. That said, I think 1. would behave like 2.
>>> because on Arm the device should not be automatically re-assigned to
>>> dom0. I know this is correct for platform device, but will it be valid
>>> for PCI as well?
>>
>> In a system with dom0 where the guest is created from the xl toolstack, we rely on "xl pci-assignable-add". Upon domain destruction, the device automatically gets assigned to domIO.
> 
> Ok. To clarify, does this mean any DMA will fault, the same as for
> platform device?

Yes, when the PCI device is assigned to domIO, any DMA from the device will produce a SMMU fault. The value of the quarantine= option doesn't change this behavior.

>> However, there's nothing preventing a user from attempting to invoke "xl pci-assignable-remove", which should assign the device back to dom0, but it is not automatic.
> 
> I don't think we want to fully prevent a user to re-assign a device to
> dom0. But we at least want to avoid re-assigning the device to dom0 by
> default. After that a user can reset the device before it gets
> re-assigned to dom0.
> 
> Cheers,
> 
> -- 
> Julien Grall
diff mbox series

Patch

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 439cc94f3344..4e0cc421ad48 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -190,6 +190,15 @@  config STATIC_SHM
 	help
 	  This option enables statically shared memory on a dom0less system.
 
+config PCI_PASSTHROUGH
+	bool "PCI passthrough" if EXPERT
+	depends on ARM_64
+	select HAS_PCI
+	select HAS_VPCI
+	default n
+	help
+	  This option enables PCI device passthrough
+
 endmenu
 
 menu "ARM errata workaround via the alternative framework"