Re: [PATCH] xen/arm: do not try to add pci-domain for disabled devices

[Julien Grall <julien@xxxxxxx>]

Hi Oleksandr,

On 27/10/2021 09:37, 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 following panic during Xen start:
>
> (XEN) Device tree generation failed (-22).
It would good to clarify in the commit message where the error is coming 
from. I think this is from pci_get_host_bridge_segment().
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Could not set up DOM0 guest OS
> (XEN) ****************************************
>
> Fix this by not adding linux,pci-domain property for hwdom if it is
> neither available nor device enabled.
From my reading of the binding [1], the property should be present in 
all the hostbridges if one specify it. IOW, I think the property should 
also be added for hostbridges that are not available.
AFAICT, Linux will ignore hostbridge that are not available. But it 
feels to me we are twisting the rule. So, could we consider to allocate 
an unused number?
> Fixes: 4cfab4425d39 ("xen/arm: Add linux,pci-domain property for hwdom if not 
> available.")
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> ---
>   xen/arch/arm/domain_build.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 124ade09123a..beeecf84a209 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -747,7 +747,8 @@ static int __init write_properties(struct domain *d, struct 
> kernel_info *kinfo,
>               return res;
>       }
>   
> -    if ( is_pci_passthrough_enabled() && dt_device_type_is_equal(node, "pci") )
> +    if ( is_pci_passthrough_enabled() && dt_device_type_is_equal(node, "pci") 
> &&
From my understanding, PCI device described in the DT would also have 
'type="pci"'. So we would also return an error because we don't have a 
corresponding hostbridge.
I think this check should be replaced with device_get_class(node) == 
DEVICE_PCI (confusingly DEVICE_PCI indicates that this is an 
hostbridge...). Although, I would consider to pass the device class as a 
parameter of write_properties() to avoid calling device_class() multiple 
time (it is already used twice in handle_node()).
I am aware this is not a bug introduced by your patch, but I think this 
should be dealt in this patch as well.
> +         dt_device_is_available(node) )
Shouldn't you also check that the hostbridge wasn't passthrough-ed?

>       {
>           if ( !dt_find_property(node, "linux,pci-domain", NULL) )
>           {
Cheers,

[1] Documentation/devicetree/bindings/pci/pci.txt

--
Julien Grall