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