Re: [PATCH 4/9] KVM: arm/arm64: vgic: Get rid of unnecessary process_maintenance operationjjjj

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 21/03/17 12:26, Christoffer Dall wrote:
> On Tue, Mar 21, 2017 at 11:36:19AM +0000, Marc Zyngier wrote:
>> On 20/03/17 10:58, Christoffer Dall wrote:
>>> From: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
>>>
>>> Since we always read back the LRs that we wrote to the guest and the
>>> MISR and EISR registers simply provide a summary of the configuration of
>>> the bits in the LRs, there is really no need to read back those status
>>> registers and process them.  We can might as well just signal the
>>> notifyfd when folding the LR state and save some cycles in the process.
>>>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
>>> ---
>>>  virt/kvm/arm/vgic/vgic-v2.c | 55 ++++++++++-----------------------------------
>>>  virt/kvm/arm/vgic/vgic-v3.c | 47 +++++++++++---------------------------
>>>  virt/kvm/arm/vgic/vgic.c    |  8 +++----
>>>  virt/kvm/arm/vgic/vgic.h    |  4 ++--
>>>  4 files changed, 31 insertions(+), 83 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
>>> index dfe6e5e..0172754 100644
>>> --- a/virt/kvm/arm/vgic/vgic-v2.c
>>> +++ b/virt/kvm/arm/vgic/vgic-v2.c
>>> @@ -22,20 +22,6 @@
>>>  
>>>  #include "vgic.h"
>>>  
>>> -/*
>>> - * Call this function to convert a u64 value to an unsigned long * bitmask
>>> - * in a way that works on both 32-bit and 64-bit LE and BE platforms.
>>> - *
>>> - * Warning: Calling this function may modify *val.
>>> - */
>>> -static unsigned long *u64_to_bitmask(u64 *val)
>>> -{
>>> -#if defined(CONFIG_CPU_BIG_ENDIAN) && BITS_PER_LONG == 32
>>> -	*val = (*val >> 32) | (*val << 32);
>>> -#endif
>>> -	return (unsigned long *)val;
>>> -}
>>> -
>>
>> Getting rid of u64_to_bitmask makes me feel slightly better...
>>
>>>  static inline void vgic_v2_write_lr(int lr, u32 val)
>>>  {
>>>  	void __iomem *base = kvm_vgic_global_state.vctrl_base;
>>> @@ -51,38 +37,10 @@ void vgic_v2_init_lrs(void)
>>>  		vgic_v2_write_lr(i, 0);
>>>  }
>>>  
>>> -void vgic_v2_process_maintenance(struct kvm_vcpu *vcpu)
>>> +void vgic_v2_clear_uie(struct kvm_vcpu *vcpu)
>>>  {
>>>  	struct vgic_v2_cpu_if *cpuif = &vcpu->arch.vgic_cpu.vgic_v2;
>>> -
>>> -	if (cpuif->vgic_misr & GICH_MISR_EOI) {
>>> -		u64 eisr = cpuif->vgic_eisr;
>>> -		unsigned long *eisr_bmap = u64_to_bitmask(&eisr);
>>> -		int lr;
>>> -
>>> -		for_each_set_bit(lr, eisr_bmap, kvm_vgic_global_state.nr_lr) {
>>> -			u32 intid = cpuif->vgic_lr[lr] & GICH_LR_VIRTUALID;
>>> -
>>> -			WARN_ON(cpuif->vgic_lr[lr] & GICH_LR_STATE);
>>> -
>>> -			/* Only SPIs require notification */
>>> -			if (vgic_valid_spi(vcpu->kvm, intid))
>>> -				kvm_notify_acked_irq(vcpu->kvm, 0,
>>> -						     intid - VGIC_NR_PRIVATE_IRQS);
>>> -		}
>>> -	}
>>> -
>>> -	/* check and disable underflow maintenance IRQ */
>>>  	cpuif->vgic_hcr &= ~GICH_HCR_UIE;
>>> -
>>> -	/*
>>> -	 * In the next iterations of the vcpu loop, if we sync the
>>> -	 * vgic state after flushing it, but before entering the guest
>>> -	 * (this happens for pending signals and vmid rollovers), then
>>> -	 * make sure we don't pick up any old maintenance interrupts
>>> -	 * here.
>>> -	 */
>>> -	cpuif->vgic_eisr = 0;
>>>  }
>>>  
>>>  void vgic_v2_set_underflow(struct kvm_vcpu *vcpu)
>>> @@ -92,6 +50,12 @@ void vgic_v2_set_underflow(struct kvm_vcpu *vcpu)
>>>  	cpuif->vgic_hcr |= GICH_HCR_UIE;
>>>  }
>>>  
>>> +static bool lr_signals_eoi_mi(u32 lr_val)
>>> +{
>>> +	return !(lr_val & GICH_LR_STATE) && (lr_val & GICH_LR_EOI) &&
>>> +	       !(lr_val & GICH_LR_HW);
>>> +}
>>> +
>>>  /*
>>>   * transfer the content of the LRs back into the corresponding ap_list:
>>>   * - active bit is transferred as is
>>> @@ -109,6 +73,11 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
>>>  		u32 intid = val & GICH_LR_VIRTUALID;
>>>  		struct vgic_irq *irq;
>>>  
>>> +		/* Notify fds when the guest EOI'ed a level-triggered SPI */
>>> +		if (lr_signals_eoi_mi(val) && vgic_valid_spi(vcpu->kvm, intid))
>>> +			kvm_notify_acked_irq(vcpu->kvm, 0,
>>> +					     intid - VGIC_NR_PRIVATE_IRQS);
>>> +
>>>  		irq = vgic_get_irq(vcpu->kvm, vcpu, intid);
>>>  
>>>  		spin_lock(&irq->irq_lock);
>>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>>> index 3d7796c..7880c5c 100644
>>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>>> @@ -21,42 +21,9 @@
>>>  
>>>  #include "vgic.h"
>>>  
>>> -void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
>>> +void vgic_v3_clear_uie(struct kvm_vcpu *vcpu)
>>>  {
>>>  	struct vgic_v3_cpu_if *cpuif = &vcpu->arch.vgic_cpu.vgic_v3;
>>> -	u32 model = vcpu->kvm->arch.vgic.vgic_model;
>>> -
>>> -	if (cpuif->vgic_misr & ICH_MISR_EOI) {
>>> -		unsigned long eisr_bmap = cpuif->vgic_eisr;
>>> -		int lr;
>>> -
>>> -		for_each_set_bit(lr, &eisr_bmap, kvm_vgic_global_state.nr_lr) {
>>> -			u32 intid;
>>> -			u64 val = cpuif->vgic_lr[lr];
>>> -
>>> -			if (model == KVM_DEV_TYPE_ARM_VGIC_V3)
>>> -				intid = val & ICH_LR_VIRTUAL_ID_MASK;
>>> -			else
>>> -				intid = val & GICH_LR_VIRTUALID;
>>> -
>>> -			WARN_ON(cpuif->vgic_lr[lr] & ICH_LR_STATE);
>>> -
>>> -			/* Only SPIs require notification */
>>> -			if (vgic_valid_spi(vcpu->kvm, intid))
>>> -				kvm_notify_acked_irq(vcpu->kvm, 0,
>>> -						     intid - VGIC_NR_PRIVATE_IRQS);
>>> -		}
>>> -
>>> -		/*
>>> -		 * In the next iterations of the vcpu loop, if we sync
>>> -		 * the vgic state after flushing it, but before
>>> -		 * entering the guest (this happens for pending
>>> -		 * signals and vmid rollovers), then make sure we
>>> -		 * don't pick up any old maintenance interrupts here.
>>> -		 */
>>> -		cpuif->vgic_eisr = 0;
>>> -	}
>>> -
>>>  	cpuif->vgic_hcr &= ~ICH_HCR_UIE;
>>>  }
>>>  
>>> @@ -67,6 +34,12 @@ void vgic_v3_set_underflow(struct kvm_vcpu *vcpu)
>>>  	cpuif->vgic_hcr |= ICH_HCR_UIE;
>>>  }
>>>  
>>> +static bool lr_signals_eoi_mi(u64 lr_val)
>>> +{
>>> +	return !(lr_val & ICH_LR_STATE) && (lr_val & ICH_LR_EOI) &&
>>> +	       !(lr_val & ICH_LR_HW);
>>> +}
>>> +
>>>  void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
>>>  {
>>>  	struct vgic_v3_cpu_if *cpuif = &vcpu->arch.vgic_cpu.vgic_v3;
>>> @@ -82,6 +55,12 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
>>>  			intid = val & ICH_LR_VIRTUAL_ID_MASK;
>>>  		else
>>>  			intid = val & GICH_LR_VIRTUALID;
>>> +
>>> +		/* Notify fds when the guest EOI'ed a level-triggered IRQ */
>>> +		if (lr_signals_eoi_mi(val) && vgic_valid_spi(vcpu->kvm, intid))
>>> +			kvm_notify_acked_irq(vcpu->kvm, 0,
>>> +					     intid - VGIC_NR_PRIVATE_IRQS);
>>> +
>>>  		irq = vgic_get_irq(vcpu->kvm, vcpu, intid);
>>>  		if (!irq)	/* An LPI could have been unmapped. */
>>>  			continue;
>>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>>> index 1436c2e..f5b3c25 100644
>>> --- a/virt/kvm/arm/vgic/vgic.c
>>> +++ b/virt/kvm/arm/vgic/vgic.c
>>> @@ -527,12 +527,12 @@ static void vgic_prune_ap_list(struct kvm_vcpu *vcpu)
>>>  	spin_unlock(&vgic_cpu->ap_list_lock);
>>>  }
>>>  
>>> -static inline void vgic_process_maintenance_interrupt(struct kvm_vcpu *vcpu)
>>> +static inline void vgic_clear_uie(struct kvm_vcpu *vcpu)
>>>  {
>>>  	if (kvm_vgic_global_state.type == VGIC_V2)
>>> -		vgic_v2_process_maintenance(vcpu);
>>> +		vgic_v2_clear_uie(vcpu);
>>>  	else
>>> -		vgic_v3_process_maintenance(vcpu);
>>> +		vgic_v3_clear_uie(vcpu);
>>>  }
>>>  
>>>  static inline void vgic_fold_lr_state(struct kvm_vcpu *vcpu)
>>> @@ -642,7 +642,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
>>>  	if (unlikely(!vgic_initialized(vcpu->kvm)))
>>>  		return;
>>>  
>>> -	vgic_process_maintenance_interrupt(vcpu);
>>> +	vgic_clear_uie(vcpu);
>>>  	vgic_fold_lr_state(vcpu);
>>
>> nit: If you moved vgic_*_clear_uie() into vgic_*_fold_lr_state, you
>> could get rid of this call, extra prototypes, and save yourself the
>> extra check. Not a big deal though.
>>
> 
> I thought about this, but the problem is that strictly speaking, you
> don't need to consume any LRs to set the underflow, you just need enough
> in the AP list, and it's theoretically possible that you have nr_lrs+1
> interrupts in the AP list which have all been migrated away.

