diff mbox

[v9,14/14] virt: arm: support hip04 gic

Message ID 87oayr1dko.fsf@approximate.cambridge.arm.com
State New
Headers show

Commit Message

Marc Zyngier May 21, 2014, 1:11 p.m. UTC 
Hi Haohian,

Christoffer has already heavily commented on some aspects of this, so
I'm going to stay clear of them. But there is a number of other issues
that I'd like to outline.

On Tue, May 20 2014 at  2:10:27 pm BST, Haojian Zhuang <haojian.zhuang@linaro.org> wrote:
> In ARM standard GIC, GICH_APR offset is 0xf0 & GICH_LR0 offset is 0x100.
> In HiP04 GIC, GICH_APR offset is 0x70 & GICH_LR0 offset is 0x80.
>
> Now reuse the nr_lr field in struct vgic_cpu. Bit[31:16] is used to store
> GICH_APR offset in HiP04, and bit[15:0] is used to store real nr_lr
> variable. In ARM standard GIC, don't set bit[31:16]. So we could avoid
> to change the VGIC implementation in arm64.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  arch/arm/kernel/asm-offsets.c   |  2 +-
>  arch/arm/kvm/interrupts_head.S  | 29 +++++++++++++++++++------
>  arch/arm64/kernel/asm-offsets.c |  2 +-
>  arch/arm64/kvm/hyp.S            | 28 ++++++++++++++++++------
>  include/kvm/arm_vgic.h          |  7 ++++--
>  include/linux/irqchip/arm-gic.h |  6 ++++++
>  virt/kvm/arm/vgic.c             | 48 +++++++++++++++++++++++++++++------------
>  7 files changed, 92 insertions(+), 30 deletions(-)
>
> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
> index 85598b5..166cc98 100644
> --- a/arch/arm/kernel/asm-offsets.c
> +++ b/arch/arm/kernel/asm-offsets.c
> @@ -189,7 +189,7 @@ int main(void)
>    DEFINE(VGIC_CPU_ELRSR,       offsetof(struct vgic_cpu, vgic_elrsr));
>    DEFINE(VGIC_CPU_APR,         offsetof(struct vgic_cpu, vgic_apr));
>    DEFINE(VGIC_CPU_LR,          offsetof(struct vgic_cpu, vgic_lr));
> -  DEFINE(VGIC_CPU_NR_LR,       offsetof(struct vgic_cpu, nr_lr));
> +  DEFINE(VGIC_CPU_HW_CFG,      offsetof(struct vgic_cpu, hw_cfg));
>  #ifdef CONFIG_KVM_ARM_TIMER
>    DEFINE(VCPU_TIMER_CNTV_CTL,  offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
>    DEFINE(VCPU_TIMER_CNTV_CVAL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval));
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 76af9302..9fbbf99 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -419,7 +419,9 @@ vcpu        .req    r0              @ vcpu pointer always in r0
>         ldr     r7, [r2, #GICH_EISR1]
>         ldr     r8, [r2, #GICH_ELRSR0]
>         ldr     r9, [r2, #GICH_ELRSR1]
> -       ldr     r10, [r2, #GICH_APR]
> +       ldr     r10, [r11, #VGIC_CPU_HW_CFG]
> +       mov     r10, r10, lsr #HWCFG_APR_SHIFT
> +       ldr     r10, [r2, r10]
>
>         str     r3, [r11, #VGIC_CPU_HCR]
>         str     r4, [r11, #VGIC_CPU_VMCR]
> @@ -435,9 +437,15 @@ vcpu       .req    r0              @ vcpu pointer always in r0
>         str     r5, [r2, #GICH_HCR]
>
>         /* Save list registers */
> -       add     r2, r2, #GICH_LR0
> +       ldr     r4, [r11, #VGIC_CPU_HW_CFG]

Can you find a way to avoid this reload?

> +       mov     r10, r4, lsr #HWCFG_APR_SHIFT
> +       /* the offset between GICH_APR & GICH_LR0 is 0x10 */
> +       add     r10, r10, #0x10
> +       add     r2, r2, r10
>         add     r3, r11, #VGIC_CPU_LR
> -       ldr     r4, [r11, #VGIC_CPU_NR_LR]
> +       /* Get NR_LR from VGIC_CPU_HW_CFG */
> +       ldr     r6, =HWCFG_NR_LR_MASK
> +       and     r4, r4, r6

Here, you're generating a memory access (loading HWCFG_NR_LR_MASK from
the constant pool), while the whole purpose of the exercise is to avoid
additional memory accesses.

Consider using the ubxf instruction to directly extract the information
you need. Actually, it is probably easier for me to directly show you
what I want to see (completely untested, of course):


See? No additional memory access compared to the original code.

>  1:     ldr     r6, [r2], #4
>         str     r6, [r3], #4
>         subs    r4, r4, #1
> @@ -469,12 +477,21 @@ vcpu      .req    r0              @ vcpu pointer always in r0
>
>         str     r3, [r2, #GICH_HCR]
>         str     r4, [r2, #GICH_VMCR]
> -       str     r8, [r2, #GICH_APR]
> +       ldr     r6, [r11, #VGIC_CPU_HW_CFG]
> +       mov     r6, r6, lsr #HWCFG_APR_SHIFT
> +       str     r8, [r2, r6]
>
>         /* Restore list registers */
> -       add     r2, r2, #GICH_LR0
> +       ldr     r4, [r11, #VGIC_CPU_HW_CFG]
> +       mov     r6, r4, lsr #HWCFG_APR_SHIFT
> +       /* the offset between GICH_APR & GICH_LR0 is 0x10 */
> +       add     r6, r6, #0x10
> +       /* get offset of GICH_LR0 */
> +       add     r2, r2, r6
> +       /* Get NR_LR from VGIC_CPU_HW_CFG */
>         add     r3, r11, #VGIC_CPU_LR
> -       ldr     r4, [r11, #VGIC_CPU_NR_LR]
> +       ldr     r6, =HWCFG_NR_LR_MASK
> +       and     r4, r4, r6

See my comments above.

>  1:     ldr     r6, [r3], #4
>         str     r6, [r2], #4
>         subs    r4, r4, #1
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 646f888..2422358 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -136,7 +136,7 @@ int main(void)
>    DEFINE(VGIC_CPU_ELRSR,       offsetof(struct vgic_cpu, vgic_elrsr));
>    DEFINE(VGIC_CPU_APR,         offsetof(struct vgic_cpu, vgic_apr));
>    DEFINE(VGIC_CPU_LR,          offsetof(struct vgic_cpu, vgic_lr));
> -  DEFINE(VGIC_CPU_NR_LR,       offsetof(struct vgic_cpu, nr_lr));
> +  DEFINE(VGIC_CPU_HW_CFG,      offsetof(struct vgic_cpu, hw_cfg));
>    DEFINE(KVM_VTTBR,            offsetof(struct kvm, arch.vttbr));
>    DEFINE(KVM_VGIC_VCTRL,       offsetof(struct kvm, arch.vgic.vctrl_base));
>  #endif
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 2c56012..a4a8b3d 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -402,7 +402,9 @@ __kvm_hyp_code_start:
>         ldr     w8, [x2, #GICH_EISR1]
>         ldr     w9, [x2, #GICH_ELRSR0]
>         ldr     w10, [x2, #GICH_ELRSR1]
> -       ldr     w11, [x2, #GICH_APR]
> +       ldr     w11, [x3, #VGIC_CPU_HW_CFG]
> +       mov     w11, w11, lsr #HWCFG_APR_SHIFT
> +       ldr     w11, [x2, w10]
>  CPU_BE(        rev     w4,  w4  )
>  CPU_BE(        rev     w5,  w5  )
>  CPU_BE(        rev     w6,  w6  )
> @@ -425,8 +427,13 @@ CPU_BE(    rev     w11, w11 )
>         str     wzr, [x2, #GICH_HCR]
>
>         /* Save list registers */
> -       add     x2, x2, #GICH_LR0
> -       ldr     w4, [x3, #VGIC_CPU_NR_LR]
> +       ldr     w4, [x3, #VGIC_CPU_HW_CFG]
> +       mov     w6, w4, lsr #HWCFG_APR_SHIFT
> +       ldr     w7, =HWCFG_NR_LR_MASK

As there is no arm64 SoC with this GIC yet, don't bother hacking the
whole thing. Just extract the right field of hw_cfg, and this should be
enough. If one day someone builds such an insanity, we'll add the
necessary code.

> +       and     w4, w4, w7
> +       /* the offset between GICH_APR and GICH_LR0 is 0x10 */
> +       add     w6, w6, 0x10
> +       add     x2, x2, w6
>         add     x3, x3, #VGIC_CPU_LR
>  1:     ldr     w5, [x2], #4
>  CPU_BE(        rev     w5, w5 )
> @@ -461,11 +468,20 @@ CPU_BE(   rev     w6, w6 )
>
>         str     w4, [x2, #GICH_HCR]
>         str     w5, [x2, #GICH_VMCR]
> -       str     w6, [x2, #GICH_APR]
> +       ldr     w4, [x3, #VGIC_CPU_HW_CFG]
> +       mov     w4, w4, #HWCFG_APR_SHIFT
> +       str     w6, [x2, w4]
>
>         /* Restore list registers */
> -       add     x2, x2, #GICH_LR0
> -       ldr     w4, [x3, #VGIC_CPU_NR_LR]
> +       ldr     w4, [x3, #VGIC_CPU_HW_CFG]
> +       mov     w6, w4, #HWCFG_APR_SHIFT
> +       /* the offset between GICH_APR and GICH_LR0 is 0x10 */
> +       add     w6, w6, #0x10
> +       /* get offset of GICH_LR0 */
> +       add     x2, x2, w6
> +       /* get NR_LR from VGIC_CPU_HW_CFG */
> +       ldr     w6, =HWCFG_NR_LR_MASK
> +       and     w4, w4, w6
>         add     x3, x3, #VGIC_CPU_LR

Same here.

>  1:     ldr     w5, [x3], #4
>  CPU_BE(        rev     w5, w5 )
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index f27000f..eba4b51 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -122,8 +122,11 @@ struct vgic_cpu {
>         /* Bitmap of used/free list registers */
>         DECLARE_BITMAP( lr_used, VGIC_MAX_LRS);
>
> -       /* Number of list registers on this CPU */
> -       int             nr_lr;
> +       /*
> +        * bit[31:16]: GICH_APR offset
> +        * bit[15:0]:  Number of list registers on this CPU
> +        */
> +       u32             hw_cfg;
>
>         /* CPU vif control registers for world switch */
>         u32             vgic_hcr;
> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
> index 45e2d8c..b055f92 100644
> --- a/include/linux/irqchip/arm-gic.h
> +++ b/include/linux/irqchip/arm-gic.h
> @@ -49,6 +49,8 @@
>  #define GICH_ELRSR1                    0x34
>  #define GICH_APR                       0xf0
>  #define GICH_LR0                       0x100
> +#define HIP04_GICH_APR                 0x70
> +/* GICH_LR0 offset in HiP04 is 0x80 */
>
>  #define GICH_HCR_EN                    (1 << 0)
>  #define GICH_HCR_UIE                   (1 << 1)
> @@ -73,6 +75,10 @@
>  #define GICH_MISR_EOI                  (1 << 0)
>  #define GICH_MISR_U                    (1 << 1)
>
> +#define HWCFG_NR_LR_MASK       0xffff
> +#define HWCFG_APR_SHIFT                16
> +#define HWCFG_APR_MASK         (0xffff << HWCFG_APR_SHIFT)
> +
>  #ifndef __ASSEMBLY__
>
>  struct device_node;
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 47b2983..4c0c1e9 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -76,6 +76,8 @@
>  #define IMPLEMENTER_ARM                0x43b
>  #define GICC_ARCH_VERSION_V2   0x2
>
> +#define vgic_nr_lr(vcpu)       (vcpu->hw_cfg & HWCFG_NR_LR_MASK)
> +
>  /* Physical address of vgic virtual cpu interface */
>  static phys_addr_t vgic_vcpu_base;
>
> @@ -97,7 +99,7 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
>  static void vgic_update_state(struct kvm *kvm);
>  static void vgic_kick_vcpus(struct kvm *kvm);
>  static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg);
> -static u32 vgic_nr_lr;
> +static u32 vgic_hw_cfg;
>
>  static unsigned int vgic_maint_irq;
>
> @@ -624,9 +626,9 @@ static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
>         struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>         int vcpu_id = vcpu->vcpu_id;
>         int i, irq, source_cpu;
> -       u32 *lr;
> +       u32 *lr, nr_lr = vgic_nr_lr(vgic_cpu);
>
> -       for_each_set_bit(i, vgic_cpu->lr_used, vgic_cpu->nr_lr) {
> +       for_each_set_bit(i, vgic_cpu->lr_used, nr_lr) {
>                 lr = &vgic_cpu->vgic_lr[i];
>                 irq = LR_IRQID(*lr);
>                 source_cpu = LR_CPUID(*lr);
> @@ -1005,8 +1007,9 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu)
>  {
>         struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>         int lr;
> +       int nr_lr = vgic_nr_lr(vgic_cpu);
>
> -       for_each_set_bit(lr, vgic_cpu->lr_used, vgic_cpu->nr_lr) {
> +       for_each_set_bit(lr, vgic_cpu->lr_used, nr_lr) {
>                 int irq = vgic_cpu->vgic_lr[lr] & GICH_LR_VIRTUALID;
>
>                 if (!vgic_irq_is_enabled(vcpu, irq)) {
> @@ -1025,6 +1028,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>  {
>         struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>         int lr;
> +       int nr_lr = vgic_nr_lr(vgic_cpu);
>
>         /* Sanitize the input... */
>         BUG_ON(sgi_source_id & ~7);
> @@ -1046,9 +1050,8 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>         }
>
>         /* Try to use another LR for this interrupt */
> -       lr = find_first_zero_bit((unsigned long *)vgic_cpu->lr_used,
> -                              vgic_cpu->nr_lr);
> -       if (lr >= vgic_cpu->nr_lr)
> +       lr = find_first_zero_bit((unsigned long *)vgic_cpu->lr_used, nr_lr);
> +       if (lr >= nr_lr)
>                 return false;
>
>         kvm_debug("LR%d allocated for IRQ%d %x\n", lr, irq, sgi_source_id);
> @@ -1181,9 +1184,10 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
>                  * active bit.
>                  */
>                 int lr, irq;
> +               int nr_lr = vgic_nr_lr(vgic_cpu);
>
>                 for_each_set_bit(lr, (unsigned long *)vgic_cpu->vgic_eisr,
> -                                vgic_cpu->nr_lr) {
> +                                nr_lr) {
>                         irq = vgic_cpu->vgic_lr[lr] & GICH_LR_VIRTUALID;
>
>                         vgic_irq_clear_active(vcpu, irq);
> @@ -1221,13 +1225,13 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
>         struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>         struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>         int lr, pending;
> +       int nr_lr = vgic_nr_lr(vgic_cpu);
>         bool level_pending;
>
>         level_pending = vgic_process_maintenance(vcpu);
>
>         /* Clear mappings for empty LRs */
> -       for_each_set_bit(lr, (unsigned long *)vgic_cpu->vgic_elrsr,
> -                        vgic_cpu->nr_lr) {
> +       for_each_set_bit(lr, (unsigned long *)vgic_cpu->vgic_elrsr, nr_lr) {
>                 int irq;
>
>                 if (!test_and_clear_bit(lr, vgic_cpu->lr_used))
> @@ -1241,8 +1245,8 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
>
>         /* Check if we still have something up our sleeve... */
>         pending = find_first_zero_bit((unsigned long *)vgic_cpu->vgic_elrsr,
> -                                     vgic_cpu->nr_lr);
> -       if (level_pending || pending < vgic_cpu->nr_lr)
> +                                     nr_lr);
> +       if (level_pending || pending < nr_lr)
>                 set_bit(vcpu->vcpu_id, &dist->irq_pending_on_cpu);
>  }
>
> @@ -1438,7 +1442,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>          */
>         vgic_cpu->vgic_vmcr = 0;
>
> -       vgic_cpu->nr_lr = vgic_nr_lr;
> +       vgic_cpu->hw_cfg = vgic_hw_cfg;
>         vgic_cpu->vgic_hcr = GICH_HCR_EN; /* Get the show on the road... */
>
>         return 0;
> @@ -1470,17 +1474,32 @@ static struct notifier_block vgic_cpu_nb = {
>         .notifier_call = vgic_cpu_notify,
>  };
>
> +static const struct of_device_id of_vgic_ids[] = {
> +       {
> +               .compatible = "arm,cortex-a15-gic",
> +               .data = (void *)GICH_APR,
> +       }, {
> +               .compatible = "hisilicon,hip04-gic",
> +               .data = (void *)HIP04_GICH_APR,
> +       }, {
> +       },
> +};
> +
>  int kvm_vgic_hyp_init(void)
>  {
>         int ret;
>         struct resource vctrl_res;
>         struct resource vcpu_res;
> +       const struct of_device_id *match;
> +       u32 vgic_nr_lr;
>
> -       vgic_node = of_find_compatible_node(NULL, NULL, "arm,cortex-a15-gic");
> +       vgic_node = of_find_matching_node_and_match(NULL, of_vgic_ids, &match);
>         if (!vgic_node) {
>                 kvm_err("error: no compatible vgic node in DT\n");
>                 return -ENODEV;
>         }
> +       /* High word of vgic_hw_cfg is the offset of GICH_APR. */
> +       vgic_hw_cfg = (unsigned int)match->data << HWCFG_APR_SHIFT;
>
>         vgic_maint_irq = irq_of_parse_and_map(vgic_node, 0);
>         if (!vgic_maint_irq) {
> @@ -1517,6 +1536,7 @@ int kvm_vgic_hyp_init(void)
>
>         vgic_nr_lr = readl_relaxed(vgic_vctrl_base + GICH_VTR);
>         vgic_nr_lr = (vgic_nr_lr & 0x3f) + 1;
> +       vgic_hw_cfg |= vgic_nr_lr;
>
>         ret = create_hyp_io_mappings(vgic_vctrl_base,
>                                      vgic_vctrl_base + resource_size(&vctrl_res),
> --
> 1.9.1
>
>

On a separate note, I'm still waiting for an answer from you about how
the LRs differ between GICv2 and this implementation. We cannot possibly
enable KVM on this HW without knowing how it differs from the original
architecture.

Thanks,

	M.
diff mbox

Patch

diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index e4eaf30..77ddf87 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -410,6 +410,8 @@  vcpu	.req	r0		@ vcpu pointer always in r0
 
 	/* Compute the address of struct vgic_cpu */
 	add	r11, vcpu, #VCPU_VGIC_CPU
+	/* Get HW configuration */
+	ldr	r12, [r11, #VGIC_CPU_HW_CFG]
 
 	/* Save all interesting registers */
 	ldr	r3, [r2, #GICH_HCR]
@@ -419,7 +421,9 @@  vcpu	.req	r0		@ vcpu pointer always in r0
 	ldr	r7, [r2, #GICH_EISR1]
 	ldr	r8, [r2, #GICH_ELRSR0]
 	ldr	r9, [r2, #GICH_ELRSR1]
-	ldr	r10, [r2, #GICH_APR]
+	/* Extract APR offset */
+	ubfx	r10, r12, #16, #16
+	ldr	r10, [r2, r10]
 
 	str	r3, [r11, #VGIC_V2_CPU_HCR]
 	str	r4, [r11, #VGIC_V2_CPU_VMCR]
@@ -434,10 +438,16 @@  vcpu	.req	r0		@ vcpu pointer always in r0
 	mov	r5, #0
 	str	r5, [r2, #GICH_HCR]
 
+	/* Compute GICH_LR0 address */
+	ubfx	r6, r12, #16, #16
+	add	r6, r6, #0x10
+	add	r2, r2, r6
+
+	/* Extract NR_LR */
+	ubfx	r4, r12, #0, #16
+
 	/* Save list registers */
-	add	r2, r2, #GICH_LR0
 	add	r3, r11, #VGIC_V2_CPU_LR
-	ldr	r4, [r11, #VGIC_CPU_NR_LR]
 1:	ldr	r6, [r2], #4
 	str	r6, [r3], #4
 	subs	r4, r4, #1