Re: [Xen-devel] [PATCH v8a 5/6] xen/arm: split vgic driver into generic and vgic-v2 driver

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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