diff mbox

[v6,15/15] virt: arm: support hip04 gic

Message ID 1399795571-17231-16-git-send-email-haojian.zhuang@linaro.org
State Changes Requested
Headers show

Commit Message

Haojian Zhuang May 11, 2014, 8:06 a.m. UTC 
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/kvm/interrupts_head.S  | 32 ++++++++++++++++++++++------
 include/kvm/arm_vgic.h          |  5 ++++-
 include/linux/irqchip/arm-gic.h |  6 ++++++
 virt/kvm/arm/vgic.c             | 47 +++++++++++++++++++++++++++++------------
 4 files changed, 69 insertions(+), 21 deletions(-)

Comments

Marc Zyngier May 15, 2014, 9:42 a.m. UTC | #1 
On 11/05/14 09:06, Haojian Zhuang 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/kvm/interrupts_head.S  | 32 ++++++++++++++++++++++------
>  include/kvm/arm_vgic.h          |  5 ++++-
>  include/linux/irqchip/arm-gic.h |  6 ++++++
>  virt/kvm/arm/vgic.c             | 47 +++++++++++++++++++++++++++++------------
>  4 files changed, 69 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 76af9302..7aacaff 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -419,7 +419,10 @@ 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_NR_LR]

Please rename this field to something else, now that it contains more
than the number of LRs.

