kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Marc Zyngier <maz@kernel.org>,
	linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org,
	kvmarm@lists.cs.columbia.edu
Cc: James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Eric Auger <eric.auger@redhat.com>,
	Hector Martin <marcan@marcan.st>,
	Mark Rutland <mark.rutland@arm.com>,
	kernel-team@android.com
Subject: Re: [PATCH v3 6/9] KVM: arm64: vgic: Implement SW-driven deactivation
Date: Mon, 24 May 2021 17:53:04 +0100	[thread overview]
Message-ID: <fbd86687-b0cb-9979-b0a1-7e67efdd6b0a@arm.com> (raw)
In-Reply-To: <20210510134824.1910399-7-maz@kernel.org>

Hi Marc,

Some questions regarding how this is supposed to work.

On 5/10/21 2:48 PM, Marc Zyngier wrote:
> In order to deal with these systems that do not offer HW-based
> deactivation of interrupts, let implement a SW-based approach:
>
> - When the irq is queued into a LR, treat it as a pure virtual
>   interrupt and set the EOI flag in the LR.
>
> - When the interrupt state is read back from the LR, force a
>   deactivation when the state is invalid (neither active nor
>   pending)
>
> Interrupts requiring such treatment get the VGIC_SW_RESAMPLE flag.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/vgic/vgic-v2.c | 19 +++++++++++++++----
>  arch/arm64/kvm/vgic/vgic-v3.c | 19 +++++++++++++++----
>  include/kvm/arm_vgic.h        | 10 ++++++++++
>  3 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
> index 11934c2af2f4..2c580204f1dc 100644
> --- a/arch/arm64/kvm/vgic/vgic-v2.c
> +++ b/arch/arm64/kvm/vgic/vgic-v2.c
> @@ -108,11 +108,22 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
>  		 * If this causes us to lower the level, we have to also clear
>  		 * the physical active state, since we will otherwise never be
>  		 * told when the interrupt becomes asserted again.
> +		 *
> +		 * Another case is when the interrupt requires a helping hand
> +		 * on deactivation (no HW deactivation, for example).
>  		 */
> -		if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT)) {
> -			irq->line_level = vgic_get_phys_line_level(irq);
> +		if (vgic_irq_is_mapped_level(irq)) {
> +			bool resample = false;
> +
> +			if (val & GICH_LR_PENDING_BIT) {
> +				irq->line_level = vgic_get_phys_line_level(irq);
> +				resample = !irq->line_level;
> +			} else if (vgic_irq_needs_resampling(irq) &&
> +				   !(irq->active || irq->pending_latch)) {

So this means that if the IRQ has the special flag, if it's not pending in the LR
or at the software level, and it's not active either, then perform interrupt
deactivation. I don't see where the state of the interrupt is checked again, am I
correct in assuming that we rely on the CPU interface to assert the interrupt to
the host while we run with interrupts enabled in the run loop, and the handler for
the interrupt will mark it pending for kvm_vgic_sync_hw_state->vgic_vx_fold_lr_state?

> +				resample = true;
> +			}
>  
> -			if (!irq->line_level)
> +			if (resample)

This name, "resample", is confusing to me, quite possibly because I'm not familiar
with the irqchip subsystem. It was my impression that "resample" means that at
some point, the physical interrupt state will be checked again, yet I don't see
that happening anywhere when VGIC_IRQ_SW_RESAMPLE is set. Am I mistaken in my
assumptions?

Thanks,

Alex

>  				vgic_irq_set_phys_active(irq, false);
>  		}
>  
> @@ -152,7 +163,7 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
>  	if (irq->group)
>  		val |= GICH_LR_GROUP1;
>  
> -	if (irq->hw) {
> +	if (irq->hw && !vgic_irq_needs_resampling(irq)) {
>  		val |= GICH_LR_HW;
>  		val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
>  		/*
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 41ecf219c333..66004f61cd83 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -101,11 +101,22 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
>  		 * If this causes us to lower the level, we have to also clear
>  		 * the physical active state, since we will otherwise never be
>  		 * told when the interrupt becomes asserted again.
> +		 *
> +		 * Another case is when the interrupt requires a helping hand
> +		 * on deactivation (no HW deactivation, for example).
>  		 */
> -		if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT)) {
> -			irq->line_level = vgic_get_phys_line_level(irq);
> +		if (vgic_irq_is_mapped_level(irq)) {
> +			bool resample = false;
> +
> +			if (val & ICH_LR_PENDING_BIT) {
> +				irq->line_level = vgic_get_phys_line_level(irq);
> +				resample = !irq->line_level;
> +			} else if (vgic_irq_needs_resampling(irq) &&
> +				   !(irq->active || irq->pending_latch)) {
> +				resample = true;
> +			}
>  
> -			if (!irq->line_level)
> +			if (resample)
>  				vgic_irq_set_phys_active(irq, false);
>  		}
>  
> @@ -136,7 +147,7 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
>  		}
>  	}
>  
> -	if (irq->hw) {
> +	if (irq->hw && !vgic_irq_needs_resampling(irq)) {
>  		val |= ICH_LR_HW;
>  		val |= ((u64)irq->hwintid) << ICH_LR_PHYS_ID_SHIFT;
>  		/*
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index e5f06df000f2..e602d848fc1a 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -99,6 +99,11 @@ enum vgic_irq_config {
>   * kvm_arm_get_running_vcpu() to get the vcpu pointer for private IRQs.
>   */
>  struct irq_ops {
> +	/* Per interrupt flags for special-cased interrupts */
> +	unsigned long flags;
> +
> +#define VGIC_IRQ_SW_RESAMPLE	BIT(0)	/* Clear the active state for resampling */
> +
>  	/*
>  	 * Callback function pointer to in-kernel devices that can tell us the
>  	 * state of the input level of mapped level-triggered IRQ faster than
> @@ -150,6 +155,11 @@ struct vgic_irq {
>  					   for in-kernel devices. */
>  };
>  
> +static inline bool vgic_irq_needs_resampling(struct vgic_irq *irq)
> +{
> +	return irq->ops && (irq->ops->flags & VGIC_IRQ_SW_RESAMPLE);
> +}
> +
>  struct vgic_register_region;
>  struct vgic_its;
>  

  reply	other threads:[~2021-05-24 16:52 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-10 13:48 [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Marc Zyngier
2021-05-10 13:48 ` [PATCH v3 1/9] irqchip/gic: Split vGIC probing information from the GIC code Marc Zyngier
2021-05-18 16:51   ` Alexandru Elisei
2021-05-10 13:48 ` [PATCH v3 2/9] KVM: arm64: Handle physical FIQ as an IRQ while running a guest Marc Zyngier
2021-05-20 17:46   ` Alexandru Elisei
2021-05-10 13:48 ` [PATCH v3 3/9] KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt Marc Zyngier
2021-05-10 16:19   ` Mark Rutland
2021-05-10 17:44     ` Marc Zyngier
2021-05-11 11:13       ` Mark Rutland
2021-05-10 13:48 ` [PATCH v3 4/9] KVM: arm64: vgic: Let an interrupt controller advertise lack of HW deactivation Marc Zyngier
2021-05-21 17:01   ` Alexandru Elisei
2021-05-24 17:17     ` Marc Zyngier
2021-05-10 13:48 ` [PATCH v3 5/9] KVM: arm64: vgic: move irq->get_input_level into an ops structure Marc Zyngier
2021-05-10 13:48 ` [PATCH v3 6/9] KVM: arm64: vgic: Implement SW-driven deactivation Marc Zyngier
2021-05-24 16:53   ` Alexandru Elisei [this message]
2021-05-24 17:43     ` Marc Zyngier
2021-05-10 13:48 ` [PATCH v3 7/9] KVM: arm64: timer: Refactor IRQ configuration Marc Zyngier
2021-05-14 12:46   ` Zenghui Yu
2021-05-24 17:48     ` Marc Zyngier
2021-05-10 13:48 ` [PATCH v3 8/9] KVM: arm64: timer: Add support for SW-based deactivation Marc Zyngier
2021-05-10 13:48 ` [PATCH v3 9/9] irqchip/apple-aic: Advertise some level of vGICv3 compatibility Marc Zyngier
2021-05-12 16:22 ` [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Alexandru Elisei
2021-05-12 16:33   ` Marc Zyngier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fbd86687-b0cb-9979-b0a1-7e67efdd6b0a@arm.com \
    --to=alexandru.elisei@arm.com \
    --cc=eric.auger@redhat.com \
    --cc=james.morse@arm.com \
    --cc=kernel-team@android.com \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=marcan@marcan.st \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=suzuki.poulose@arm.com \
    --subject='Re: [PATCH v3 6/9] KVM: arm64: vgic: Implement SW-driven deactivation' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).