Re: [PATCH v1 1/3] arm/pci: Add pci-scan boot argument

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.08.2025 11:22, Mykyta Poturai wrote:
> From: Edward Pickup <Edward.Pickup@xxxxxxx>
> 
> This patch adds a Xen boot arguments that, if enabled, causes a call to
> existing code to scan pci devices enumerated by the firmware.
> 
> This patch also makes an existing debug function viewable outside its
> translation unit, and uses this to dump the PCI devices found.
> The debug message is controlled by config DEBUG.
> 
> Additionally, this patch modifies segment loading to ensure that PCI
> devices on other segments are properly found.
> 
> This will be needed ahead of dom0less support for pci passthrough on
> arm.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Signed-off-by: Edward Pickup <Edward.Pickup@xxxxxxx>

Considering the From: above and this order of S-o-b: Who is it really that
was the original author here?

> --- a/xen/arch/arm/include/asm/pci.h
> +++ b/xen/arch/arm/include/asm/pci.h
> @@ -23,6 +23,7 @@
>  #define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
>  
>  extern bool pci_passthrough_enabled;
> +extern bool pci_scan_enabled;

With the variable non-static, ...

> @@ -128,6 +129,11 @@ static always_inline bool 
> is_pci_passthrough_enabled(void)
>      return pci_passthrough_enabled;
>  }
>  
> +static inline bool is_pci_scan_enabled(void)
> +{
> +    return pci_scan_enabled;
> +}
> +
>  void arch_pci_init_pdev(struct pci_dev *pdev);
>  
>  int pci_get_new_domain_nr(void);
> @@ -155,6 +161,11 @@ bool arch_pci_device_physdevop(void);
>  
>  #else   /*!CONFIG_HAS_PCI*/
>  
> +static inline bool is_pci_scan_enabled(void)
> +{
> +    return false;
> +}

... what's the point of the wrappers? Constrain the variable as such to
HAS_PCI=y, and use "#define pci_scan_enabled false" in the opposite case.
Just like we do elsewhere in a number of cases.

> --- a/xen/arch/arm/pci/pci.c
> +++ b/xen/arch/arm/pci/pci.c
> @@ -91,8 +91,13 @@ bool arch_pci_device_physdevop(void)
>  bool __read_mostly pci_passthrough_enabled;
>  boolean_param("pci-passthrough", pci_passthrough_enabled);
>  
> +/* By default pci scan is disabled. */
> +bool __read_mostly pci_scan_enabled;

__ro_after_init?

> +boolean_param("pci-scan", pci_scan_enabled);
> +
>  static int __init pci_init(void)
>  {
> +    int ret;
>      /*

Nit: Blank line please between declaration(s) and statement(s).

> @@ -104,9 +109,26 @@ static int __init pci_init(void)
>          panic("Could not initialize PCI segment 0\n");
>  
>      if ( acpi_disabled )
> -        return dt_pci_init();
> +        ret = dt_pci_init();
>      else
> -        return acpi_pci_init();
> +        ret = acpi_pci_init();
> +
> +    if ( ret < 0 )
> +        return ret;
> +
> +    if ( is_pci_scan_enabled() )
> +    {
> +        ret = scan_pci_devices();
> +
> +        if ( ret < 0 )
> +            return ret;
> +
> +#ifdef DEBUG
> +        dump_pci_devices('c');
> +#endif

If I was a maintainer of this code, I would request such to be dropped.
Or if there was a good reason to have such, I think it would want to be
arch-independent.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1384,7 +1384,7 @@ static int cf_check _dump_pci_devices(struct pci_seg 
> *pseg, void *arg)
>      return 0;
>  }
>  
> -static void cf_check dump_pci_devices(unsigned char ch)
> +void cf_check dump_pci_devices(unsigned char ch)

Note the cf_check here. It, for some reason, ...

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -217,6 +217,7 @@ static always_inline bool pcidevs_trylock(void)
>  bool pci_known_segment(u16 seg);
>  bool pci_device_detect(u16 seg, u8 bus, u8 dev, u8 func);
>  int scan_pci_devices(void);
> +void dump_pci_devices(unsigned char ch);

... needs reproducing on the declaration. What about x86 though? It'll end up
as a non-static function with no caller outside of the defining CU, hence
violating some Misra rule.

Jan