Re: [PATCH v6 2/7] xen/arm: add pci-domain for disabled devices

[Julien Grall <julien@xxxxxxx>]

Hi Oleksander,

On 05/11/2021 06:33, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>
> If a PCI host bridge device is present in the device tree, but is
> disabled, then its PCI host bridge driver was not instantiated.
> This results in the failure of the pci_get_host_bridge_segment()
> and the following panic during Xen start:
>
> (XEN) Device tree generation failed (-22).
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Could not set up DOM0 guest OS
> (XEN) ****************************************
>
> Fix this by adding "linux,pci-domain" property for all device tree nodes
> which have "pci" device type, so we know which segments will be used by
> the guest for which bridges.
>
> Fixes: 4cfab4425d39 ("xen/arm: Add linux,pci-domain property for hwdom if not 
> available.")
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>
> ---
> New in v6
> ---
>   xen/arch/arm/domain_build.c        | 15 ++++++++++++++-
>   xen/arch/arm/pci/pci-host-common.c |  2 +-
>   xen/include/asm-arm/pci.h          |  8 ++++++++
>   3 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 491f5e2c316e..f7fcb1400c19 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -753,9 +753,22 @@ static int __init write_properties(struct domain *d, 
> struct kernel_info *kinfo,
>           {
>               uint16_t segment;
>   
> +            /*
> +             * The node doesn't have "linux,pci-domain" property and it is
> +             * possible that:
> +             *  - Xen only has drivers for a part of the host bridges
> +             *  - some host bridges are disabled
> +             * Make sure we insert the correct "linux,pci-domain" property
> +             * in any case, so we know which segments will be used
> +             * by Linux for which bridges.
The check above will check the node type is "pci". AFAICT, this would 
also cover PCI devices. I am not aware of any issue to add 
"linux,pci-domain" for them. However, this feels a bit odd.
From my understanding, a PCI device would always be described as a 
child of the hostbridges. So I would rework the 'if' to also check if 
the parent type is not "pci".
> +             */
>               res = pci_get_host_bridge_segment(node, &segment);
>               if ( res < 0 )
> -                return res;
> +            {
> +                segment = pci_get_new_domain_nr();
> +                printk(XENLOG_DEBUG "Assigned segment %d to %s\n",
> +                       segment, node->full_name);
> +            }
>   
>               res = fdt_property_cell(kinfo->fdt, "linux,pci-domain", segment);
>               if ( res )
> diff --git a/xen/arch/arm/pci/pci-host-common.c 
> b/xen/arch/arm/pci/pci-host-common.c
> index d8cbaaaba654..47104b22b221 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -137,7 +137,7 @@ void pci_add_host_bridge(struct pci_host_bridge *bridge)
>       list_add_tail(&bridge->node, &pci_host_bridges);
>   }
>   
> -static int pci_get_new_domain_nr(void)
> +int pci_get_new_domain_nr(void)
>   {
>       return atomic_inc_return(&domain_nr);
We may have a DT where only the nodes used by Xen have 
"linux,pci-domain". In this case, we would end up to return 0, 1... 
which may have already been used.
This will probably make Linux unhappy. So I would return -1 here if 
use_dt_domains == 1. The caller would also need to bail out if -1 is 
returned.
Cheers,

--
Julien Grall