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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Mon, Mar 16, 2015 at 7:50 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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?

On receiving MAPD/MAPVI command

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel