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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>]

Hi, Julien!

On 23.11.21 19:15, Julien Grall wrote:
> Hi,
>
> On 23/11/2021 16:44, Oleksandr Andrushchenko wrote:
>> On 23.11.21 18:05, Julien Grall wrote:
>>> Hi Oleksandr,
>>>
>>> On 23/11/2021 06:31, Oleksandr Andrushchenko wrote:
>>>>
>>>>
>>>> On 22.11.21 19:17, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 22/11/2021 16:23, Oleksandr Andrushchenko wrote:
>>>>>> On 22.11.21 17:29, Julien Grall wrote:
>>>>>>> I would prefer to go with my way. This can be refined in the future if we find Device-Tree that matches what you wrote.
>>>>>> Ok, so just to make it clear:
>>>>>>     >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"
>>>>>
>>>>> That's correct. Thank you!
>>>> Ok, so how about
>>>>        if ( is_pci_passthrough_enabled() && dt_device_type_is_equal(node, "pci") )
>>>>        {
>>>>            bool skip = false;
>>>>
>>>>            /*
>>>>             * If the parent is also a "pci" device, then "linux,pci-domain"
>>>>             * should already be there, so nothing to do then.
>>>>             */
>>>
>>> This comment is a bit confusing.
>> Do you have something on your mind?
>
> Yes, explain that we only want to cover hostbridge (see my reply on the previous answer).
I guess this will be explained by the comment to handle_linux_pci_domain below
>
>>> I think what matter if the parent is a "pci" device, then the current node must not be a hostbridge. So we can skip it.
>> By skipping you only mean we do not need to add/assign "linux,pci-domain", right?
>
> Yes.
>
>>> However...
>>>
>>>>            if ( node->parent && dt_device_type_is_equal(node->parent, "pci") )
>>>>                skip = true;
>>>>
>>>>            if ( !skip && !dt_find_property(node, "linux,pci-domain", NULL) )
>>>>            {
>>>> I played with a single if and it looks scary...
>>>
>>> ... how about introducing an helper that will return true if this node is likely an hostbridge?
>> This is yet a single use of such a check: why would we need a helper and what would that
>> helper do?
>
> I like splitting code in multiple functions even if they are only called once because this:
>   1) keeps the functions line count small
>   2) easier to understand what is the purpose of the check
>
> In fact, I would actually move the handling for "linux,pci-domain" in a separate helper. Something like:
>
> /*
>  * When PCI passthrough is available we want to keep the
>  * "linux,pci-domain" in sync for every hostbridge.
>  *
>  * Xen may not have a driver for all the hostbridges. So we have
>  * to write an heuristic to detect whether a device node describes
>  * an hostbridge.
>  *
>  * The current heuristic assumes that a device is an hostbridge
>  * if the type is "pci" and then parent type is not "pci".
>  */
> static int handle_linux_pci_domain(struct dt_device *node)
> {
>         if ( !is_pci_passthrough_enabled() )
>           return 0;
>
>     if ( !dt_device_type_is_equal(node, "pci") )
>           return 0;
>
>         if ( node->parent && dt_device_type_is_equal(node->parent) )
>           return 0;
>
>         if ( dt_find_property(node, "linux,pci-domain", NULL )
>           return 0;
>
>         /* Allocate and create the linux,pci-domain */
> }
>
I'm fine with this, will move, thanks
> Cheers,
>
Thank you,
Oleksandr