From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v6 4/6] xen/arm: vgic: Optimize the way to store the target vCPU in the rank Date: Mon, 30 Nov 2015 14:02:12 +0000 Message-ID: <565C56E4.2020803@citrix.com> References: <1447864963-21577-1-git-send-email-julien.grall@citrix.com> <1447864963-21577-5-git-send-email-julien.grall@citrix.com> <1448451462.17688.73.camel@citrix.com> <565C4FF0.5050609@citrix.com> <1448891740.15768.41.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1a3P3W-0004TT-RB for xen-devel@lists.xenproject.org; Mon, 30 Nov 2015 14:03:50 +0000 In-Reply-To: <1448891740.15768.41.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , xen-devel@lists.xenproject.org Cc: stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 30/11/15 13:55, Ian Campbell wrote: > On Mon, 2015-11-30 at 13:32 +0000, Julien Grall wrote: >> Hi Ian, >> >> On 25/11/15 11:37, Ian Campbell wrote: >>> On Wed, 2015-11-18 at 16:42 +0000, Julien Grall wrote: >>>> Xen is currently directly storing the value of GICD_ITARGETSR >>>> register >>>> (for GICv2) and GICD_IROUTER (for GICv3) in the rank. This makes the >>>> emulation of the registers access very simple but makes the code to >>>> get >>>> the target vCPU for a given vIRQ more complex. >>>> >>>> While the target vCPU of an vIRQ is retrieved every time an vIRQ is >>>> injected to the guest, the access to the register occurs less often. >>>> >>>> So the data structure should be optimized for the most common case >>>> rather than the inverse. >>>> >>>> This patch introduces the usage of an array to store the target vCPU >>>> for >>>> every interrupt in the rank. This will make the code to get the >>>> target >>>> very quick. The emulation code will now have to generate the >>>> GICD_ITARGETSR >>>> and GICD_IROUTER register for read access and split it to store in a >>>> convenient way. >>>> >>>> With the new way to store the target vCPU, the structure >>>> vgic_irq_rank >>>> is shrunk down from 320 bytes to 92 bytes. This is saving about 228 >>>> bytes of memory allocated separately per vCPU. >>>> >>>> Note that with these changes, any read to those register will list >>>> only >>>> the target vCPU used by Xen. As the spec is not clear whether this is >>>> a >>>> valid choice or not, OSes which have a different interpretation of >>>> the >>>> spec (i.e OSes which perform read-modify-write operations on these >>>> registers) may not boot anymore on Xen. Although, I think this is >>>> fair >>>> trade between memory usage in Xen (1KB less on a domain using 4 vCPUs >>>> with no SPIs) and a strict interpretation of the spec (though all the >>>> cases are not clearly defined). >>>> >>>> Furthermore, the implementation of the callback get_target_vcpu is >>>> now >>>> exactly the same. Consolidate the implementation in the common vGIC >>>> code >>>> and drop the callback. >>>> >>>> Finally take the opportunity to fix coding style and replace "irq" by >>>> "virq" to make clear that we are dealing with virtual IRQ in section >>>> we >>>> are modifying. >>>> >>>> Signed-off-by: Julien Grall >>> >>> Acked-by: Ian Campbell >>> >>> I have one clarifying question, which may or may not be worth a >>> followup: >>> >>>> + * Fetch an ITARGETSR register based on the offset from ITARGETSR0. >>> >>> Is the offset here in terms of bytes or in terms of entire ITARGETSR >>> registers (i.e. 4 bytes)? >> >> The offset is in term of bytes. >> >>> Might be worth clarifying the comment? >> >> I'm not sure, I think it's implicit with the following sentence in the >> comment: >> >> "Note the offset will be aligned to the appropriate boundary." > > It's very implicit, since without knowing the answer it's not clear what an > appropriate boundary is. > > How about: "Note the byte offset will be aligned to an ITARGETSR" > boundary" ? Ok. I will do the same for the comment on top of vgic_*_irouter. Regards, -- Julien Grall