Re: [RFC PATCH 03/19] xen/arm: its: Port ITS driver to xen

[Julien Grall <julien.grall@linaro.org>]

On 16/03/15 14:06, Vijay Kilari wrote:
>>> @@ -343,17 +357,23 @@ static int its_queue_full(struct its_node *its)
>>>   static struct its_cmd_block *its_allocate_entry(struct its_node *its)
>>>   {
>>>       struct its_cmd_block *cmd;
>>> -    u32 count = 1000000;    /* 1s! */
>>> +    bool_t timeout = 0;
>>> +    s_time_t deadline = NOW() + MILLISECS(1000);
>>>
>>>       while (its_queue_full(its)) {
>>> -        count--;
>>> -        if (!count) {
>>> -            pr_err_ratelimited("ITS queue not draining\n");
>>> -            return NULL;
>>> +       if ( NOW() > deadline )
>>> +        {
>>> +            timeout = 1;
>>> +            break;
>>>           }
>>>           cpu_relax();
>>>           udelay(1);
>>>       }
>>> +    if ( timeout )
>>> +    {
>>> +        its_err("ITS queue not draining\n");
>>> +        return NULL;
>>> +    }
>>
>>
>> Why do you need to modify the loop? It looks like to me it will work Xen
>> too.
> 
> I remember we used similar approach for GICv3.

The GICv3 driver is not really imported from Linux... The loop solution
was suggested by Ian in order to avoid an infinite loop.

Anyway, that doesn't make a reason to change a valid code...

>>
>>>   /*
>>> - * irqchip functions - assumes MSI, mostly.
>>> - */
>>> -
>>> -static void lpi_set_config(struct its_device *its_dev, u32 hwirq,
>>> -               u32 id, int enable)
>>> -{
>>> -    u8 *cfg = page_address(gic_rdists->prop_page) + hwirq - 8192;
>>> -
>>> -    if (enable)
>>> -        *cfg |= LPI_PROP_ENABLED;
>>> -    else
>>> -        *cfg &= ~LPI_PROP_ENABLED;
>>> -
>>> -    /*
>>> -     * Make the above write visible to the redistributors.
>>> -     * And yes, we're flushing exactly: One. Single. Byte.
>>> -     * Humpf...
>>> -     */
>>> -    if (gic_rdists->flags & RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING)
>>> -        __flush_dcache_area(cfg, sizeof(*cfg));
>>> -    else
>>> -        dsb(ishst);
>>> -    its_send_inv(its_dev, id);
>>> -}
>>> -
>>> -static inline u16 its_msi_get_entry_nr(struct msi_desc *desc)
>>> -{
>>> -    return desc->msi_attrib.entry_nr;
>>> -}
>>> -
>>> -static void its_mask_irq(struct irq_data *d)
>>> -{
>>> -    struct its_device *its_dev = irq_data_get_irq_handler_data(d);
>>> -    u32 id;
>>> -
>>> -    /* If MSI, propagate the mask to the RC */
>>> -    if (IS_ENABLED(CONFIG_PCI_MSI) && d->msi_desc) {
>>> -        id = its_msi_get_entry_nr(d->msi_desc);
>>> -        mask_msi_irq(d);
>>> -    } else {
>>> -        id = d->hwirq;
>>> -    }
>>> -
>>> -    lpi_set_config(its_dev, d->hwirq, id, 0);
>>> -}
>>> -
>>> -static void its_unmask_irq(struct irq_data *d)
>>> -{
>>> -    struct its_device *its_dev = irq_data_get_irq_handler_data(d);
>>> -    u32 id;
>>> -
>>> -    /* If MSI, propagate the unmask to the RC */
>>> -    if (IS_ENABLED(CONFIG_PCI_MSI) && d->msi_desc) {
>>> -        id = its_msi_get_entry_nr(d->msi_desc);
>>> -        unmask_msi_irq(d);
>>> -    } else {
>>> -        id = d->hwirq;
>>> -    }
>>> -
>>> -    lpi_set_config(its_dev, d->hwirq, id, 1);
>>> -}
>>> -
>>> -static void its_eoi_irq(struct irq_data *d)
>>> -{
>>> -    gic_write_eoir(d->hwirq);
>>> -}
>>> -
>>> -static int its_set_affinity(struct irq_data *d, const struct cpumask
>>> *mask_val,
>>> -                            bool force)
>>> -{
>>> -    unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
>>> -    struct its_device *its_dev = irq_data_get_irq_handler_data(d);
>>> -    struct its_collection *target_col;
>>> -    u32 id;
>>> -
>>> -    if (cpu >= nr_cpu_ids)
>>> -        return -EINVAL;
>>> -
>>> -    target_col = &its_dev->its->collections[cpu];
>>> -    if (IS_ENABLED(CONFIG_PCI_MSI) && d->msi_desc)
>>> -        id = its_msi_get_entry_nr(d->msi_desc);
>>> -    else
>>> -        id = d->hwirq;
>>> -    its_send_movi(its_dev, target_col, id);
>>> -    its_dev->collection = target_col;
>>> -
>>> -    return IRQ_SET_MASK_OK;
>>> -}
>>> -
>>> -static struct irq_chip its_irq_chip = {
>>> -    .name            = "ITS",
>>> -    .irq_mask        = its_mask_irq,
>>> -    .irq_unmask        = its_unmask_irq,
>>> -    .irq_eoi        = its_eoi_irq,
>>> -    .irq_set_affinity    = its_set_affinity,
>>> -};
>>> -
>>
>>
>> Most of those callbacks seems useful to me. Why did you drop them?
> 
> There is no irq_chip in xen. So thought of removing in completely and
> adding as separate patch

What the point to remove and re-add code later? Except making more
difficult to review.

If this code is valid please move in an #if 0/#endif

> 
>>>
>>> -
>>> -
>>> -static int its_msi_get_vec_count(struct pci_dev *pdev, struct msi_desc
>>> *desc)
>>> -{
>>> -#ifdef CONFIG_PCI_MSI
>>> -    if (desc->msi_attrib.is_msix)
>>> -        return pci_msix_vec_count(pdev);
>>> -    else
>>> -        return pci_msi_vec_count(pdev);
>>> -#else
>>> -    return -EINVAL;
>>> -#endif
>>> -}
>>> -
>>> -int pci_requester_id(struct pci_dev *dev);
>>> -static int its_msi_setup_irq(struct msi_chip *chip,
>>> -                             struct pci_dev *pdev,
>>> -                             struct msi_desc *desc)
>>> -{
>>> -    struct its_node *its = container_of(chip, struct its_node, msi_chip);
>>> -    struct its_device *its_dev;
>>> -    struct msi_msg msg;
>>> -    unsigned int irq;
>>> -    u64 addr;
>>> -    int hwirq;
>>> -    int err;
>>> -    u32 dev_id = pci_requester_id(pdev);
>>> -    u32 vec_nr;
>>> -
>>> -    its_dev = its_find_device(its, dev_id);
>>> -    if (!its_dev) {
>>> -        int nvec = its_msi_get_vec_count(pdev, desc);
>>> -        if (WARN_ON(nvec <= 0))
>>> -            return nvec;
>>> -        its_dev = its_create_device(its, dev_id, nvec);
>>> -    }
>>> -    if (!its_dev)
>>> -        return -ENOMEM;
>>> -    vec_nr = its_msi_get_entry_nr(desc);
>>> -    err = its_alloc_device_irq(its_dev, vec_nr, &hwirq, &irq);
>>> -    if (err)
>>> -        return err;
>>> -
>>> -    irq_set_msi_desc(irq, desc);
>>> -    irq_set_handler_data(irq, its_dev);
>>> -
>>> -    addr = its->phys_base + GITS_TRANSLATER;
>>> -
>>> -    msg.address_lo        = (u32)addr;
>>> -    msg.address_hi        = (u32)(addr >> 32);
>>> -    msg.data        = vec_nr;
>>> -
>>> -    write_msi_msg(irq, &msg);
>>> -    return 0;
>>> -}
>>> -
>>> -static void its_msi_teardown_irq(struct msi_chip *chip, unsigned int irq)
>>> -{
>>> -    struct irq_data *d = irq_get_irq_data(irq);
>>> -    struct its_device *its_dev = irq_data_get_irq_handler_data(d);
>>> -
>>> -    BUG_ON(d->hwirq < its_dev->lpi_base ||        /* OMG! */
>>> -           d->hwirq > (its_dev->lpi_base + its_dev->nr_lpis));
>>> -
>>> -    /* Stop the delivery of interrupts */
>>> -    its_send_discard(its_dev, its_msi_get_entry_nr(d->msi_desc));
>>> -
>>> -    /* Mark interrupt index as unused, and clear the mapping */
>>> -    clear_bit(d->hwirq - its_dev->lpi_base, its_dev->lpi_map);
>>> -    irq_dispose_mapping(irq);
>>> -
>>> -    /* If all interrupts have been freed, start mopping the floor */
>>> -    if (bitmap_empty(its_dev->lpi_map, its_dev->nr_lpis)) {
>>> -        its_lpi_free(its_dev->lpi_map,
>>> -                     its_dev->lpi_base,
>>> -                     its_dev->nr_lpis);
>>> -
>>> -        /* Unmap device/itt */
>>> -        its_send_mapd(its_dev, 0);
>>> -        its_free_device(its_dev);
>>> -    }
>>> -}
>>> -
>>
>>
>> Those 2 functions seems useful for me. Why did you drop them?
> 
> These are callbacks registered for msi_chip in linux, Since we have removed
> msi_chip from its_node structure, these functions are supposed to be removed.

How do you register/unregister MSI in this case?

Regards,

-- 
Julien Grall