[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v8a 5/6] xen/arm: split vgic driver into generic and vgic-v2 driver
On Thu, 2014-07-03 at 15:21 +0100, Julien Grall wrote: > 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. I don't mind this but I would be equally happy with vgic_emulate being in gic-v3.c at this stage without the unnecessary callback via ->send_sgi. > > 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. Like I said, for this particular instance I don't think it is a big deal right now. IOW I'd rather see this series go in whether this aspect is perfect or not. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |