From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v8a 5/6] xen/arm: split vgic driver into generic and vgic-v2 driver Date: Thu, 03 Jul 2014 15:21:22 +0100 Message-ID: <53B566E2.4030403@linaro.org> References: <1404376489-25165-1-git-send-email-vijay.kilari@gmail.com> <1404376489-25165-6-git-send-email-vijay.kilari@gmail.com> <53B5534E.5080509@linaro.org> <1404392555.19893.12.camel@kazak.uk.xensource.com> <53B559E6.3010403@linaro.org> <1404396160.19893.30.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1404396160.19893.30.camel@kazak.uk.xensource.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 Cc: vijay.kilari@gmail.com, stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, tim@xen.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org On 07/03/2014 03:02 PM, Ian Campbell wrote: > On Thu, 2014-07-03 at 14:25 +0100, Julien Grall wrote: >> On 07/03/2014 02:02 PM, Ian Campbell wrote: >>> On Thu, 2014-07-03 at 13:57 +0100, Julien Grall wrote: >>> >>>>> +struct vgic_ops { >>>>> + /* Initialize vGIC */ >>>>> + int (*vcpu_init)(struct vcpu *v); >>>>> + /* Domain specific initialization of vGIC */ >>>>> + int (*domain_init)(struct domain *d); >>>>> + /* SGI handler of vGIC */ >>>>> + int (*send_sgi)(struct vcpu *v, register_t sgir); >>>> >>>> By reviewing the VGIC-v3 support, I still don't think this is the right >>>> callback to add. You bypass the VGIC common emulation with your >>>> vgic_emulate... >>>> >>>> I would introduce a callback to emulate_sysreg rather than this send_sgi. >>> >>> Why? The vgic will either be v2 or v3, so either MMIO or sysreg, once >>> the thing has been decoded then you want to send an SGI I think, hence >>> the callback. Passing a register_t does seem odd though, I'd have >>> thought it would take an SGI number and any other flags which would then >>> be interpreted for either v2 or v3 as appropriate. >> >> The decoding depends on the vgic emulation. For now this function is >> badly implement in vgic-v3.c. >> >> What I was trying to say is send_sgi can be handled internally. If you >> are looking to the calls of this function, it's only happen within the >> file vgic-v2.c (or vgic-v3.c) >> >> But, the sysreg emulation is called outside the vgic code. So we should >> add a callaback for this. > > So the common code would have > case HSR_SYSREG_ICC_SGI0R: > gic->handle_sysreg(esr, val) > instead of > case HSR_SYSREG_ICC_SGI0R: > gic->handle_sysreg(val); > ? The is not the actual case, Actually, the common code is: case HSR_SYSREG_ICC_SGI0R: vgic_emulate(regs, hsr) where vgic_emulate is implemented in vgic-v3.c rather than in vgic.c. The function will decode the register and then call vgic_send_sgi. But, as the function send_sgi is only used internaly there is no reason to create a callback. What I ask is to have a new callback emulate_sysreg. The common code (i.e traps.c) will have: case HSR_SYSREG_ICC_SGI0R: vgic_emulate(regs, hsr). The function vgic_emulate will be implemented in vgic.c: vgic_emulate(...) { vgic->emulate_sysreg(regs, hsr); } The vgic-v3.c will implement the callback correctly. > That might be nicer, but TBH given that there is only one trappable gic > sysreg right now I don't think it is worth getting too worried about. We > can always rework this interface when gic v4 or v5 needs something more. I don't see why we should break the vgic common implementation as it does on the next series: http://www.gossamer-threads.com/lists/xen/devel/337708. Regards, -- Julien Grall