Re: [RFC PATCH v1 1/4] arm/pci: PCI setup and PCI host bridge discovery within XEN on ARM.

[Julien Grall <julien@xxxxxxx>]

On 24/07/2020 15:44, Roger Pau Monné wrote:
> > diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
> > new file mode 100644
> > index 0000000000..358508b787
> > --- /dev/null
> > +++ b/xen/arch/arm/pci/Makefile
> > @@ -0,0 +1,4 @@
> > +obj-y += pci.o
> > +obj-y += pci-host-generic.o
> > +obj-y += pci-host-common.o
> > +obj-y += pci-access.o
> The Kconfig option mentions the support being explicitly for ARM64,
> would it be better to place the code in arch/arm/arm64 then?
I don't believe any of the code in this series is very arm64 specific. I 
guess it was just only tested on arm64. So I would rather keep that 
under arm/pci.
> > +
> > +    struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
> > +
> > +    if ( unlikely(!bridge) )
> > +    {
> > +        printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
> > +                sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
> I had a patch to add a custom modifier to out printf format in
> order to handle pci_sbdf_t natively:
>
> https://patchew.org/Xen/20190822065132.48200-1-roger.pau@xxxxxxxxxx/
>
> It missed maintainers Acks and was never committed. Since you are
> doing a bunch of work here, and likely adding a lot of SBDF related
> prints, feel free to import the modifier (%pp) and use in your code
> (do not attempt to switch existing users, or it's likely to get
> stuck again).
I forgot about this patch :/. It would be good to revive it. Which acks 
are you missing?
[...]

> > +static bool __init dt_pci_parse_bus_range(struct dt_device_node *dev,
> > +        struct pci_config_window *cfg)
> > +{
> > +    const __be32 *cells;
> It's my impression that while based on Linux this is not a verbatim
> copy of a Linux file, and tries to adhere with the Xen coding style.
> If so please use uint32_t here.
uint32_t would be incorrect because this is a 32-bit value always in big 
endian. I don't think we have other typedef to show it is a 32-bit BE 
value, so __be32 is the best choice.
[...]

> > +
> > +    if ( acpi_disabled )
> > +        dt_pci_init();
> > +    else
> > +        acpi_pci_init();
> Isn't there an enum or something that tells you whether the system
> description is coming from ACPI or from DT?
>
> This if .. else seems fragile.
This is the common way we do it on Arm.... I would welcome any 
improvement, but I don't think this should be part of this work.
Cheers,

--
Julien Grall