From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC PATCH v2 13/22] xen/arm: its: Add virtual ITS command support Date: Mon, 30 Mar 2015 16:47:17 +0100 Message-ID: <55197005.7090408@linaro.org> References: <1426775889-29442-1-git-send-email-vijay.kilari@gmail.com> <1426775889-29442-14-git-send-email-vijay.kilari@gmail.com> <55114EFA.9050304@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" 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 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 30/03/15 16:02, Vijay Kilari wrote: > Hi Julien, Hello Vijay, > On Tue, Mar 24, 2015 at 5:18 PM, Julien Grall wrote: >> On 19/03/2015 14:38, vijay.kilari@gmail.com wrote: >>> >>> +int vgic_its_process_cmd(struct vcpu *v, struct vgic_its *vits) >>> +{ >>> + struct its_cmd_block virt_cmd; >>> + >>> + /* XXX: Currently we are processing one cmd at a time */ >>> + ASSERT(spin_is_locked(&vits->lock)); >>> + >>> + do { >>> + if ( vgic_its_read_virt_cmd(v, vits, &virt_cmd) ) >>> + goto err; >>> + if ( vgic_its_parse_its_command(v, vits, &virt_cmd) ) >>> + goto err; >>> + } while ( vits->cmd_write != vits->cmd_write_save ); >>> + >>> + vits->cmd_write_save = vits->cmd_write; >>> + DPRINTK("vITS: write_save 0x%lx write 0x%lx \n", >>> + vits->cmd_write_save, >>> + vits->cmd_write); >>> + /* XXX: Currently we are processing one cmd at a time */ >>> + vgic_its_update_read_ptr(v, vits); >> >> >> From the spec the GITS_CREADR should be updated at every command processing. >> That would make cmd_write_save pointless. > > See notes under section 4.9.9 Adding New Commands to the Queue > Multiple commands can be written to a queue at once. You didn't understand my point. The steps to process a command are: 1) read command 2) handle command 3) increment CREADR 4) loop to 1 if another command to process Currently, you only do the step 3 when all commands are processed. >> >> Also, you are taking the VITS lock for the whole process. This process can >> be very long. How will it affect the other vCPUs of the domain? >> > > Yes, lock is taken on first command trap and holds until all commands > are processed. > In any case ITS commands are processed in synchronously. So any VCPU that > send ITS commands is blocked. This is wrong. The command processing is an asynchronous process and can be long. A VCPU may want to do other things (like handling interrupt) while the ITS is processing. With your implementation you rule out this possibility. > Also ITS commands are sent while setting up device/irq and while > releasing device/irq. > So there should not be any overhead when device is under use. Ok. >> Finally, in environment with multiple guests using ITS, the ITS command send >> to the physical ITS may be interleaved (i.e DOM1 cmd, DOM2 cmd, DOM1 cmd >> ...). Is there any possible side-effect? > > Each command is independent. Generally SYNC/INV is followed after some > commands. But it should not be a problem if they are interleaved. What happen if the guest decide to not send the SYNC/INV? What is the state of the ITS in this case? Would it be possible to receive a wrong LPIs? Regards, -- Julien Grall