But isn't that a problem we already have, independently of reworking the
above?

> So, it would require something like this including a rework in
> flush_lr_state to work with the following patch which only calls
> vgic_fold_lr_state when there are in fact anything live in LRs:
> 
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index e5cf930..4b1feef 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -601,10 +601,8 @@ static void vgic_flush_lr_state(struct kvm_vcpu *vcpu)
>  
>  	DEBUG_SPINLOCK_BUG_ON(!spin_is_locked(&vgic_cpu->ap_list_lock));
>  
> -	if (compute_ap_list_depth(vcpu) > kvm_vgic_global_state.nr_lr) {
> -		vgic_set_underflow(vcpu);
> +	if (compute_ap_list_depth(vcpu) > kvm_vgic_global_state.nr_lr)
>  		vgic_sort_ap_list(vcpu);
> -	}
>  
>  	list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) {
>  		spin_lock(&irq->irq_lock);
> @@ -623,8 +621,12 @@ static void vgic_flush_lr_state(struct kvm_vcpu *vcpu)
>  next:
>  		spin_unlock(&irq->irq_lock);
>  
> -		if (count == kvm_vgic_global_state.nr_lr)
> +		if (count == kvm_vgic_global_state.nr_lr) {
> +			if (!list_is_last(&irq->ap_list,
> +					  &vgic_cpu->ap_list_head))
> +				vgic_set_underflow(vcpu);
>  			break;
> +		}
>  	}
>  
>  	vcpu->arch.vgic_cpu.used_lrs = count;
> 
> What do you think?

I think this deserves a patch of its own. Also, how about renaming
clear_uie to clear_underflow?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux