Re: [PATCH v3 07/16] xen/arm: segregate and split GIC low level functionality

[Julien Grall <julien.grall@linaro.org>]

Hello Vijay,

A couple of things I've noticed while reviewing this series.

On 04/15/2014 12:17 PM, vijay.kilari@gmail.com wrote:
> +static void gicv2_write_lr(int lr, struct gic_lr *lr_reg)
> +{
> +    uint32_t lrv = 0;

Newline here.

> +    lrv = ( ((lr_reg->pirq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT)  |
> +            ((lr_reg->virq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT)   |
> +            ((uint32_t)(lr_reg->priority & GICH_LR_PRIORITY_MASK) << GICH_LR_PRIORITY_SHIFT) |
> +            ((uint32_t)(lr_reg->state & GICH_LR_STATE_MASK) << GICH_LR_STATE_SHIFT) |
> +            ((uint32_t)(lr_reg->hw_status & GICH_LR_HW_MASK)  << GICH_LR_HW_SHIFT)  |
> +            ((uint32_t)(lr_reg->grp & GICH_LR_GRP_MASK) << GICH_LR_GRP_SHIFT) );
> +
> +    GICH[GICH_LR + lr] = lrv;
> +}

[..]

> +struct gic_hw_operations {

Some of these functions require locks. You have to specified here which
lock is required.

That will avoid headache for people who are planning to implement new
GIC driver.

> +    /* GIC version */
> +    enum gic_version hw_version;
> +    /* Number of GIC lines supported */
> +    unsigned int nr_lines;
> +    /* Number of LR registers */
> +    unsigned int nr_lrs;
> +    /* Maintenance irq is derived from dt node. Fetch from gic driver */
> +    struct dt_irq * (*get_maintenance_irq)(void);
> +    /* Save GIC registers */
> +    void (*save_state)(struct vcpu *);
> +    /* Restore GIC registers */
> +    void (*restore_state)(struct vcpu *);
> +    /* Dump GIC LR register information */
> +    void (*dump_state)(struct vcpu *);
> +    /* Map MMIO region of GIC and get GIC address information */
> +    int (*gicv_setup)(struct domain *);
> +
> +    /* hw_irq_controller for enable/disable/eoi irq */
> +    hw_irq_controller *gic_irq_ops;
> +
> +    /* Deactivate/reduce priority of irq */
> +    void (*deactivate_irq)(int);
> +    /* Ack IRQ and return irq id */
> +    unsigned int (*ack_irq)(void);

This name of this function is not clear for me. I would rename into

read_irq_and_ack (maybe the and_ack is not necessary).

> +    /* Set IRQ property */
> +    void (*set_irq_property)(unsigned int irq, bool_t level,
> +                            const cpumask_t *cpu_mask,
> +                            unsigned int priority);
> +    /* Send SGI */
> +    void (*send_sgi)(const cpumask_t *online_mask,
> +                         enum gic_sgi sgi, uint8_t irqmode);
> +    /* Disable CPU physical and virtual interfaces */
> +    void (*disable_interface)(void);
> +    /* Update LR with state and priority */
> +    void (*update_lr)(int lr, struct pending_irq *, unsigned int state);
> +    /* Update HCR status register */
> +    void (*update_hcr_status)(uint32_t flag, bool_t set);
> +    /* Clear LR register */
> +    void (*clear_lr)(int lr);
> +    /* Read LR register and populate gic_lr structure */
> +    void (*read_lr)(int lr, struct gic_lr *);
> +    /* Write LR register from gic_lr structure */
> +    void (*write_lr)(int lr, struct gic_lr *);
> +    /* Read VMCR priority */
> +    unsigned int (*read_vmcr_priority)(void);
> +    /* Secondary CPU init */
> +    void (*secondary_init)(void);

I would return an int here. I saw a panic on GICv3, and I don't think
it's acceptable for secondary CPUs. I can be used during CPU hotplug.

Regards,

-- 
Julien Grall