Re: [PATCH v7a 13/14] xen/arm: split vgic driver into generic and vgic-v2 driver

[Julien Grall <julien.grall@linaro.org>]

On 02/07/14 07:39, Vijay Kilari wrote:
> On Tue, Jul 1, 2014 at 6:38 PM, Julien Grall <julien.grall@linaro.org> wrote:
>
>>> -static int vgic_to_sgi(struct vcpu *v, register_t sgir)
>>> +/* TODO: unsigned long is used to fit vcpu_mask. Change to cpu_mask */
>>> +int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq,
>>> +                unsigned long vcpu_mask)
>>
>>
>> [..]
>>
>>> +    case SGI_TARGET_OTHERS:
>>
>> [..]
>>
>>> +        break;
>>> +    case SGI_TARGET_SELF:
>>> +        set_bit(current->vcpu_id, &vcpu_mask);
>>
>> I already said it on V4, V5 and now V6a and don't see any change or
>> comment from you side.
>>
>> For SGI_TARGET_{OTHERS,SELF}, you can't assume that vcpu_mask will be
>> equal to 0...
>>
>> It comes directly guest write to GICD_SIGR. Please make sure to handle
>> it correctly or the guest could send SGI to the wrong VCPUs.
>
> Sorry I missed this patch to fix the comments.
>
> First, if Guest uses this series of patches, mask is not written to GICD_SGIR
> for OTHERS & SELF case in v2 driver

That's not a reason to make buggy assumption. You will have to fix some 
day for another GIC driver. It's better to fix it today otherwise we 
will forget it later.

> static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
>                             const cpumask_t *cpu_mask)
> {
>      unsigned int mask = 0;
>
>      switch ( irqmode )
>      {
>      case SGI_TARGET_OTHERS:
>          writel_relaxed(GICD_SGI_TARGET_OTHERS | sgi, GICD + GICD_SGIR);
>          break;
>      case SGI_TARGET_SELF:
>          writel_relaxed(GICD_SGI_TARGET_SELF | sgi, GICD + GICD_SGIR);
>          break;
>
> Even if Guest writes some values to mask, then only way I check is to forcefully
> make vcpu_mask as 0 for OTHERS & SELF case before using it with warning

For the GICv2 POV it's perfectly valid to write garbage in CPUTargetList 
while we send an SGI to ourself. I'm fine with vcpu_mask = 0, but the 
warning is not useful as it's not a bug.

>>
>> [..]
>>
>>> +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);
>>
>> You've introduced vgic_send_sgi here, I'm not sure to understand why...
>> Can you give more input?
>
>      You have asked me make sending sgi as a callback.
> So I have introduced the following way
>
> On GICD_SGIR write by guest, vgic-v2.c calls vgic_send_sgi()
> which will call the registered callback (*send_sgi).
> Here send_sgi is registered with vgic_v2_to_sgi() which will inject
> sgi

IIRC, I've asked this change for the SGI support on VGICv3 to handle 
sysreg per vGIC.
IHMO, here it's as no-sense because the usage looks useless. I would 
like to see your GICv3 series before saying this is OK.

>>
>>> +};
>>> +
>>>   /* Number of ranks of interrupt registers for a domain */
>>>   #define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_lines+31)/32)
>>>
>>> @@ -133,6 +142,8 @@ static inline void vgic_byte_write(uint32_t *reg, uint32_t var, int offset)
>>>       *reg |= var;
>>>   }
>>>
>>> +extern enum gic_sgi_mode irqmode;
>>> +
>>
>> Hu? What is used for?
>
>     This is forward declaration for below function in vgic.h file. May
> be extern is not required here
>     extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
>                         enum gic_sgi_mode irqmode, int virq,
>                         unsigned long vcpu_mask);

Forward declaration of what? Here, you are defining an external variable 
"irqmode" which is never used.

Regards,

-- 
Julien Grall