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

[Julien Grall <julien@xxxxxxx>]

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 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 */
}

Cheers,

--
Julien Grall