> +	movs	r10, r10, lsr #HWCFG_APR_SHIFT
> +	ldrne	r10, [r2, r10]
> +	ldreq	r10, [r2, #GICH_APR]

No. Just encode the offset there always. No need for a test.

>  	str	r3, [r11, #VGIC_CPU_HCR]
>  	str	r4, [r11, #VGIC_CPU_VMCR]
> @@ -435,9 +438,16 @@ vcpu	.req	r0		@ vcpu pointer always in r0
>  	str	r5, [r2, #GICH_HCR]
>  
>  	/* Save list registers */
> -	add	r2, r2, #GICH_LR0
> -	add	r3, r11, #VGIC_CPU_LR
>  	ldr	r4, [r11, #VGIC_CPU_NR_LR]
> +	addeq	r2, r2, #GICH_LR0
> +	movne	r10, r4, lsr #HWCFG_APR_SHIFT
> +	/* the offset between GICH_APR & GICH_LR0 is 0x10 */
> +	addne	r10, r10, #0x10
> +	addne	r2, r2, r10
> +	add	r3, r11, #VGIC_CPU_LR
> +	/* Get NR_LR from VGIC_CPU_NR_LR */
> +	ldr	r6, =HWCFG_NR_LR_MASK
> +	and	r4, r4, r6

And the above simplifies all this complicated mess.

>  1:	ldr	r6, [r2], #4
>  	str	r6, [r3], #4
>  	subs	r4, r4, #1
> @@ -469,12 +479,22 @@ 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_NR_LR]
> +	movs	r6, r6, lsr #HWCFG_APR_SHIFT
> +	strne	r8, [r2, r6]
> +	streq	r8, [r2, #GICH_APR]
>  
>  	/* Restore list registers */
> -	add	r2, r2, #GICH_LR0
> -	add	r3, r11, #VGIC_CPU_LR
>  	ldr	r4, [r11, #VGIC_CPU_NR_LR]
> +	addeq	r2, r2, #GICH_LR0
> +	movne	r6, r4, lsr #HWCFG_APR_SHIFT
> +	/* the offset between GICH_APR & GICH_LR0 is 0x10 */
> +	addne	r6, r6, #0x10
> +	addne	r2, r2, r6
> +	/* Get NR_LR from VGIC_CPU_NR_LR */
> +	add	r3, r11, #VGIC_CPU_LR
> +	ldr	r6, =HWCFG_NR_LR_MASK
> +	and	r4, r4, r6

Same here.

>  1:	ldr	r6, [r3], #4
>  	str	r6, [r2], #4
>  	subs	r4, r4, #1
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index f27000f..089de25 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -122,7 +122,10 @@ struct vgic_cpu {
>  	/* Bitmap of used/free list registers */
>  	DECLARE_BITMAP(	lr_used, VGIC_MAX_LRS);
>  
> -	/* Number of list registers on this CPU */
> +	/*
> +	 * bit[31:16]: GICH_APR offset
> +	 * bit[15:0]:  Number of list registers on this CPU
> +	 */
>  	int		nr_lr;

Make it a u32 to reflect the encoding, rename it to reflect the change
in functionality.

>  
>  	/* CPU vif control registers for world switch */
> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
> index 45e2d8c..5530388 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_MASK		0xffff
> +#define HWCFG_APR_SHIFT		16

HWCFG_APR_MASK is wrong, as it should be shifted.

>  #ifndef __ASSEMBLY__
>  
>  struct device_node;
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 47b2983..e635b9b 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -97,7 +97,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 +624,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_cpu->nr_lr & HWCFG_NR_LR_MASK;

Define an accessor (vgic_nr_lr(vcpu)). and use it everywhere you've
introduced this construct.

>  
> -	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 +1005,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_cpu->nr_lr & HWCFG_NR_LR_MASK;
>  
> -	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 +1026,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_cpu->nr_lr & HWCFG_NR_LR_MASK;
>  
>  	/* Sanitize the input... */
>  	BUG_ON(sgi_source_id & ~7);
> @@ -1046,9 +1048,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 +1182,10 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
>  		 * active bit.
>  		 */
>  		int lr, irq;
> +		int nr_lr = vgic_cpu->nr_lr & HWCFG_NR_LR_MASK;
>  
>  		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 +1223,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_cpu->nr_lr & HWCFG_NR_LR_MASK;
>  	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 +1243,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 +1440,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>  	 */
>  	vgic_cpu->vgic_vmcr = 0;
>  
> -	vgic_cpu->nr_lr = vgic_nr_lr;
> +	vgic_cpu->nr_lr = vgic_hw_cfg;
>  	vgic_cpu->vgic_hcr = GICH_HCR_EN; /* Get the show on the road... */
>  
>  	return 0;
> @@ -1470,17 +1472,33 @@ 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",
> +	}, {
> +		.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;
>  	}
> +	/* If it's standard ARM GIC, high word in vgic_hw_cfg is 0.
> +	 * Otherwise, it's the offset of non-standard GICH_APR (in HiP04).
> +	 */
> +	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 +1535,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),
> 

Thanks,

	M.
Haojian Zhuang May 15, 2014, 1:09 p.m. UTC | #2 
On 15 May 2014 17:42, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 11/05/14 09:06, Haojian Zhuang 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/kvm/interrupts_head.S  | 32 ++++++++++++++++++++++------
>>  include/kvm/arm_vgic.h          |  5 ++++-
>>  include/linux/irqchip/arm-gic.h |  6 ++++++
>>  virt/kvm/arm/vgic.c             | 47 +++++++++++++++++++++++++++++------------
>>  4 files changed, 69 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
>> index 76af9302..7aacaff 100644
>> --- a/arch/arm/kvm/interrupts_head.S
>> +++ b/arch/arm/kvm/interrupts_head.S
>> @@ -419,7 +419,10 @@ 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_NR_LR]
>
> Please rename this field to something else, now that it contains more
> than the number of LRs.

Since vgic driver is shared between arm & arm64, I only use high word
in VGIC_CPU_NR_LR register if SoC is HiP04. Then I could avoid to
change the vgic implementation in arm64.

Do you want to me change arm64 implementation at the same time?

>
>> +     movs    r10, r10, lsr #HWCFG_APR_SHIFT
>> +     ldrne   r10, [r2, r10]
>> +     ldreq   r10, [r2, #GICH_APR]
>
> No. Just encode the offset there always. No need for a test.
>

It's the same reason above.

>>       str     r3, [r11, #VGIC_CPU_HCR]
>>       str     r4, [r11, #VGIC_CPU_VMCR]
>> @@ -435,9 +438,16 @@ vcpu     .req    r0              @ vcpu pointer always in r0
>>       str     r5, [r2, #GICH_HCR]
>>
>>       /* Save list registers */
>> -     add     r2, r2, #GICH_LR0
>> -     add     r3, r11, #VGIC_CPU_LR
>>       ldr     r4, [r11, #VGIC_CPU_NR_LR]
>> +     addeq   r2, r2, #GICH_LR0
>> +     movne   r10, r4, lsr #HWCFG_APR_SHIFT
>> +     /* the offset between GICH_APR & GICH_LR0 is 0x10 */
>> +     addne   r10, r10, #0x10
>> +     addne   r2, r2, r10
>> +     add     r3, r11, #VGIC_CPU_LR
>> +     /* Get NR_LR from VGIC_CPU_NR_LR */
>> +     ldr     r6, =HWCFG_NR_LR_MASK
>> +     and     r4, r4, r6
>
> And the above simplifies all this complicated mess.
>
Same reason.

>>  1:   ldr     r6, [r2], #4
>>       str     r6, [r3], #4
>>       subs    r4, r4, #1
>> @@ -469,12 +479,22 @@ 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_NR_LR]
>> +     movs    r6, r6, lsr #HWCFG_APR_SHIFT
>> +     strne   r8, [r2, r6]
>> +     streq   r8, [r2, #GICH_APR]
>>
>>       /* Restore list registers */
>> -     add     r2, r2, #GICH_LR0
>> -     add     r3, r11, #VGIC_CPU_LR
>>       ldr     r4, [r11, #VGIC_CPU_NR_LR]
>> +     addeq   r2, r2, #GICH_LR0
>> +     movne   r6, r4, lsr #HWCFG_APR_SHIFT
>> +     /* the offset between GICH_APR & GICH_LR0 is 0x10 */
>> +     addne   r6, r6, #0x10
>> +     addne   r2, r2, r6
>> +     /* Get NR_LR from VGIC_CPU_NR_LR */
>> +     add     r3, r11, #VGIC_CPU_LR
>> +     ldr     r6, =HWCFG_NR_LR_MASK
>> +     and     r4, r4, r6
>
> Same here.
>
Same reason.

>>  1:   ldr     r6, [r3], #4
>>       str     r6, [r2], #4
>>       subs    r4, r4, #1
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index f27000f..089de25 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -122,7 +122,10 @@ struct vgic_cpu {
>>       /* Bitmap of used/free list registers */
>>       DECLARE_BITMAP( lr_used, VGIC_MAX_LRS);
>>
>> -     /* Number of list registers on this CPU */
>> +     /*
>> +      * bit[31:16]: GICH_APR offset
>> +      * bit[15:0]:  Number of list registers on this CPU
>> +      */
>>       int             nr_lr;
>
> Make it a u32 to reflect the encoding, rename it to reflect the change
> in functionality.
>
OK. I'll change it to u32. And rename it in struct vgic_cpu.

>>
>>       /* CPU vif control registers for world switch */
>> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
>> index 45e2d8c..5530388 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_MASK               0xffff
>> +#define HWCFG_APR_SHIFT              16
>
> HWCFG_APR_MASK is wrong, as it should be shifted.
>
OK. I'll change it.

>>  #ifndef __ASSEMBLY__
>>
>>  struct device_node;
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 47b2983..e635b9b 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -97,7 +97,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 +624,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_cpu->nr_lr & HWCFG_NR_LR_MASK;
>
> Define an accessor (vgic_nr_lr(vcpu)). and use it everywhere you've
> introduced this construct.
>
>>
>> -     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 +1005,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_cpu->nr_lr & HWCFG_NR_LR_MASK;
>>
>> -     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 +1026,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_cpu->nr_lr & HWCFG_NR_LR_MASK;
>>
>>       /* Sanitize the input... */
>>       BUG_ON(sgi_source_id & ~7);
>> @@ -1046,9 +1048,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 +1182,10 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
>>                * active bit.
>>                */
>>               int lr, irq;
>> +             int nr_lr = vgic_cpu->nr_lr & HWCFG_NR_LR_MASK;
>>
>>               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 +1223,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_cpu->nr_lr & HWCFG_NR_LR_MASK;
>>       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 +1243,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 +1440,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>>        */
>>       vgic_cpu->vgic_vmcr = 0;
>>
>> -     vgic_cpu->nr_lr = vgic_nr_lr;
>> +     vgic_cpu->nr_lr = vgic_hw_cfg;
>>       vgic_cpu->vgic_hcr = GICH_HCR_EN; /* Get the show on the road... */
>>
>>       return 0;
>> @@ -1470,17 +1472,33 @@ 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",
>> +     }, {
>> +             .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;
>>       }
>> +     /* If it's standard ARM GIC, high word in vgic_hw_cfg is 0.
>> +      * Otherwise, it's the offset of non-standard GICH_APR (in HiP04).
>> +      */
>> +     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 +1535,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),
>>
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
Marc Zyngier May 15, 2014, 1:26 p.m. UTC | #3 
On 15/05/14 14:09, Haojian Zhuang wrote:
> On 15 May 2014 17:42, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 11/05/14 09:06, Haojian Zhuang 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/kvm/interrupts_head.S  | 32 ++++++++++++++++++++++------
>>>  include/kvm/arm_vgic.h          |  5 ++++-
>>>  include/linux/irqchip/arm-gic.h |  6 ++++++
>>>  virt/kvm/arm/vgic.c             | 47 +++++++++++++++++++++++++++++------------
>>>  4 files changed, 69 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
>>> index 76af9302..7aacaff 100644
>>> --- a/arch/arm/kvm/interrupts_head.S
>>> +++ b/arch/arm/kvm/interrupts_head.S
>>> @@ -419,7 +419,10 @@ 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_NR_LR]
>>
>> Please rename this field to something else, now that it contains more
>> than the number of LRs.
> 
> Since vgic driver is shared between arm & arm64, I only use high word
> in VGIC_CPU_NR_LR register if SoC is HiP04. Then I could avoid to
> change the vgic implementation in arm64.
> 
> Do you want to me change arm64 implementation at the same time?

Is there anything that guarantees we'll never see the same GIC connected
to an arm64 CPU? Probably not. So you might as well do it completely. At
the minimum, mask out the top bits of the nr_lr word.

Also, there is a number of things that are not quite clear about this
GICH implementation. Given how extensive the changes are in GICD, do the
LRs have the exact same format as GICv2 (i.e. supporting only 8 vcpus,
1020 interrupts)?

Thanks,

	M.
Christoffer Dall May 20, 2014, 10:51 a.m. UTC | #4 
On Thu, May 15, 2014 at 09:09:05PM +0800, Haojian Zhuang wrote:
> On 15 May 2014 17:42, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > On 11/05/14 09:06, Haojian Zhuang 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/kvm/interrupts_head.S  | 32 ++++++++++++++++++++++------
> >>  include/kvm/arm_vgic.h          |  5 ++++-
> >>  include/linux/irqchip/arm-gic.h |  6 ++++++
> >>  virt/kvm/arm/vgic.c             | 47 +++++++++++++++++++++++++++++------------
> >>  4 files changed, 69 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> >> index 76af9302..7aacaff 100644
> >> --- a/arch/arm/kvm/interrupts_head.S
> >> +++ b/arch/arm/kvm/interrupts_head.S
> >> @@ -419,7 +419,10 @@ 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_NR_LR]
> >
> > Please rename this field to something else, now that it contains more
> > than the number of LRs.
> 
> Since vgic driver is shared between arm & arm64, I only use high word
> in VGIC_CPU_NR_LR register if SoC is HiP04. Then I could avoid to
> change the vgic implementation in arm64.
> 
> Do you want to me change arm64 implementation at the same time?
> 
> >
> >> +     movs    r10, r10, lsr #HWCFG_APR_SHIFT
> >> +     ldrne   r10, [r2, r10]
> >> +     ldreq   r10, [r2, #GICH_APR]
> >
> > No. Just encode the offset there always. No need for a test.
> >

We don't want all these conditionals in the critical path and it makes
the code impossible to read, please follow Marc's advice.

Thanks,
-Christoffer
Haojian Zhuang July 1, 2014, 8:57 a.m. UTC | #5 
On 15 May 2014 21:26, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 15/05/14 14:09, Haojian Zhuang wrote:
>> On 15 May 2014 17:42, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 11/05/14 09:06, Haojian Zhuang 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/kvm/interrupts_head.S  | 32 ++++++++++++++++++++++------
>>>>  include/kvm/arm_vgic.h          |  5 ++++-
>>>>  include/linux/irqchip/arm-gic.h |  6 ++++++
>>>>  virt/kvm/arm/vgic.c             | 47 +++++++++++++++++++++++++++++------------
>>>>  4 files changed, 69 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
>>>> index 76af9302..7aacaff 100644
>>>> --- a/arch/arm/kvm/interrupts_head.S
>>>> +++ b/arch/arm/kvm/interrupts_head.S
>>>> @@ -419,7 +419,10 @@ 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_NR_LR]
>>>
>>> Please rename this field to something else, now that it contains more
>>> than the number of LRs.
>>
>> Since vgic driver is shared between arm & arm64, I only use high word
>> in VGIC_CPU_NR_LR register if SoC is HiP04. Then I could avoid to
>> change the vgic implementation in arm64.
>>
>> Do you want to me change arm64 implementation at the same time?
>
> Is there anything that guarantees we'll never see the same GIC connected
> to an arm64 CPU? Probably not. So you might as well do it completely. At
> the minimum, mask out the top bits of the nr_lr word.
>

I checked that this GIC is only connected to an arm32 CPU. So we don't need
to worry about the same offset issue on arm64.

> Also, there is a number of things that are not quite clear about this
> GICH implementation. Given how extensive the changes are in GICD, do the
> LRs have the exact same format as GICv2 (i.e. supporting only 8 vcpus,
> 1020 interrupts)?

In HiP04 GICH_LR0, the CPUID is in bit[13:10]. Other bits are same as GICv2.

If I only support 4 or 8 vcpus, I needn't to change code to support
it. Am I right?

Regards
Haojian
diff mbox

Patch

diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 76af9302..7aacaff 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -419,7 +419,10 @@  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_NR_LR]
+	movs	r10, r10, lsr #HWCFG_APR_SHIFT
+	ldrne	r10, [r2, r10]
+	ldreq	r10, [r2, #GICH_APR]
 
 	str	r3, [r11, #VGIC_CPU_HCR]
 	str	r4, [r11, #VGIC_CPU_VMCR]
@@ -435,9 +438,16 @@  vcpu	.req	r0		@ vcpu pointer always in r0
 	str	r5, [r2, #GICH_HCR]
 
 	/* Save list registers */
-	add	r2, r2, #GICH_LR0
-	add	r3, r11, #VGIC_CPU_LR
 	ldr	r4, [r11, #VGIC_CPU_NR_LR]
+	addeq	r2, r2, #GICH_LR0
+	movne	r10, r4, lsr #HWCFG_APR_SHIFT
+	/* the offset between GICH_APR & GICH_LR0 is 0x10 */
+	addne	r10, r10, #0x10
+	addne	r2, r2, r10
+	add	r3, r11, #VGIC_CPU_LR
+	/* Get NR_LR from VGIC_CPU_NR_LR */
+	ldr	r6, =HWCFG_NR_LR_MASK
+	and	r4, r4, r6
 1:	ldr	r6, [r2], #4
 	str	r6, [r3], #4
 	subs	r4, r4, #1
@@ -469,12 +479,22 @@  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_NR_LR]
+	movs	r6, r6, lsr #HWCFG_APR_SHIFT
+	strne	r8, [r2, r6]
+	streq	r8, [r2, #GICH_APR]
 
 	/* Restore list registers */
-	add	r2, r2, #GICH_LR0
-	add	r3, r11, #VGIC_CPU_LR
 	ldr	r4, [r11, #VGIC_CPU_NR_LR]
+	addeq	r2, r2, #GICH_LR0
+	movne	r6, r4, lsr #HWCFG_APR_SHIFT
+	/* the offset between GICH_APR & GICH_LR0 is 0x10 */
+	addne	r6, r6, #0x10
+	addne	r2, r2, r6
+	/* Get NR_LR from VGIC_CPU_NR_LR */
+	add	r3, r11, #VGIC_CPU_LR
+	ldr	r6, =HWCFG_NR_LR_MASK
+	and	r4, r4, r6
 1:	ldr	r6, [r3], #4
 	str	r6, [r2], #4
 	subs	r4, r4, #1
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index f27000f..089de25 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -122,7 +122,10 @@  struct vgic_cpu {
 	/* Bitmap of used/free list registers */
 	DECLARE_BITMAP(	lr_used, VGIC_MAX_LRS);
 
-	/* Number of list registers on this CPU */
+	/*
+	 * bit[31:16]: GICH_APR offset
+	 * bit[15:0]:  Number of list registers on this CPU
+	 */
 	int		nr_lr;
 
 	/* CPU vif control registers for world switch */
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 45e2d8c..5530388 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_MASK		0xffff
+#define HWCFG_APR_SHIFT		16
+
 #ifndef __ASSEMBLY__
 
 struct device_node;
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 47b2983..e635b9b 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -97,7 +97,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 +624,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_cpu->nr_lr & HWCFG_NR_LR_MASK;
 
-	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 +1005,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_cpu->nr_lr & HWCFG_NR_LR_MASK;
 
-	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 +1026,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_cpu->nr_lr & HWCFG_NR_LR_MASK;
 
 	/* Sanitize the input... */
 	BUG_ON(sgi_source_id & ~7);
@@ -1046,9 +1048,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 +1182,10 @@  static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
 		 * active bit.
 		 */
 		int lr, irq;
+		int nr_lr = vgic_cpu->nr_lr & HWCFG_NR_LR_MASK;
 
 		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 +1223,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_cpu->nr_lr & HWCFG_NR_LR_MASK;
 	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 +1243,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 +1440,7 @@  int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 	 */
 	vgic_cpu->vgic_vmcr = 0;
 
-	vgic_cpu->nr_lr = vgic_nr_lr;
+	vgic_cpu->nr_lr = vgic_hw_cfg;
 	vgic_cpu->vgic_hcr = GICH_HCR_EN; /* Get the show on the road... */
 
 	return 0;
@@ -1470,17 +1472,33 @@  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",
+	}, {
+		.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;
 	}
+	/* If it's standard ARM GIC, high word in vgic_hw_cfg is 0.
+	 * Otherwise, it's the offset of non-standard GICH_APR (in HiP04).
+	 */
+	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 +1535,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),