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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

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

> > @@ -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.

Ian.


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