| Message ID | 1396969969-18973-5-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote: > A later patch is going to use uint8_t to keep track of LRs. > Both GICv3 and GICv2 don't need any more than an uint8_t to keep track > of the number of LRs. Did you confirm that this doesn't lead to inefficient loading and masking stuff on access? > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Acked-by: Julien Grall <julien.grall@linaro.org> If yes then: Acked-by: Ian Campbell <ian.campbell@citrix.com> Although TBH I'm not sure why unsigned int was so harmful here. Ian.
On 04/23/2014 01:47 PM, Ian Campbell wrote: > On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote: >> A later patch is going to use uint8_t to keep track of LRs. >> Both GICv3 and GICv2 don't need any more than an uint8_t to keep track >> of the number of LRs. > > Did you confirm that this doesn't lead to inefficient loading and > masking stuff on access? > >> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> Acked-by: Julien Grall <julien.grall@linaro.org> > > If yes then: > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > Although TBH I'm not sure why unsigned int was so harmful here. In struct irq_pending (see next patch: #6) the lr is stored using an uint8. IHMO, the both variable should have the same type to avoid mistake.
On Wed, 2014-04-23 at 13:53 +0100, Julien Grall wrote: > On 04/23/2014 01:47 PM, Ian Campbell wrote: > > On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote: > >> A later patch is going to use uint8_t to keep track of LRs. > >> Both GICv3 and GICv2 don't need any more than an uint8_t to keep track > >> of the number of LRs. > > > > Did you confirm that this doesn't lead to inefficient loading and > > masking stuff on access? > > > >> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > >> Acked-by: Julien Grall <julien.grall@linaro.org> > > > > If yes then: > > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > > > Although TBH I'm not sure why unsigned int was so harmful here. > > In struct irq_pending (see next patch: #6) the lr is stored using an uint8. > > IHMO, the both variable should have the same type to avoid mistake. That could easily be avoided by a range check before setting nr_lrs, which we must have to do anyway. Ian.
On 04/23/2014 02:07 PM, Ian Campbell wrote: > On Wed, 2014-04-23 at 13:53 +0100, Julien Grall wrote: >> On 04/23/2014 01:47 PM, Ian Campbell wrote: >>> On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote: >>>> A later patch is going to use uint8_t to keep track of LRs. >>>> Both GICv3 and GICv2 don't need any more than an uint8_t to keep track >>>> of the number of LRs. >>> >>> Did you confirm that this doesn't lead to inefficient loading and >>> masking stuff on access? >>> >>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>>> Acked-by: Julien Grall <julien.grall@linaro.org> >>> >>> If yes then: >>> Acked-by: Ian Campbell <ian.campbell@citrix.com> >>> >>> Although TBH I'm not sure why unsigned int was so harmful here. >> >> In struct irq_pending (see next patch: #6) the lr is stored using an uint8. >> >> IHMO, the both variable should have the same type to avoid mistake. > > That could easily be avoided by a range check before setting nr_lrs, > which we must have to do anyway. We might want to check against the hardcoded value used to create gic_lr (i.e 64). For your sentence "Did you confirm that this doesn't lead to inefficient loading and masking stuff on access?", there is an instruction to only load/store 8 bits (strb, ldrb). I don't see how the compiler will do inefficient loading.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index b8b1452..f1ce9b7 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -56,7 +56,7 @@ static irq_desc_t irq_desc[NR_IRQS]; static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc); static DEFINE_PER_CPU(uint64_t, lr_mask); -static unsigned nr_lrs; +static uint8_t nr_lrs; #define lr_all_full() (this_cpu(lr_mask) == ((1 << nr_lrs) - 1)) /* The GIC mapping of CPU interfaces does not necessarily match the