diff mbox

[v3,07/19] KVM: ARM: vgic: abstract access to the ELRSR bitmap

Message ID 1397655591-2761-8-git-send-email-marc.zyngier@arm.com
State New
Headers show

Commit Message

Marc Zyngier April 16, 2014, 1:39 p.m. UTC 
Move the GICH_ELRSR access to its own function, and add it to the
vgic_ops structure.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/kvm/arm_vgic.h |  1 +
 virt/kvm/arm/vgic.c    | 20 ++++++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

Comments

Christoffer Dall May 9, 2014, 2:06 p.m. UTC | #1 
On Wed, Apr 16, 2014 at 02:39:39PM +0100, Marc Zyngier wrote:
> Move the GICH_ELRSR access to its own function, and add it to the
> vgic_ops structure.
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  include/kvm/arm_vgic.h |  1 +
>  virt/kvm/arm/vgic.c    | 20 ++++++++++++++++----
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 17bbe51..01013ec 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -84,6 +84,7 @@ struct vgic_lr {
>  struct vgic_ops {
>  	struct vgic_lr	(*get_lr)(const struct kvm_vcpu *, int);
>  	void	(*set_lr)(struct kvm_vcpu *, int, struct vgic_lr);
> +	u64	(*get_elrsr)(const struct kvm_vcpu *vcpu);
>  };
>  
>  struct vgic_dist {
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index bac37b8..04206a8 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1022,9 +1022,16 @@ static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
>  		set_bit(lr, (unsigned long *)vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr);
>  }
>  
> +static u64 vgic_v2_get_elrsr(const struct kvm_vcpu *vcpu)
> +{
> +	const u32 *elrsr = vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr;
> +	return *(u64 *)elrsr;
> +}
> +
>  static const struct vgic_ops vgic_ops = {
>  	.get_lr			= vgic_v2_get_lr,
>  	.set_lr			= vgic_v2_set_lr,
> +	.get_elrsr		= vgic_v2_get_elrsr,
>  };
>  
>  static inline struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr)
> @@ -1038,6 +1045,11 @@ static inline void vgic_set_lr(struct kvm_vcpu *vcpu, int lr,
>  	vgic_ops.set_lr(vcpu, lr, vlr);
>  }
>  
> +static inline u64 vgic_get_elrsr(struct kvm_vcpu *vcpu)
> +{
> +	return vgic_ops.get_elrsr(vcpu);
> +}
> +
>  static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
>  {
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> @@ -1278,14 +1290,15 @@ 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;
> +	u64 elrsr = vgic_get_elrsr(vcpu);
> +	unsigned long *elrsr_ptr = (unsigned long *)&elrsr;

looks to me like you're changing functionality here,
vgic_process_maintenance() called below can modify the elrsr bitmap, but
you're copying it into elrsr (local variable) here.  Why du you even
need the temporary elrsr variable here, and not just declare elrsr_ptr btw.?

>  	int lr, pending;
>  	bool level_pending;
>  
>  	level_pending = vgic_process_maintenance(vcpu);
>  
>  	/* Clear mappings for empty LRs */
> -	for_each_set_bit(lr, (unsigned long *)vgic_cpu->vgic_v2.vgic_elrsr,
> -			 vgic_cpu->nr_lr) {
> +	for_each_set_bit(lr, elrsr_ptr, vgic_cpu->nr_lr) {
>  		struct vgic_lr vlr;
>  
>  		if (!test_and_clear_bit(lr, vgic_cpu->lr_used))
> @@ -1298,8 +1311,7 @@ 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_v2.vgic_elrsr,
> -				      vgic_cpu->nr_lr);
> +	pending = find_first_zero_bit(elrsr_ptr, vgic_cpu->nr_lr);
>  	if (level_pending || pending < vgic_cpu->nr_lr)
>  		set_bit(vcpu->vcpu_id, &dist->irq_pending_on_cpu);
>  }
> -- 
> 1.8.3.4
> 
-Christoffer
Marc Zyngier May 12, 2014, 5:41 p.m. UTC | #2 
On Fri, May 09 2014 at  3:06:03 pm BST, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Wed, Apr 16, 2014 at 02:39:39PM +0100, Marc Zyngier wrote:
>> Move the GICH_ELRSR access to its own function, and add it to the
>> vgic_ops structure.
>> 
>> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  include/kvm/arm_vgic.h |  1 +
>>  virt/kvm/arm/vgic.c    | 20 ++++++++++++++++----
>>  2 files changed, 17 insertions(+), 4 deletions(-)
>> 
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 17bbe51..01013ec 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -84,6 +84,7 @@ struct vgic_lr {
>>  struct vgic_ops {
>>  	struct vgic_lr	(*get_lr)(const struct kvm_vcpu *, int);
>>  	void	(*set_lr)(struct kvm_vcpu *, int, struct vgic_lr);
>> +	u64	(*get_elrsr)(const struct kvm_vcpu *vcpu);
>>  };
>>  
>>  struct vgic_dist {
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index bac37b8..04206a8 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -1022,9 +1022,16 @@ static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
>>  		set_bit(lr, (unsigned long *)vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr);
>>  }
>>  
>> +static u64 vgic_v2_get_elrsr(const struct kvm_vcpu *vcpu)
>> +{
>> +	const u32 *elrsr = vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr;
>> +	return *(u64 *)elrsr;
>> +}
>> +
>>  static const struct vgic_ops vgic_ops = {
>>  	.get_lr			= vgic_v2_get_lr,
>>  	.set_lr			= vgic_v2_set_lr,
>> +	.get_elrsr		= vgic_v2_get_elrsr,
>>  };
>>  
>>  static inline struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr)
>> @@ -1038,6 +1045,11 @@ static inline void vgic_set_lr(struct kvm_vcpu *vcpu, int lr,
>>  	vgic_ops.set_lr(vcpu, lr, vlr);
>>  }
>>  
>> +static inline u64 vgic_get_elrsr(struct kvm_vcpu *vcpu)
>> +{
>> +	return vgic_ops.get_elrsr(vcpu);
>> +}
>> +
>>  static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
>>  {
>>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>> @@ -1278,14 +1290,15 @@ 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;
>> +	u64 elrsr = vgic_get_elrsr(vcpu);
>> +	unsigned long *elrsr_ptr = (unsigned long *)&elrsr;
>
> looks to me like you're changing functionality here,
> vgic_process_maintenance() called below can modify the elrsr bitmap, but
> you're copying it into elrsr (local variable) here.  Why du you even
> need the temporary elrsr variable here, and not just declare elrsr_ptr btw.?

Duh. Nice one. The whole reason for elrsr_ptr is to have a pointer to an
unsigned long which the bitmap ops can use... Don't say a word, I know.

>>  	int lr, pending;
>>  	bool level_pending;
>>  
>>  	level_pending = vgic_process_maintenance(vcpu);
>>  
>>  	/* Clear mappings for empty LRs */
>> -	for_each_set_bit(lr, (unsigned long *)vgic_cpu->vgic_v2.vgic_elrsr,
>> -			 vgic_cpu->nr_lr) {
>> +	for_each_set_bit(lr, elrsr_ptr, vgic_cpu->nr_lr) {
>>  		struct vgic_lr vlr;
>>  
>>  		if (!test_and_clear_bit(lr, vgic_cpu->lr_used))
>> @@ -1298,8 +1311,7 @@ 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_v2.vgic_elrsr,
>> -				      vgic_cpu->nr_lr);
>> +	pending = find_first_zero_bit(elrsr_ptr, vgic_cpu->nr_lr);
>>  	if (level_pending || pending < vgic_cpu->nr_lr)
>>  		set_bit(vcpu->vcpu_id, &dist->irq_pending_on_cpu);
>>  }
>> -- 
>> 1.8.3.4
>> 
> -Christoffer
>
diff mbox

Patch

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 17bbe51..01013ec 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -84,6 +84,7 @@  struct vgic_lr {
 struct vgic_ops {
 	struct vgic_lr	(*get_lr)(const struct kvm_vcpu *, int);
 	void	(*set_lr)(struct kvm_vcpu *, int, struct vgic_lr);
+	u64	(*get_elrsr)(const struct kvm_vcpu *vcpu);
 };
 
 struct vgic_dist {
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index bac37b8..04206a8 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1022,9 +1022,16 @@  static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
 		set_bit(lr, (unsigned long *)vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr);
 }
 
+static u64 vgic_v2_get_elrsr(const struct kvm_vcpu *vcpu)
+{
+	const u32 *elrsr = vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr;
+	return *(u64 *)elrsr;
+}
+
 static const struct vgic_ops vgic_ops = {
 	.get_lr			= vgic_v2_get_lr,
 	.set_lr			= vgic_v2_set_lr,
+	.get_elrsr		= vgic_v2_get_elrsr,
 };
 
 static inline struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr)
@@ -1038,6 +1045,11 @@  static inline void vgic_set_lr(struct kvm_vcpu *vcpu, int lr,
 	vgic_ops.set_lr(vcpu, lr, vlr);
 }
 
+static inline u64 vgic_get_elrsr(struct kvm_vcpu *vcpu)
+{
+	return vgic_ops.get_elrsr(vcpu);
+}
+
 static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
@@ -1278,14 +1290,15 @@  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;
+	u64 elrsr = vgic_get_elrsr(vcpu);
+	unsigned long *elrsr_ptr = (unsigned long *)&elrsr;
 	int lr, pending;
 	bool level_pending;
 
 	level_pending = vgic_process_maintenance(vcpu);
 
 	/* Clear mappings for empty LRs */
-	for_each_set_bit(lr, (unsigned long *)vgic_cpu->vgic_v2.vgic_elrsr,
-			 vgic_cpu->nr_lr) {
+	for_each_set_bit(lr, elrsr_ptr, vgic_cpu->nr_lr) {
 		struct vgic_lr vlr;
 
 		if (!test_and_clear_bit(lr, vgic_cpu->lr_used))
@@ -1298,8 +1311,7 @@  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_v2.vgic_elrsr,
-				      vgic_cpu->nr_lr);
+	pending = find_first_zero_bit(elrsr_ptr, vgic_cpu->nr_lr);
 	if (level_pending || pending < vgic_cpu->nr_lr)
 		set_bit(vcpu->vcpu_id, &dist->irq_pending_on_cpu);
 }