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

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

On 23/03/15 12:24, Vijay Kilari wrote:
>>>   /*
>>>    * 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

Again, what's the purpose of fixing compilation bug in code which will
be remove a patch later?

If you want to fix build errors, you have to fix them correctly. Not
trying to add wrong code in order to make the compiler happy...

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

All GUEST_* and INFO/DEBUG are ratelimited. It might be worth to
introduce ratelimited concept for ERROR.

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

Depend if you care about the reviewers time or not...

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

I have the feeling that counting the number of MSI for a device will be
useful later.

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

Unfortunately not... anyway the for loop is valid. So please drop your
while here.


>>
>>> +       }
>>>
>>> -       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

You modify so heavily the Linux code (pr_* -> its_*) that modifying
again a single line (yes only one) wouldn't hurt...

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

You define GICD_PIDR2_ARCH_REV_MASK with GIC_PIDR2_ARCH_REV_MASK.

So the pre-processor will replace by the correct value.

Regards,

-- 
Julien Grall