Re: [PATCH v5 05/22] xen/arm: ITS: Port ITS driver to Xen

[Vijay Kilari <vijay.kilari@gmail.com>]

Hi Julien,

  Can you please explain what is the problem with making a function
non-static for compilation purpose and later make it static when used?
In anycase we are going to merge all the patches at once.

Regards
Vijay


On Tue, Jul 28, 2015 at 10:16 PM, Julien Grall <julien.grall@citrix.com> wrote:
> Hi Vijay,
>
> On 27/07/15 12:11, vijay.kilari@gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> The linux driver is based on 4.1 with below commit id
>>
>> 3ad2a5f57656a14d964b673a5a0e4ab0e583c870
>
> This doesn't include commit 591e5bec13f15feb13fc445b6c9c59954711c4ac
> "irqchip/gicv3-its: Fix mapping of LPIs to collections".
>
> On the version 4 of this series, Ian [1] said that it would be very nice
> to have a similar approach in Xen. I would like to see it too.
>
> [..]
>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> new file mode 100644
>> index 0000000..ba4110f
>> --- /dev/null
>> +++ b/xen/arch/arm/gic-v3-its.c
>
> [..]
>
>> +#define its_err(fmt, ...) its_print(XENLOG_ERR, fmt, ## __VA_ARGS__)
>> +/* TODO: ratelimit for Xen messages */
>> +#define its_err_ratelimited(fmt, ...)                                 \
>> +    its_print(XENLOG_ERR, fmt, ## __VA_ARGS__)
>
> The macro its_err_ratelimited is mostly used in function that are
> accessible by the guest (via enable/disable LPI...). Which means that a
> guest could theoretically hit the problem and DOS xen.
>
> I would use XENLOG_G_ERR to have a rate limited until we fix it
> correctly for XENLOG_ERR.
>
> [..]
>
>> +#ifdef DEBUG_GIC_ITS
>> +void dump_cmd(its_cmd_block *cmd)
>
> static void
>
> and const its_cmd_block *cmd
>
>> +{
>> +    printk(XENLOG_DEBUG,
>> +           "ITS: CMD[0] = 0x%lx CMD[1] = 0x%lx CMD[2] = 0x%lx CMD[3] = 0x%lx\n",
>> +           cmd->bits[0], cmd->bits[1], cmd->bits[2], cmd->bits[3]);
>> +}
>> +#else
>> +void dump_cmd(its_cmd_block *cmd) { do {} while ( 0 ); }
>
> ditto for static and const
>
> Also, the do {} while 0 is not necessary given you are using function
> and not macro.
>
>> +#endif
>
>> +void its_send_inv(struct its_device *dev, struct its_collection *col,
>> +                  u32 event_id)
>
> On a follow-up patch (see #15) you change the prototype to be static.
> The static should be set now and not deferring until you use it just
> because of compilation issue. This is a call to reorder the patches or
> split them.
>
> Also, if you include the commit I mentioned at the beginning of the
> mail, you won't need to pass its_collection *col.
>
> It would be more cleaner for the caller and defer the choice of the
> collection within the function as Linux does.
>
> [..]
>
>> +void its_send_mapd(struct its_device *dev, int valid)
>
> static void ...
>
>> +{
>> +    its_cmd_block cmd;
>> +    unsigned long itt_addr;
>> +    u8 size;
>> +
>> +    size = max(ilog2(dev->nr_lpis), 1);
>
> Why do you need the max? dev->nr_lpis should always contains a valid value.
>
> [..]
>
>> +void its_send_mapvi(struct its_device *dev, struct its_collection *col,
>> +                    u32 phys_id, u32 event)
>
> All my remark on its_send_inv applies here too.
>
> [..]
>
>> +void its_send_discard(struct its_device *dev, struct its_collection *col,
>> +                      u32 event)
>
> All my remarks on its_send_inv applies here too.
>
> [..]
>
>> +unsigned long *its_lpi_alloc_chunks(int nirqs, int *base)
>
> static unsigned long ...
>
> [..]
>
>> +void its_lpi_free(struct its_device *dev)
>
> static void ...
>
> [..]
>
>> +static void its_cpu_init_lpis(void)
>> +{
>> +    void __iomem *rbase = gic_data_rdist().rbase;
>> +    void *pend_page;
>> +    u64 val, tmp;
>> +
>> +    /* If we didn't allocate the pending table yet, do it now */
>> +    pend_page = gic_data_rdist().pend_page;
>> +    if ( !pend_page )
>> +    {
>> +        paddr_t paddr;
>> +        u32 order;
>
> NIT: Newline here please.
>
>> +        /*
>> +         * The pending pages have to be at least 64kB aligned,
>> +         * hence the 'max(LPI_PENDBASE_SZ, SZ_64K)' below.
>> +         */
>
> [..]
>
>> +int __init its_init(struct rdist_prop *rdists)
>> +{
>> +    struct dt_device_node *np = NULL;
>> +
>> +    static const struct dt_device_match its_device_ids[] __initconst =
>> +    {
>> +        DT_MATCH_GIC_ITS,
>> +        { /* sentinel */ },
>> +    };
>> +
>> +    for (np = dt_find_matching_node(NULL, its_device_ids); np;
>> +             np = dt_find_matching_node(np, its_device_ids))
>
> The indentation looks wrong here.
>
>> +        its_probe(np);
>> +
>> +    if ( list_empty(&its_nodes) )
>> +    {
>> +        its_warn("ITS: No ITS available, not enabling LPIs\n");
>> +        return -ENXIO;
>> +    }
>> +
>> +    gic_rdists = rdists;
>> +    its_lpi_init(rdists->id_bits);
>> +    its_alloc_lpi_tables();
>
> I don't see much reason to change the order compare to Linux. Please
> do
>
> its_alloc_lpi_tables();
> its_its_lpi_init(rdists->id_bits);
>
> Regards,
>
> [1] http://lists.xen.org/archives/html/xen-devel/2015-07/msg03369.html
>
> --
> Julien Grall