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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

Hi Julien,


On Tue, Jul 1, 2014 at 6:38 PM, Julien Grall <julien.grall@xxxxxxxxxx> 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

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


>
> [..]
>
>> +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

>
>> +};
>> +
>>  /* 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);

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel