Re: [Xen-devel] [PATCH v3 14/16] xen/arm: Add virtual GICv3 support

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 04/24/2014 11:37 AM, Ian Campbell wrote:
> On Thu, 2014-04-17 at 10:27 +0100, Julien Grall wrote:
>>> +        return 0;
>>> +    case GICR_SYNCR:
>>> +        if ( dabt.size != 2 ) goto bad_width;
>>> +        /* RO */
>>> +        /* Return as not busy */
>>> +        *r = GICR_SYNCR_NOT_BUSY;
>>
>> Please explain why this value.
> 
> Doesn't the comment already do so?

Hmmm ... right, I don't remember why I though a better comment was needed.


>>> +        return 0;
>>> +    case GICR_PIDR7... GICR_PIDR0:
> 
> Strange ordering, and whitespace incorrect.
> 
>>> +        *r = GICR_PIDR0_GICV3;
>>
>> Hrrmmm... no. PIDR1 should return 0xB (on ARM implementation)...
> 
> It's certainly odd to return PIDR0 for all 7 registers.
> 
> It's not clear that returning the ARM values is the right thing to do,
> but this is similar to the JEP106 thing in GICD_IIDR.
> 
>>> +    /* Implementation defined -- read as zero */
>>> +    case REG(0x020) ... REG(0x03c):
>>
>> Please move the comment after the case.
>>
>> Also can you give a name to this register? For clarity.
> 
> They are implementation defined, so no I would say. The existing code
> does the same, it is fine IMHO.

I've noticed this part after I wrote my email.

> 
>>> @@ -51,6 +54,9 @@ static inline int REG_RANK_NR(int b, uint32_t n)
>>>  {
>>>      switch ( b )
>>>      {
>>> +    case 64: return n >> 6;
>>> +    case 32: return n >> 5;
>>> +    case 16: return n >> 4;
>>
>> It doesn't sounds right to update this common function. What happen if
>> VGIC v2 is trying to use this value?
> 
> These are correct responses to being passed those values of b and n, I
> think this is absolutely fine to do this in a common place.

I agree with you. With this change, if the VGICv2 emulation decides to
use this value by mistake, the code doesn't hit the BUG_ON anymore.

On another side the call site of this function check if the rank is
valid... So I guess it's fine.

Regards,

-- 
Julien Grall

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