From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC PATCH v3 05/18] xen/arm: ITS: Port ITS driver to xen Date: Fri, 26 Jun 2015 11:52:22 +0200 Message-ID: <558D20D6.2090805@gmail.com> References: <1434974517-12136-1-git-send-email-vijay.kilari@gmail.com> <1434974517-12136-6-git-send-email-vijay.kilari@gmail.com> <558842DE.10100@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Vijay Kilari , Julien Grall Cc: Ian Campbell , Stefano Stabellini , Prasun Kapoor , Vijaya Kumar K , Tim Deegan , "xen-devel@lists.xen.org" , Stefano Stabellini , manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org On 26/06/2015 11:19, Vijay Kilari wrote: > Hi Julien, Hi Vijay, > On Mon, Jun 22, 2015 at 10:46 PM, Julien Grall 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