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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Thu, Jul 3, 2014 at 8:48 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 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.

  The reason why vgic_emulate is kept in vgic-v3.c is because it is
GICv3 specific
and also the traps are landing in respective drivers. Ex: for GICv2
write to GICD_SGIR
is trapped in vgic-v2.c and similarly I expect for GICv3 is should be
kept in GICv3.

If we introduce emulate_sysreg as a callback, then there is no need of
send_sgi callback
and hence the handling path of SGI trap is different for GICv2 and GICv3.

>>
>> 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