Re: [Xen-devel] [RFC PATCH v3 05/18] xen/arm: ITS: Port ITS driver to xen

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

On 26/06/2015 11:19, Vijay Kilari wrote:
> Hi Julien,
Hi Vijay,

> On Mon, Jun 22, 2015 at 10:46 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> > Hi,
> > > +    cmd->mapd.cmd = GITS_CMD_MAPD;
> > > +    cmd->mapd.devid = desc->its_mapd_cmd.dev->device_id;
> > > +    cmd->mapd.size = size - 1;
> > > +    cmd->mapd.itt = itt_addr >> 8;
> > I think the code is more difficult to read without the helpers. You
> > opencode every trick in all the builder rather than in a single place.
>      ITS commands does not consider all the bits of the values esp. when
> encoding addresses. So we need to shift and write it.
A comment on the code would have been welcomed (either here or in the 
definition of the structure). By naming the field itt it gives the 
impression that we have to pass all the ITT address.
> Helpers might be useful, but usage is hardly once or twice in the code.
IHMO, once is okay but twice is too much. It means that you duplicated 
code that may be non straigh-forward to understand (such as shift). 
Anyway, I will let Ian & Stefano decide.
> > > +static void its_cpu_init_collection(void)
> > > +{
> > > +    struct its_node *its;
> > > +    int cpu;
> > > +
> > > +    spin_lock(&its_lock);
> > > +    cpu = smp_processor_id();
> > > +
> > > +    list_for_each_entry(its, &its_nodes, entry)
> > > +    {
> > > +        u64 target;
> > > +        /*
> > > +         * We now have to bind each collection to its target
> > > +         * redistributor.
> > > +         */
> > > +        if ( readq_relaxed(its->base + GITS_TYPER) & GITS_TYPER_PTA )
> > > +        {
> > > +            /*
> > > +             * This ITS wants the physical address of the
> > > +             * redistributor.
> > > +             */
> > > +            target = gic_data_rdist().phys_base;
> > > +        }
> > > +        else
> > > +        {
> > > +            /*
> > > +             * This ITS wants a linear CPU number.
> > > +             */
> > > +            target = readq_relaxed(gic_data_rdist_rd_base() + GICR_TYPER);
> > > +            target = GICR_TYPER_CPU_NUMBER(target);
> > Why did you drop the << 16?
> >
> > Although, as said earlier, given the usage of target_address you could
> > do shift >> directly in this function rather than on multiple__ place.
> Yes, we can shift at the setup. But we lose actual value of target_address.
Do we really need to get the target_address intact? I believe not.

> > > +    gic_rdists = rdists;
> > > +    its_alloc_lpi_tables();
> > > +
> > > +    BUILD_BUG_ON(sizeof(its_cmd_block) != 32);
> > Why this build bug on here? Shouldn't it be part of the builder code?
>   Where is builder code in xen?
By builder I meant the function who built the command i.e 
its_send_single_command.
Regards,

--
Julien Grall

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