Re: [Xen-devel] [RFC PATCH v2 06/22] xen/arm: its: Port ITS driver to xen

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

Hi Julien,

On Fri, Mar 20, 2015 at 8:36 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hello Vijay,
>
> On 19/03/2015 14:37, vijay.kilari@xxxxxxxxx wrote:
>>
>>   static LIST_HEAD(its_nodes);
>>   static DEFINE_SPINLOCK(its_lock);
>> -static struct device_node *gic_root_node;
>> -static struct rdists *gic_rdists;
>> +static struct dt_device_node *gic_root_node;
>> +static struct rdist_prop  *gic_rdists;
>>
>> -#define gic_data_rdist()               (raw_cpu_ptr(gic_rdists->rdist))
>> -#define gic_data_rdist_rd_base()       (gic_data_rdist()->rd_base)
>> +#define gic_data_rdist()               (per_cpu(rdist,
>> smp_processor_id()))
>
>
> Again why didn't you return a pointer here? It would have been avoid some
> confusing changes (s/->/./) in the code.
>
> #define gic_data_rdist(&per_cpu(rdist, smp_processor_id))
>
>> +#define gic_data_rdist_rd_base()       (per_cpu(rdist,
>> smp_processor_id()).rbase)
>
>
> That would avoid this change too.

OK. Let me try

>
>>
>>   /*
>>    * ITS command descriptors - parameters to be encoded in a command
>> @@ -228,10 +243,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 = ilog2(desc->its_mapd_cmd.dev->nr_ites);
>> +       u8 size = max(fls(desc->its_mapd_cmd.dev->nr_ites) - 1, 1);
>
>
> ilog2 on an uint32_t is defined as fls(val) - 1. Where does the max come
> from?
>
> IHMO, I would define ilog2 in Xen, that would be easier.

Anyway this code is not used later. So I don't bother much about this

>
>>
>> -       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 = ROUNDUP(itt_addr, ITS_ITT_ALIGN);
>
>
> This file use the Linux coding style. Please use hard tab.
>
>>
>>         its_encode_cmd(cmd, GITS_CMD_MAPD);
>>         its_encode_devid(cmd, desc->its_mapd_cmd.dev->device_id);
>> @@ -348,7 +363,7 @@ static struct its_cmd_block *its_allocate_entry(struct
>> its_node *its)
>>         while (its_queue_full(its)) {
>>                 count--;
>>                 if (!count) {
>> -                       pr_err_ratelimited("ITS queue not draining\n");
>> +                       its_err("ITS queue not draining\n");
>
>
> its_err and pr_err_ratelimited are not the same things. The former is not
> ratelimited.
>
> AFAICT this function will be accessible in someway from the guest. It would
> be possible to DOS Xen when sending a command.

Any equivalent ratelimited function in Xen?

>
>>                         return NULL;
>>                 }
>>                 cpu_relax();
>> @@ -380,7 +395,7 @@ static void its_flush_cmd(struct its_node *its, struct
>> its_cmd_block *cmd)
>>          * the ITS.
>>          */
>>         if (its->flags & ITS_FLAGS_CMDQ_NEEDS_FLUSHING)
>> -               __flush_dcache_area(cmd, sizeof(*cmd));
>> +               clean_and_invalidate_dcache_va_range(cmd, sizeof(*cmd));
>>         else
>>                 dsb(ishst);
>>   }
>> @@ -402,7 +417,7 @@ static void its_wait_for_range_completion(struct
>> its_node *its,
>>
>>                 count--;
>>                 if (!count) {
>> -                       pr_err_ratelimited("ITS queue timeout\n");
>> +                       its_err("ITS queue timeout\n");
>
>
> Ditto
>
> [..]
>
>> -static void its_send_inv(struct its_device *dev, u32 event_id)
>> +/* TODO: Remove static for the sake of compilation */
>> +void its_send_inv(struct its_device *dev, u32 event_id)
>
>
> Rather than changing the prototype. Would it be possible to #if 0 the
> function? It would be easier to keep track change.

Does not matter much. Anyway I can try as you wish

>
>> -static int its_alloc_device_irq(struct its_device *dev, irq_hw_number_t
>> *hwirq)
>> +/* TODO: Remove static for the sake of compilation */
>> +int its_alloc_device_irq(struct its_device *dev, int *hwirq)
>>   {
>>         int idx;
>>
>> @@ -1139,6 +1169,8 @@ static int its_alloc_device_irq(struct its_device
>> *dev, irq_hw_number_t *hwirq)
>>         return 0;
>>   }
>>
>> +/* pci and msi handling no more required here */
>
>
> Hmmm why?

This code is not required. we don't have msi_domain_ops

>
>
> Already said on V1: of_device_id and dt_device_match are compatible. If you
> change the name it will work too...
>
>> +       while ((np = dt_find_matching_node(np, its_device_ids)))
>> +       {
>> +               if (!dt_find_property(np, "msi-controller", NULL))
>> +               continue;
>
>
> In your cover letter, you said you support multiple ITS node but this piece
> of code show that it's not the case...

   If I remember  correctly, this is later updated
>
>> +       }
>>
>> -       for (np = of_find_matching_node(node, its_device_id); np;
>> -            np = of_find_matching_node(np, its_device_id)) {
>> -               its_probe(np, parent_domain);
>
>
> The for loop was perfect, why did you drop it?
>
>> +       if (np) {
>> +               its_probe(np);
>>         }
>>
>>         if (list_empty(&its_nodes)) {
>> -               pr_warn("ITS: No ITS available, not enabling LPIs\n");
>> +               its_warn("ITS: No ITS available, not enabling LPIs\n");
>>                 return -ENXIO;
>>         }
>>
>> diff --git a/xen/include/asm-arm/gic_v3_defs.h
>> b/xen/include/asm-arm/gic_v3_defs.h
>> index 4e64b56..f8bac52 100644
>> --- a/xen/include/asm-arm/gic_v3_defs.h
>> +++ b/xen/include/asm-arm/gic_v3_defs.h
>> @@ -59,11 +59,12 @@
>>   #define GICR_WAKER_ProcessorSleep    (1U << 1)
>>   #define GICR_WAKER_ChildrenAsleep    (1U << 2)
>>
>> -#define GICD_PIDR2_ARCH_REV_MASK     (0xf0)
>> +#define GIC_PIDR2_ARCH_REV_MASK      (0xf0)
>> +#define GICD_PIDR2_ARCH_REV_MASK     GIC_PIDR2_ARCH_REV_MASK
>
>
> Why do you define GIC_PIDR2_ARCH_REV_MASK? It's not consistent with the
> other part of the code.

Linux code uses GIC_PIDR2_ARCH_REV_MASK

>
>>   #define GICD_PIDR2_ARCH_REV_SHIFT    (0x4)
>>   #define GICD_PIDR2_ARCH_GICV3        (0x3)
>>
>> -#define GICR_PIDR2_ARCH_REV_MASK     GICD_PIDR2_ARCH_REV_MASK
>> +#define GICR_PIDR2_ARCH_REV_MASK     GIC_PIDR2_ARCH_REV_MASK
>
>
> Why this change? GICD_PIDR2_ARCH_REV_MASK still exists...

gic-v3.c still uses this.

Regards
Vijay

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