Re: [PATCH 14/31] KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

[Marc Zyngier <marc.zyngier@arm.com>]

On Wed, 31 May 2017 08:33:05 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Marc,
> 
> On 30/05/2017 16:24, Marc Zyngier wrote:
> > On 30/05/17 08:48, Auger Eric wrote:  
> >> Hi Marc
> >>
> >> On 03/05/2017 12:45, Marc Zyngier wrote:  
> >>> Add a handler for writing the guest's view of the ICC_EOIR1_EL1
> >>> register. This involves dropping the priority of the interrupt,
> >>> and deactivating it if required (EOImode == 0).
> >>>
> >>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >>> ---
> >>>  include/linux/irqchip/arm-gic-v3.h |   2 +
> >>>  virt/kvm/arm/hyp/vgic-v3-sr.c      | 119 +++++++++++++++++++++++++++++++++++++
> >>>  2 files changed, 121 insertions(+)
> >>>
> >>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> >>> index 7610ea4e8337..c56d9bc2c904 100644
> >>> --- a/include/linux/irqchip/arm-gic-v3.h
> >>> +++ b/include/linux/irqchip/arm-gic-v3.h
> >>> @@ -403,6 +403,8 @@
> >>>  
> >>>  #define ICH_HCR_EN			(1 << 0)
> >>>  #define ICH_HCR_UIE			(1 << 1)
> >>> +#define ICH_HCR_EOIcount_SHIFT		27
> >>> +#define ICH_HCR_EOIcount_MASK		(0x1f << ICH_HCR_EOIcount_SHIFT)
> >>>  
> >>>  #define ICH_VMCR_CBPR_SHIFT		4
> >>>  #define ICH_VMCR_CBPR_MASK		(1 << ICH_VMCR_CBPR_SHIFT)
> >>> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
> >>> index 49aad1de3ac8..a76351b3ad66 100644
> >>> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
> >>> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
> >>> @@ -425,6 +425,26 @@ static int __hyp_text __vgic_v3_highest_priority_lr(struct kvm_vcpu *vcpu,
> >>>  	return lr;
> >>>  }
> >>>  
> >>> +static int __hyp_text __vgic_v3_find_active_lr(struct kvm_vcpu *vcpu,
> >>> +					       int intid, u64 *lr_val)
> >>> +{
> >>> +	unsigned int used_lrs = vcpu->arch.vgic_cpu.used_lrs;
> >>> +	int i;
> >>> +
> >>> +	for (i = 0; i < used_lrs; i++) {
> >>> +		u64 val = __gic_v3_get_lr(i);
> >>> +
> >>> +		if ((val & ICH_LR_VIRTUAL_ID_MASK) == intid &&
> >>> +		    (val & ICH_LR_ACTIVE_BIT)) {  
> >> I guess it is safe because we don't have yet virtual interrupts directly
> >> mapped to phys IRQs, besides timer one?  
> > 
> > What would that change? I don't see how having a HW interrupt here would
> > be unsafe... Am I missing something?  
> I was thinking about the case of an HW mapped interrupt whose active
> state must be observed at distributor level - as I understood the spec -
> and not at LR level.

What part of the spec are you referring to?

A virtual interrupt, even backed by a HW interrupt, does have an active
state (unless it is an LPI). The only state we cannot observe in the LR
when the HW bit is set is the Active+Pending state because the pending
bit is then at the physical distributor level.

For all intent and purposes, the active state in the LR behaves the
same, irrespective of the HW state, until the guest issues a
deactivation (either using EOI or DIR, depending on EOImode).

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.