From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 14/16] xen/arm: Add virtual GICv3 support Date: Thu, 24 Apr 2014 12:39:58 +0100 Message-ID: <5358F80E.5040909@linaro.org> References: <1397560675-29861-1-git-send-email-vijay.kilari@gmail.com> <1397560675-29861-15-git-send-email-vijay.kilari@gmail.com> <534F9E97.2000203@linaro.org> <1398335851.18537.316.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1398335851.18537.316.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: vijay.kilari@gmail.com, stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org 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