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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Julien!

On 16.11.21 20:48, Julien Grall wrote:
> 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".
>
We may have "bridge -> bridge -> device" topology as well.
So, I prefer to have the check as it is.
>> +             */
>>               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.
Yes, this sounds reasonable, I will add this change and print an error message 
so it is
easier to understand what Xen doesn't like (it took me a while to debug and 
understand
why I have "(XEN) Device tree generation failed (-22).")
>
> Cheers,
>
Thank you,
Oleksandr