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

[Julien Grall <julien.grall@linaro.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