Re: [PATCH v2 3/3] xen: arm: handle PCI DT node ranges and interrupt-map properties

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-03-16 at 16:14 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 12/03/15 17:17, Ian Campbell wrote:
> > These properties are defined in ePAPR (2.3.8 and 2.4.3.1 respectively)
> > and the OpenFirmware PCI Bus Binding Specification (IEEE Std 1275-1994).
> > 
> > This replaces the xgene specific mapping. Tested on Mustang and on a
> > model with a PCI virtio controller.
> > 
> > TODO: Use a helper iterator (e.g. dt_for_each_range) for the ranges
> > property, like is already done for interrupts using
> > dt_for_each_irq_map.
> > 
> > TODO: Should we stop calling map_device for nodes beneath this one
> > (since we should have mapped everything underneath)? I think this is
> > complex for cases which map interrupt but not ranges or vice versa,
> > perhaps meaning we need to recurse on them separately. Maybe we can
> > continue to descend and the mappings may just be harmlessly
> > instantiated twice.
> 
> There is not harm to route twice the same IRQ. Although it's pointless
> to continue.
> 
> How difficult would it be to introduce a new bus?

It's a couple of reasonably trivial hook functions (which one would
naturally just get from Linux) and a struct tying a name to those. Not
too hard.

> > +    if ( dt_device_type_is_equal(dev, "pci") )
> > +        return map_pci_device_ranges(d, dev, ranges, len);
> > +
> > +    printk("Cannot handle ranges for non-PCI device %s type %s\n",
> > +           dt_node_name(dev), dev->type);
> > +
> 
> Is the printk really necessary? It will a spurious log on platform where
> ranges is not empty (midway, arndale, foundation model...).

If the ranges is present and non-empty it's not impossible that we need
to be doing something with it. I'd rather try and figure out how to
whitelist such nodes, perhaps they lack a dev_type completely?

> In general, we should not handle ranges by default as we may overlap the
> mapping done during the domain creation (such as the GIC).
> 
> > +    /* Lets not worry for now... */
> > +    return 0;
> > +}
> > +
> > +static int map_interrupt_to_domain(const struct dt_device_node *dev,
> > +                                   const struct dt_raw_irq *dt_raw_irq,
> > +                                   void *data)
> > +{
> > +    struct domain *d = data;
> > +    struct dt_irq dt_irq;
> > +    int res;
> > +
> > +    res = dt_irq_translate(dt_raw_irq, &dt_irq);
> > +    if ( res < 0 )
> > +    {
> > +        printk(XENLOG_ERR "%s: Failed to translate IRQ: %d\n",
> > +               dt_node_name(dev), res);
> > +        return res;
> > +    }
> > +
> > +    if ( dt_irq.irq < NR_LOCAL_IRQS )
> > +    {
> > +        printk(XENLOG_ERR "%s: IRQ%"PRId32" is not a SPI\n",
> > +               dt_node_name(dev), dt_irq.irq);
> > +        return -EINVAL;
> > +    }
> > +
> > +    /* Setup the IRQ type */
> > +    res = irq_set_spi_type(dt_irq.irq, dt_irq.type);
> > +    if ( res )
> > +    {
> > +        printk(XENLOG_ERR
> > +               "%s: Unable to setup IRQ%"PRId32" to dom%d\n",
> > +               dt_node_name(dev), dt_irq.irq, d->domain_id);
> > +        return res;
> > +    }
> > +
> > +    res = route_irq_to_guest(d, dt_irq.irq, dt_node_name(dev));
> > +    if ( res < 0 )
> > +    {
> > +        printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
> > +               dt_irq.irq, d->domain_id);
> > +        return res;
> > +    }
> > +
> > +    DPRINT("PCI IRQ %u mapped to guest", dt_irq.irq);
> > +
> > +    return 0;
> > +}
> > +
> > +static int map_device_interrupts(struct domain *d, const struct dt_device_node *dev)
> > +{
> > +
> > +    if ( !dt_property_read_bool(dev, "interrupt-map") )
> 
> It's strange to use dt_property_read_bool here and dt_get_property above
> to check the emptiness.
> 
> I prefer the dt_get_property version which is less confusing.
> 
> Anyway, why do you need to check interrupt-map. Can't your new helper
> deal with empty property?

It can, but the code below would log for any non-pci device, which is
undesirable if there is no interrupt-map present.


> 
> > +        return 0; /* No interrupt map to handle */
> > +
> > +    if ( dt_device_type_is_equal(dev, "pci") )
> > +        return dt_for_each_irq_map(dev, &map_interrupt_to_domain, d);
> > +
> > +    printk("Cannot handle interrupt-map for non-PCI device %s type %s\n",
> > +           dt_node_name(dev), dev->type);
> > +
> > +    /* Lets not worry for now... */
> > +    return 0;
> > +}
> > +
> > +
> >  /* Map the device in the domain */
> >  static int map_device(struct domain *d, struct dt_device_node *dev)
> >  {
> > @@ -1025,6 +1173,14 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
> >          }
> >      }
> >  
> > +    res = map_device_ranges(d, dev);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = map_device_interrupts(d, dev);
> 
> The name of the function doesn't make much sense. We already map the
> interrupt above (see platform get_irq).

child_interrupt perhaps?

Ian.