Re: [Xen-devel] [PATCH v5 16/21] xen/arm: split vgic driver into generic and vgic-v2 driver

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Sun, Jun 15, 2014 at 10:34 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Vijay,
>
> You didn't address the comments I made on V4 for this patch.
> See a copy of them inline...
>
>
> On 12/06/14 14:36, vijay.kilari@xxxxxxxxx wrote:
>>
>> +static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
>
>
> I didn't review closer this function. I guess it's a simple copy from
> vgic_distr_mmio_read?

Yes, it is just code movement

>
> [..]
>
>
>> +static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>
>
> Same question here?

Yes, it is just code movement

>> -static int vgic_to_sgi(struct vcpu *v, register_t sgir)
>> +int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode
>> irqmode, int virq,
>> +                unsigned long vcpu_mask)
>
>
>
> You can't assume that all the VCPU bits will fit in an unsigned long. We
> will have to use cpumask_t at some point.
>
> I'm fine if you don't handle it for now, but you need to *write down*
> somewhere the limitation of this function.

 I have just put TODO here. IMO vcpu_mask cannot be more than
8 bits (8 cpus) in GICv2 and 16 bits (16 cpus)  in GICv3.

>
>
> [..]
>
>> +    case SGI_TARGET_OTHERS:
>
>
> [..]
>
>> +    case SGI_TARGET_SELF:
>
>
> For this 2 case, you can't assume that vcpu_mask will be equal to 0...
> It comes from the GICD_SGIR...

 Yes, it comes from GICD_SGIR emulation write.  Only way to check is to
put an assert

Regards
Vijay

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