Re: [PATCH v7] xen/arm: pci: introduce PCI_PASSTHROUGH Kconfig option

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.06.2025 17:18, Stewart Hildebrand wrote:
> On 6/12/25 10:58, Jan Beulich wrote:
>> On 12.06.2025 15:39, Stewart Hildebrand wrote:
>>> On 6/11/25 01:23, Jan Beulich wrote:
>>>> On 10.06.2025 19:46, Stewart Hildebrand wrote:
>>>>> --- a/xen/arch/arm/Kconfig
>>>>> +++ b/xen/arch/arm/Kconfig
>>>>> @@ -258,6 +258,16 @@ config PARTIAL_EMULATION
>>>>>  
>>>>>  source "arch/arm/firmware/Kconfig"
>>>>>  
>>>>> +config PCI_PASSTHROUGH
>>>>> + bool "PCI passthrough" if EXPERT
>>>>> + depends on ARM_64
>>>>> + select HAS_PCI
>>>>> + select HAS_VPCI
>>>>> + select HAS_VPCI_GUEST_SUPPORT
>>>>
>>>> What about HAS_PASSTHROUGH? Seeing that being selected by MMU, what about
>>>> that connection here?
>>>
>>> Hm, yes. I'm not sure if depends or select would be better?
>>>
>>>     depends on ARM_64 && HAS_PASSTHROUGH
>>>
>>> or
>>>
>>>     select HAS_PASSTHROUGH
>>
>> Actually, thinking of it: HAS_* shouldn't be selected based on user choices,
>> imo. Perhaps unless the HAS_* is a sub-feature of the dependent (as in e.g.
>> "select HAS_VPCI if HVM" as we have it for x86). In which case all the
>> select-s here might be wrong to use.
> 
> How about something like this:
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 3f25da3ca5fd..95a2cd3d006d 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -8,6 +8,8 @@ config ARM_64
>       depends on !ARM_32
>       select 64BIT
>       select HAS_FAST_MULTIPLY
> +     select HAS_VPCI_GUEST_SUPPORT if PCI_PASSTHROUGH

This one may be okay, subject to the Arm maintainers' views.

> +     select HAS_PASSTHROUGH if PCI_PASSTHROUGH

At the first glance this one instead should be ...

> @@ -258,6 +260,12 @@ config PARTIAL_EMULATION
>  
>  source "arch/arm/firmware/Kconfig"
>  
> +config PCI_PASSTHROUGH
> +     bool "PCI passthrough" if EXPERT
> +     depends on ARM_64
> +     help
> +       This option enables PCI device passthrough

... a "depends on" here, imo. Yet that would go against your intentions,
I think, so doing it the way you have it may be okay, subject to how you
justify it.

> --- a/xen/drivers/Kconfig
> +++ b/xen/drivers/Kconfig
> @@ -13,6 +13,7 @@ source "drivers/pci/Kconfig"
>  source "drivers/video/Kconfig"
>  
>  config HAS_VPCI
> +     select HAS_PCI
>       bool

Please can the "bool" remain first?

Jan