On 01.08.2025 11:22, Mykyta Poturai wrote:
> From: Edward Pickup <edward.pic...@arm.com>
>
> 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.fance...@arm.com>
> Signed-off-by: Edward Pickup <edward.pic...@arm.com>
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
