|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v2 06/22] xen/arm: its: Port ITS driver to xen
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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |