From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2 05/15] xen/arm: segregate GIC low level functionality Date: Fri, 04 Apr 2014 14:55:16 +0100 Message-ID: <533EB9C4.3020709@linaro.org> References: <1396612593-443-1-git-send-email-vijay.kilari@gmail.com> <1396612593-443-6-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1396612593-443-6-git-send-email-vijay.kilari@gmail.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: vijay.kilari@gmail.com Cc: Ian.Campbell@citrix.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 Hi Vijaya, Thank you for the patch. On 04/04/2014 12:56 PM, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K > > GIC low level functionality is segregated into > separate functions and are called using registered > callback wherever required. > > This helps to separate generic and hardware functionality > later Honestly, your patch on V1 was far better to read. As you are nearly modify every functions, you should directly move it to a new file (e.g merge with patch #9). > Signed-off-by: Vijaya Kumar K > --- > xen/arch/arm/gic.c | 362 ++++++++++++++++++++++++++++--------- > xen/include/asm-arm/gic.h | 50 +++++ > xen/include/asm-arm/gic_v2_defs.h | 16 +- > 3 files changed, 328 insertions(+), 100 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 64699e4..9f03135 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -57,8 +57,21 @@ 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 struct gic_hw_operations *gic_hw_ops; > +static struct gic_hw_operations gic_ops; > + > +void register_gic_ops(struct gic_hw_operations *ops) the callback should not change during runtime. Should the prototype should be: void register_gic_ops(const struct gic_hw_operations *ops); > +{ > + gic_hw_ops = ops; > +} > + > +void update_cpu_lr_mask(void) > +{ > + this_cpu(lr_mask) = 0ULL; > +} > + > static uint8_t nr_lrs; > -#define lr_all_full() (this_cpu(lr_mask) == ((1 << nr_lrs) - 1)) > +#define lr_all_full() (this_cpu(lr_mask) == ((1 << gic_hw_ops->nr_lrs()) - 1)) > > /* The GIC mapping of CPU interfaces does not necessarily match the > * logical CPU numbering. Let's use mapping as returned by the GIC > @@ -89,48 +102,124 @@ static unsigned int gic_cpu_mask(const cpumask_t *cpumask) > > unsigned int gic_number_lines(void) > { > + return gic_hw_ops->nr_lines(); > +} > + > +static unsigned int gic_nr_lines(void) > +{ > return gic.lines; > } If you had directly move the code to another files you would avoid this kind of non sense name... > > -irq_desc_t *__irq_to_desc(int irq) > +static unsigned int gic_nr_lrs(void) > { > - if (irq < NR_LOCAL_IRQS) return &this_cpu(local_irq_desc)[irq]; > - return &irq_desc[irq-NR_LOCAL_IRQS]; Why did you had to modify __irq_to_desc? > + return nr_lrs; > } > > -void gic_save_state(struct vcpu *v) > +static int gic_state_init(struct vcpu *v) > +{ > + v->arch.gic_state = xzalloc(struct gic_state_data); > + if(!v->arch.gic_state) if ( !v->arch.gic_state ) > + return -ENOMEM; > + return 0; > +} > + > +static void save_state(struct vcpu *v) > { > int i; > - ASSERT(!local_irq_is_enabled()); > > /* No need for spinlocks here because interrupts are disabled around > * this call and it only accesses struct vcpu fields that cannot be > * accessed simultaneously by another pCPU. > */ > - for ( i = 0; i < nr_lrs; i++ ) > + for ( i = 0; i < nr_lrs; i++) Spurious change [..] > @@ -129,6 +149,36 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize, > unsigned int *out_hwirq, unsigned int *out_type); > void gic_clear_lrs(struct vcpu *v); > > +struct gic_hw_operations { I made some remarks on v1, that you completely forgot to address on this structure. Can you document every callbacks (see asm-arm/platform.h for an example)? Why do you need them... > + struct dt_irq * (*get_maintenance_irq)(void); With my interrupts patch series dt_irq will be removed soon. > + unsigned int (*nr_lines)(void); > + unsigned int (*nr_lrs)(void); I don't think we need a callbacks for both of them. 2 unsigned int are fine. [..] > + void (*enable_irq)(int); > + void (*disable_irq)(int); > + void (*eoi_irq)(int); > + void (*deactivate_irq)(int); > + unsigned int (*ack_irq)(void); I would prefer to introduce a new hw_irq_controller here rather than adding another abstraction. > + void (*set_irq_property)(unsigned int irq, bool_t level, > + const cpumask_t *cpu_mask, > + unsigned int priority); I would rename the function into set_irq_properties. [..] > diff --git a/xen/include/asm-arm/gic_v2_defs.h b/xen/include/asm-arm/gic_v2_defs.h > index f9ff885..e1bb09c 100644 > --- a/xen/include/asm-arm/gic_v2_defs.h > +++ b/xen/include/asm-arm/gic_v2_defs.h > @@ -93,15 +93,6 @@ > #define GICC_IA_CPU_MASK 0x1c00 > #define GICC_IA_CPU_SHIFT 10 > > -#define GICH_HCR_EN (1 << 0) > -#define GICH_HCR_UIE (1 << 1) > -#define GICH_HCR_LRENPIE (1 << 2) > -#define GICH_HCR_NPIE (1 << 3) > -#define GICH_HCR_VGRP0EIE (1 << 4) > -#define GICH_HCR_VGRP0DIE (1 << 5) > -#define GICH_HCR_VGRP1EIE (1 << 6) > -#define GICH_HCR_VGRP1DIE (1 << 7) > - > #define GICH_MISR_EOI (1 << 0) > #define GICH_MISR_U (1 << 1) > #define GICH_MISR_LRENP (1 << 2) > @@ -118,9 +109,12 @@ > #define GICH_LR_STATE_MASK 0x3 > #define GICH_LR_STATE_SHIFT 28 > #define GICH_LR_PRIORITY_SHIFT 23 > +#define GICH_LR_PRIORITY_MASK 0x1f > +#define GICH_LR_HW_SHIFT 31 > +#define GICH_LR_HW_MASK 0x1 > +#define GICH_LR_GRP_SHIFT 30 > +#define GICH_LR_GRP_MASK 0x1 > #define GICH_LR_MAINTENANCE_IRQ (1<<19) > -#define GICH_LR_PENDING (1<<28) > -#define GICH_LR_ACTIVE (1<<29) > #define GICH_LR_GRP1 (1<<30) > #define GICH_LR_HW (1<<31) > #define GICH_LR_CPUID_SHIFT 9 Why did you move all theses define on gic_v2_defs.h, and move back to gic.h again? Regards, -- Julien Grall