Subject: [RFC PATCH 1/6] arm/arm64: KVM: Rename irq_state to irq_pending

From: [email-masked] (Eric Auger)
Date: Wed, 18 Jun 2014 16:30:23 +0200

On 06/14/2014 10:51 PM, Christoffer Dall wrote:
> The irq_state field on the distributor struct is ambiguous in its
> meaning; the comment says it's the level of the input put, but that
> doesn't make much sense for edge-triggered interrupts.  The code
> actually uses this state variable to check if the interrupt is in the
> pending state on the distributor so clarify the comment and rename the
> actual variable and accessor methods.
> 
> Signed-off-by: Christoffer Dall <[email-masked]>
> ---
>  include/kvm/arm_vgic.h |  4 ++--
>  virt/kvm/arm/vgic.c    | 52 +++++++++++++++++++++++++-------------------------
>  2 files changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index f27000f..a5d55d5 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -86,8 +86,8 @@ struct vgic_dist {
>  	/* Interrupt enabled (one bit per IRQ) */
>  	struct vgic_bitmap	irq_enabled;
>  
> -	/* Interrupt 'pin' level */
> -	struct vgic_bitmap	irq_state;
> +	/* Interrupt state is pending on the distributor */
> +	struct vgic_bitmap	irq_pending;
>  
>  	/* Level-triggered interrupt in progress */
>  	struct vgic_bitmap	irq_active;
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 47b2983..aba960a 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -37,7 +37,7 @@
>   *
>   * - At any time, the dist->irq_pending_on_cpu is the oracle that knows if
>   *   something is pending
> - * - VGIC pending interrupts are stored on the vgic.irq_state vgic
> + * - VGIC pending interrupts are stored on the vgic.irq_pending vgic
>   *   bitmap (this bitmap is updated by both user land ioctls and guest
>   *   mmio ops, and other in-kernel peripherals such as the
>   *   arch. timers) and indicate the 'wire' state.
Hi Christoffer,

Shouldn't we say that vgic.irq_pending is a combination of wire state
and soft pending set action(ISPENDR), corresponding to GIC archi spec
"status_includes_pending" - at least for level sensitive IRQS - ?

Best Regards

Eric

> @@ -45,8 +45,8 @@
>   *   recalculated
>   * - To calculate the oracle, we need info for each cpu from
>   *   compute_pending_for_cpu, which considers:
> - *   - PPI: dist->irq_state & dist->irq_enable
> - *   - SPI: dist->irq_state & dist->irq_enable & dist->irq_spi_target
> + *   - PPI: dist->irq_pending & dist->irq_enable
> + *   - SPI: dist->irq_pending & dist->irq_enable & dist->irq_spi_target
>   *   - irq_spi_target is a 'formatted' version of the GICD_ICFGR
>   *     registers, stored on each vcpu. We only keep one bit of
>   *     information per interrupt, making sure that only one vcpu can
> @@ -204,21 +204,21 @@ static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  
> -	return vgic_bitmap_get_irq_val(&dist->irq_state, vcpu->vcpu_id, irq);
> +	return vgic_bitmap_get_irq_val(&dist->irq_pending, vcpu->vcpu_id, irq);
>  }
>  
> -static void vgic_dist_irq_set(struct kvm_vcpu *vcpu, int irq)
> +static void vgic_dist_irq_set_pending(struct kvm_vcpu *vcpu, int irq)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  
> -	vgic_bitmap_set_irq_val(&dist->irq_state, vcpu->vcpu_id, irq, 1);
> +	vgic_bitmap_set_irq_val(&dist->irq_pending, vcpu->vcpu_id, irq, 1);
>  }
>  
> -static void vgic_dist_irq_clear(struct kvm_vcpu *vcpu, int irq)
> +static void vgic_dist_irq_clear_pending(struct kvm_vcpu *vcpu, int irq)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  
> -	vgic_bitmap_set_irq_val(&dist->irq_state, vcpu->vcpu_id, irq, 0);
> +	vgic_bitmap_set_irq_val(&dist->irq_pending, vcpu->vcpu_id, irq, 0);
>  }
>  
>  static void vgic_cpu_irq_set(struct kvm_vcpu *vcpu, int irq)
> @@ -392,7 +392,7 @@ static bool handle_mmio_set_pending_reg(struct kvm_vcpu *vcpu,
>  					struct kvm_exit_mmio *mmio,
>  					phys_addr_t offset)
>  {
> -	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_state,
> +	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_pending,
>  				       vcpu->vcpu_id, offset);
>  	vgic_reg_access(mmio, reg, offset,
>  			ACCESS_READ_VALUE | ACCESS_WRITE_SETBIT);
> @@ -408,7 +408,7 @@ static bool handle_mmio_clear_pending_reg(struct kvm_vcpu *vcpu,
>  					  struct kvm_exit_mmio *mmio,
>  					  phys_addr_t offset)
>  {
> -	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_state,
> +	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_pending,
>  				       vcpu->vcpu_id, offset);
>  	vgic_reg_access(mmio, reg, offset,
>  			ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
> @@ -650,7 +650,7 @@ static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
>  		 * is fine, then we are only setting a few bits that were
>  		 * already set.
>  		 */
> -		vgic_dist_irq_set(vcpu, irq);
> +		vgic_dist_irq_set_pending(vcpu, irq);
>  		if (irq < VGIC_NR_SGIS)
>  			dist->irq_sgi_sources[vcpu_id][irq] |= 1 << source_cpu;
>  		*lr &= ~GICH_LR_PENDING_BIT;
> @@ -929,7 +929,7 @@ static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg)
>  	kvm_for_each_vcpu(c, vcpu, kvm) {
>  		if (target_cpus & 1) {
>  			/* Flag the SGI as pending */
> -			vgic_dist_irq_set(vcpu, sgi);
> +			vgic_dist_irq_set_pending(vcpu, sgi);
>  			dist->irq_sgi_sources[c][sgi] |= 1 << vcpu_id;
>  			kvm_debug("SGI%d from CPU%d to CPU%d\n", sgi, vcpu_id, c);
>  		}
> @@ -949,11 +949,11 @@ static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
>  	pend_percpu = vcpu->arch.vgic_cpu.pending_percpu;
>  	pend_shared = vcpu->arch.vgic_cpu.pending_shared;
>  
> -	pending = vgic_bitmap_get_cpu_map(&dist->irq_state, vcpu_id);
> +	pending = vgic_bitmap_get_cpu_map(&dist->irq_pending, vcpu_id);
>  	enabled = vgic_bitmap_get_cpu_map(&dist->irq_enabled, vcpu_id);
>  	bitmap_and(pend_percpu, pending, enabled, VGIC_NR_PRIVATE_IRQS);
>  
> -	pending = vgic_bitmap_get_shared_map(&dist->irq_state);
> +	pending = vgic_bitmap_get_shared_map(&dist->irq_pending);
>  	enabled = vgic_bitmap_get_shared_map(&dist->irq_enabled);
>  	bitmap_and(pend_shared, pending, enabled, VGIC_NR_SHARED_IRQS);
>  	bitmap_and(pend_shared, pend_shared,
> @@ -1085,7 +1085,7 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
>  	 * our emulated gic and can get rid of them.
>  	 */
>  	if (!sources) {
> -		vgic_dist_irq_clear(vcpu, irq);
> +		vgic_dist_irq_clear_pending(vcpu, irq);
>  		vgic_cpu_irq_clear(vcpu, irq);
>  		return true;
>  	}
> @@ -1100,7 +1100,7 @@ static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
>  
>  	if (vgic_queue_irq(vcpu, 0, irq)) {
>  		if (vgic_irq_is_edge(vcpu, irq)) {
> -			vgic_dist_irq_clear(vcpu, irq);
> +			vgic_dist_irq_clear_pending(vcpu, irq);
>  			vgic_cpu_irq_clear(vcpu, irq);
>  		} else {
>  			vgic_irq_set_active(vcpu, irq);
> @@ -1297,7 +1297,7 @@ static void vgic_kick_vcpus(struct kvm *kvm)
>  
>  static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
>  {
> -	int is_edge = vgic_irq_is_edge(vcpu, irq);
> +	int edge_triggered = vgic_irq_is_edge(vcpu, irq);
>  	int state = vgic_dist_irq_is_pending(vcpu, irq);
>  
>  	/*
> @@ -1305,26 +1305,26 @@ static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
>  	 * - edge triggered and we have a rising edge
>  	 * - level triggered and we change level
>  	 */
> -	if (is_edge)
> +	if (edge_triggered)
>  		return level > state;
>  	else
>  		return level != state;
>  }
>  
> -static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
> +static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>  				  unsigned int irq_num, bool level)
>  {
>  	struct vgic_dist *dist = &kvm->arch.vgic;
>  	struct kvm_vcpu *vcpu;
> -	int is_edge, is_level;
> +	int edge_triggered, level_triggered;
>  	int enabled;
>  	bool ret = true;
>  
>  	spin_lock(&dist->lock);
>  
>  	vcpu = kvm_get_vcpu(kvm, cpuid);
> -	is_edge = vgic_irq_is_edge(vcpu, irq_num);
> -	is_level = !is_edge;
> +	edge_triggered = vgic_irq_is_edge(vcpu, irq_num);
> +	level_triggered = !edge_triggered;
>  
>  	if (!vgic_validate_injection(vcpu, irq_num, level)) {
>  		ret = false;
> @@ -1339,9 +1339,9 @@ static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
>  	kvm_debug("Inject IRQ%d level %d CPU%d\n", irq_num, level, cpuid);
>  
>  	if (level)
> -		vgic_dist_irq_set(vcpu, irq_num);
> +		vgic_dist_irq_set_pending(vcpu, irq_num);
>  	else
> -		vgic_dist_irq_clear(vcpu, irq_num);
> +		vgic_dist_irq_clear_pending(vcpu, irq_num);
>  
>  	enabled = vgic_irq_is_enabled(vcpu, irq_num);
>  
> @@ -1350,7 +1350,7 @@ static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
>  		goto out;
>  	}
>  
> -	if (is_level && vgic_irq_is_active(vcpu, irq_num)) {
> +	if (level_triggered && vgic_irq_is_active(vcpu, irq_num)) {
>  		/*
>  		 * Level interrupt in progress, will be picked up
>  		 * when EOId.
> @@ -1387,7 +1387,7 @@ out:
>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>  			bool level)
>  {
> -	if (vgic_update_irq_state(kvm, cpuid, irq_num, level))
> +	if (vgic_update_irq_pending(kvm, cpuid, irq_num, level))
>  		vgic_kick_vcpus(kvm);
>  
>  	return 0;
>