linux-kernel - Re: [PATCH v2 5/8] KVM: arm/arm64: vgic: Handle mapped level sensitive SPIs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 21 Jul 2017 14:11:37 +0200
From:   Christoffer Dall <cdall@...aro.org>
To:     Eric Auger <eric.auger@...hat.com>
Cc:     eric.auger.pro@...il.com, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, kvmarm@...ts.cs.columbia.edu,
        alex.williamson@...hat.com, b.reynal@...tualopensystems.com,
        pbonzini@...hat.com, marc.zyngier@....com,
        christoffer.dall@...aro.org, drjones@...hat.com, wei@...hat.com
Subject: Re: [PATCH v2 5/8] KVM: arm/arm64: vgic: Handle mapped level
 sensitive SPIs

On Thu, Jun 15, 2017 at 02:52:37PM +0200, Eric Auger wrote:
> Currently, the line level of unmapped level sensitive SPIs is
> toggled down by the maintenance IRQ handler/resamplefd mechanism.
> 
> As mapped SPI completion is not trapped, we cannot rely on this
> mechanism and the line level needs to be observed at distributor
> level instead.
> 
> This patch handles the physical IRQ case in vgic_validate_injection
> and get the line level of a mapped SPI at distributor level.
> 
> Signed-off-by: Eric Auger <eric.auger@...hat.com>
> 
> ---
> 
> v1 -> v2:
> - renamed is_unshared_mapped into is_mapped_spi
> - changes to kvm_vgic_map_phys_irq moved in the previous patch
> - make vgic_validate_injection more readable
> - reword the commit message
> ---
>  virt/kvm/arm/vgic/vgic.c | 16 ++++++++++++++--
>  virt/kvm/arm/vgic/vgic.h |  7 ++++++-
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index 075f073..2e35ac7 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -139,6 +139,17 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
>  	kfree(irq);
>  }
>  
> +bool irq_line_level(struct vgic_irq *irq)
> +{
> +	bool line_level = irq->line_level;
> +
> +	if (unlikely(is_mapped_spi(irq)))
> +		WARN_ON(irq_get_irqchip_state(irq->host_irq,
> +					      IRQCHIP_STATE_PENDING,
> +					      &line_level));
> +	return line_level;
> +}
> +
>  /**
>   * kvm_vgic_target_oracle - compute the target vcpu for an irq
>   *
> @@ -236,13 +247,14 @@ static void vgic_sort_ap_list(struct kvm_vcpu *vcpu)
>  
>  /*
>   * Only valid injection if changing level for level-triggered IRQs or for a
> - * rising edge.
> + * rising edge. Injection of virtual interrupts associated to physical
> + * interrupts always is valid.

why?  I don't remember this now, and that means I probably won't in the
future either.

When I look at this now, I'm thinking, if we're not going to change
anything, why proceed beyond validate injection?

>   */
>  static bool vgic_validate_injection(struct vgic_irq *irq, bool level)
>  {
>  	switch (irq->config) {
>  	case VGIC_CONFIG_LEVEL:
> -		return irq->line_level != level;
> +		return (irq->line_level != level || unlikely(is_mapped_spi(irq)));
>  	case VGIC_CONFIG_EDGE:
>  		return level;
>  	}
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index bba7fa2..da254ae 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -96,14 +96,19 @@
>  /* we only support 64 kB translation table page size */
>  #define KVM_ITS_L1E_ADDR_MASK		GENMASK_ULL(51, 16)
>  
> +bool irq_line_level(struct vgic_irq *irq);
> +
>  static inline bool irq_is_pending(struct vgic_irq *irq)
>  {
>  	if (irq->config == VGIC_CONFIG_EDGE)
>  		return irq->pending_latch;
>  	else
> -		return irq->pending_latch || irq->line_level;
> +		return irq->pending_latch || irq_line_level(irq);
>  }
>  
> +#define is_mapped_spi(i) \
> +((i)->hw && (i)->intid >= VGIC_NR_PRIVATE_IRQS && (i)->intid < 1020)
> +

nit: why is this not a static inline ?

>  /*
>   * This struct provides an intermediate representation of the fields contained
>   * in the GICH_VMCR and ICH_VMCR registers, such that code exporting the GIC
> -- 
> 2.5.5
> 

Thanks,
-Christoffer