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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

Hi Julien,

On Fri, Mar 13, 2015 at 5:16 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hello Vijay,
>
> On 02/03/2015 12:30, vijay.kilari@xxxxxxxxx wrote:
>>
>> @@ -228,10 +242,10 @@ static struct its_collection
>> *its_build_mapd_cmd(struct its_cmd_block *cmd,
>>                                                    struct its_cmd_desc
>> *desc)
>>   {
>>       unsigned long itt_addr;
>> -    u8 size = max(order_base_2(desc->its_mapd_cmd.dev->nr_ites), 1);
>> +    u8 size = max(fls(desc->its_mapd_cmd.dev->nr_ites) - 1, 1);
>
>
> You may want to give a look to get_count_order.
>
>>
>> -    itt_addr = virt_to_phys(desc->its_mapd_cmd.dev->itt);
>> -    itt_addr = ALIGN(itt_addr, ITS_ITT_ALIGN);
>> +    itt_addr = __pa(desc->its_mapd_cmd.dev->itt);
>> +    itt_addr = ((itt_addr) + (ITS_ITT_ALIGN - 1)) & ~(ITS_ITT_ALIGN - 1);
>
>
> You can use ROUNDUP.
>
> [..]
>
>> @@ -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.

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



>>
>> -
>> -
>> -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.

Regards
Vijay

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