Re: [PATCH] xen/arm: gic-v3: Implement correctly the callback send_SGI

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-05-08 at 15:34 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 08/05/15 15:01, Ian Campbell wrote:
> > On Mon, 2015-04-27 at 19:31 +0100, Julien Grall wrote:
> >> Currently, the GICv3 drivers is only able to send an SGI when the cpumask
> > 
> > "driver"
> > 
> >> is provided. Although with the modes SGI_TARGET_OTHERS and SGI_TARGET_SELF,
> >> no cpumask is provided. Any usage of those modes will crash the hypersivor.
> >>
> >> Move the rename gicv3_send_sgi to gicv3_send_sgi_list and implement the
> > 
> > s/Move the r/R/
> 
> I missed while reviewing the commit message. I will fix it in the next
> version.
> 
> >> different modes:
> >>     - SGI_TARGET_OTHERS: Set the Interrupt Routing Mode (bit 40) to 1
> >>     (see Table 4 on Section 4.2.6 PRD03-GENC-010745 24.0)
> >>     - SGI_TARGET_SELF: Contrawise GICv2, the SGI registers doesn't provide
> > 
> > "Contrariwise"? But I think you really mean "Unlike". I'd also say "the
> > GICv3 SGI registers" and s/doesn't/don't/.
> 
> Yes, "unlike" is better here.
> 
> > 
> >>     a specific field. So use gicv3_send_sgi_list and pass the cpumask of
> >>     the current CPU
> >>     - SGI_TARGET_LIST: Directly call gicv3_send_sgi_list with the given
> >>     cpumask
> >>
> >> Reported-by: Chen Baozi <baozich@gmail.com>
> >> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > 
> > Apart from the commit message nits one code comment:
> > 
> > [...]
> >> +    case SGI_TARGET_OTHERS:
> >> +        WRITE_SYSREG(ICH_SGI_TARGET_OTHERS << ICH_SGI_IRQMODE_SHIFT |
> >> +                     (register_t)sgi << ICH_SGI_IRQ_SHIFT,
> > 
> > I don't think you want register_t here, unless ICC_SGI1R_EL1 might be a
> > different width on different architectures.
> 
> Yes, ICC_SGIR1_EL1 is always 64-bit.
> 
> > I think you should just cast to uint64_t and WRITE_SYSREG64.
> 
> Right, I blindly re-use the WRITE_SYSREG as it was done on the previous
> implementation.
> 
> Although, the gic-v3 code seems to always use {READ,WRITE}_SYSREG. This
> seems wrong too. I will send a patch for replace the calls by the the
> 64-bit version.

Thanks (although do make sure each register really is 64-bit...)