[PATCH] KVM: arm/arm64: Remove struct vgic_irq pending field
Andre Przywara
andre.przywara at arm.com
Mon Jan 23 09:57:05 PST 2017
Hi Christoffer,
On 23/01/17 13:39, Christoffer Dall wrote:
> One of the goals behind the VGIC redesign was to get rid of cached or
> intermediate state in the data structures, but we decided to allow
> ourselves to precompute the pending value of an IRQ based on the line
> level and pending latch state. However, this has now become difficult
> to base proper GICv3 save/restore on, because there is a potential to
> modify the pending state without knowing if an interrupt is edge or
> level configured.
>
> See the following post and related message for more background:
> https://lists.cs.columbia.edu/pipermail/kvmarm/2017-January/023195.html
>
> This commit gets rid of the precomputed pending field in favor of a
> function that calculates the value when needed, irq_is_pending().
>
> The soft_pending field is renamed to pending_latch to represent that
> this latch is the equivalent hardware latch which gets manipulated by
> the input signal for edge-triggered interrupts and when writing to the
> SPENDR/CPENDR registers.
>
> After this commit save/restore code should be able to simply restore the
> pending_latch state, line_level state, and config state in any order and
> get the desired result.
In general I like this very much and am wondering why we didn't come up
with this earlier. But I guess we were so subscribed to our "cached
pending" bit.
So I checked several cases, tested some logic equations and drew the
state diagrams for the "before" and "after" case, but I couldn't find a
reason why this wouldn't work.
I also think you can get rid of the irq_set_pending_latch() wrapper and
assign the value directly, as I don't see any reason to hide the fact
that it's indeed a simple assignment and nothing magic behind this
function. Also it would make the diff a bit easier to read.
But apart from that:
Reviewed-by: Andre Przywara <andre.przywara at arm.com>
I also gave this a quick test on the Juno and the Midway with both
kvmtool and QEMU and they survived some basic testing.
I need to check a GICv3 machine tomorrow, but I don't expect any
differences here.
Cheers,
Andre.
> Signed-off-by: Christoffer Dall <christoffer.dall at linaro.org>
> ---
> include/kvm/arm_vgic.h | 5 +++--
> virt/kvm/arm/vgic/vgic-its.c | 6 +++---
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 6 +++---
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 2 +-
> virt/kvm/arm/vgic/vgic-mmio.c | 19 +++++--------------
> virt/kvm/arm/vgic/vgic-v2.c | 12 +++++-------
> virt/kvm/arm/vgic/vgic-v3.c | 12 +++++-------
> virt/kvm/arm/vgic/vgic.c | 16 +++++++---------
> virt/kvm/arm/vgic/vgic.h | 13 +++++++++++++
> 9 files changed, 45 insertions(+), 46 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 002f092..da2ce08 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -101,9 +101,10 @@ struct vgic_irq {
> */
>
> u32 intid; /* Guest visible INTID */
> - bool pending;
> bool line_level; /* Level only */
> - bool soft_pending; /* Level only */
> + bool pending_latch; /* The pending latch state used to calculate
> + * the pending state for both level
> + * and edge triggered IRQs. */
> bool active; /* not used for LPIs */
> bool enabled;
> bool hw; /* Tied to HW IRQ */
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 8c2b3cd..7170a00 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -350,7 +350,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
>
> irq = vgic_get_irq(vcpu->kvm, NULL, intids[i]);
> spin_lock(&irq->irq_lock);
> - irq->pending = pendmask & (1U << bit_nr);
> + irq_set_pending_latch(irq, pendmask & (1U << bit_nr));
> vgic_queue_irq_unlock(vcpu->kvm, irq);
> vgic_put_irq(vcpu->kvm, irq);
> }
> @@ -465,7 +465,7 @@ static int vgic_its_trigger_msi(struct kvm *kvm, struct vgic_its *its,
> return -EBUSY;
>
> spin_lock(&itte->irq->irq_lock);
> - itte->irq->pending = true;
> + irq_set_pending_latch(itte->irq, true);
> vgic_queue_irq_unlock(kvm, itte->irq);
>
> return 0;
> @@ -913,7 +913,7 @@ static int vgic_its_cmd_handle_clear(struct kvm *kvm, struct vgic_its *its,
> if (!itte)
> return E_ITS_CLEAR_UNMAPPED_INTERRUPT;
>
> - itte->irq->pending = false;
> + irq_set_pending_latch(itte->irq, false);
>
> return 0;
> }
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 78e34bc..6b07fa9 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -98,7 +98,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu *source_vcpu,
> irq = vgic_get_irq(source_vcpu->kvm, vcpu, intid);
>
> spin_lock(&irq->irq_lock);
> - irq->pending = true;
> + irq_set_pending_latch(irq, true);
> irq->source |= 1U << source_vcpu->vcpu_id;
>
> vgic_queue_irq_unlock(source_vcpu->kvm, irq);
> @@ -182,7 +182,7 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu *vcpu,
>
> irq->source &= ~((val >> (i * 8)) & 0xff);
> if (!irq->source)
> - irq->pending = false;
> + irq_set_pending_latch(irq, false);
>
> spin_unlock(&irq->irq_lock);
> vgic_put_irq(vcpu->kvm, irq);
> @@ -204,7 +204,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,
> irq->source |= (val >> (i * 8)) & 0xff;
>
> if (irq->source) {
> - irq->pending = true;
> + irq_set_pending_latch(irq, true);
> vgic_queue_irq_unlock(vcpu->kvm, irq);
> } else {
> spin_unlock(&irq->irq_lock);
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 50f42f0..7300ec4 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -646,7 +646,7 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
> irq = vgic_get_irq(vcpu->kvm, c_vcpu, sgi);
>
> spin_lock(&irq->irq_lock);
> - irq->pending = true;
> + irq_set_pending_latch(irq, true);
>
> vgic_queue_irq_unlock(vcpu->kvm, irq);
> vgic_put_irq(vcpu->kvm, irq);
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index ebe1b9f..0dfd306 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -111,7 +111,7 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> for (i = 0; i < len * 8; i++) {
> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>
> - if (irq->pending)
> + if (irq_is_pending(irq))
> value |= (1U << i);
>
> vgic_put_irq(vcpu->kvm, irq);
> @@ -131,9 +131,7 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>
> spin_lock(&irq->irq_lock);
> - irq->pending = true;
> - if (irq->config == VGIC_CONFIG_LEVEL)
> - irq->soft_pending = true;
> + irq_set_pending_latch(irq, true);
>
> vgic_queue_irq_unlock(vcpu->kvm, irq);
> vgic_put_irq(vcpu->kvm, irq);
> @@ -152,12 +150,7 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
>
> spin_lock(&irq->irq_lock);
>
> - if (irq->config == VGIC_CONFIG_LEVEL) {
> - irq->soft_pending = false;
> - irq->pending = irq->line_level;
> - } else {
> - irq->pending = false;
> - }
> + irq_set_pending_latch(irq, false);
>
> spin_unlock(&irq->irq_lock);
> vgic_put_irq(vcpu->kvm, irq);
> @@ -359,12 +352,10 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,
> irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> spin_lock(&irq->irq_lock);
>
> - if (test_bit(i * 2 + 1, &val)) {
> + if (test_bit(i * 2 + 1, &val))
> irq->config = VGIC_CONFIG_EDGE;
> - } else {
> + else
> irq->config = VGIC_CONFIG_LEVEL;
> - irq->pending = irq->line_level | irq->soft_pending;
> - }
>
> spin_unlock(&irq->irq_lock);
> vgic_put_irq(vcpu->kvm, irq);
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index 834137e..a29cf33 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -104,7 +104,7 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
> /* Edge is the only case where we preserve the pending bit */
> if (irq->config == VGIC_CONFIG_EDGE &&
> (val & GICH_LR_PENDING_BIT)) {
> - irq->pending = true;
> + irq_set_pending_latch(irq, true);
>
> if (vgic_irq_is_sgi(intid)) {
> u32 cpuid = val & GICH_LR_PHYSID_CPUID;
> @@ -120,9 +120,7 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
> */
> if (irq->config == VGIC_CONFIG_LEVEL) {
> if (!(val & GICH_LR_PENDING_BIT))
> - irq->soft_pending = false;
> -
> - irq->pending = irq->line_level || irq->soft_pending;
> + irq_set_pending_latch(irq, false);
> }
>
> spin_unlock(&irq->irq_lock);
> @@ -145,11 +143,11 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
> {
> u32 val = irq->intid;
>
> - if (irq->pending) {
> + if (irq_is_pending(irq)) {
> val |= GICH_LR_PENDING_BIT;
>
> if (irq->config == VGIC_CONFIG_EDGE)
> - irq->pending = false;
> + irq_set_pending_latch(irq, false);
>
> if (vgic_irq_is_sgi(irq->intid)) {
> u32 src = ffs(irq->source);
> @@ -158,7 +156,7 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
> val |= (src - 1) << GICH_LR_PHYSID_CPUID_SHIFT;
> irq->source &= ~(1 << (src - 1));
> if (irq->source)
> - irq->pending = true;
> + irq_set_pending_latch(irq, true);
> }
> }
>
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index e6b03fd..76d7d75 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -94,7 +94,7 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
> /* Edge is the only case where we preserve the pending bit */
> if (irq->config == VGIC_CONFIG_EDGE &&
> (val & ICH_LR_PENDING_BIT)) {
> - irq->pending = true;
> + irq_set_pending_latch(irq, true);
>
> if (vgic_irq_is_sgi(intid) &&
> model == KVM_DEV_TYPE_ARM_VGIC_V2) {
> @@ -111,9 +111,7 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
> */
> if (irq->config == VGIC_CONFIG_LEVEL) {
> if (!(val & ICH_LR_PENDING_BIT))
> - irq->soft_pending = false;
> -
> - irq->pending = irq->line_level || irq->soft_pending;
> + irq_set_pending_latch(irq, false);
> }
>
> spin_unlock(&irq->irq_lock);
> @@ -127,11 +125,11 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
> u32 model = vcpu->kvm->arch.vgic.vgic_model;
> u64 val = irq->intid;
>
> - if (irq->pending) {
> + if (irq_is_pending(irq)) {
> val |= ICH_LR_PENDING_BIT;
>
> if (irq->config == VGIC_CONFIG_EDGE)
> - irq->pending = false;
> + irq_set_pending_latch(irq, false);
>
> if (vgic_irq_is_sgi(irq->intid) &&
> model == KVM_DEV_TYPE_ARM_VGIC_V2) {
> @@ -141,7 +139,7 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
> val |= (src - 1) << GICH_LR_PHYSID_CPUID_SHIFT;
> irq->source &= ~(1 << (src - 1));
> if (irq->source)
> - irq->pending = true;
> + irq_set_pending_latch(irq, true);
> }
> }
>
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index 6440b56..ac978b4 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -160,7 +160,7 @@ static struct kvm_vcpu *vgic_target_oracle(struct vgic_irq *irq)
> * If the distributor is disabled, pending interrupts shouldn't be
> * forwarded.
> */
> - if (irq->enabled && irq->pending) {
> + if (irq->enabled && irq_is_pending(irq)) {
> if (unlikely(irq->target_vcpu &&
> !irq->target_vcpu->kvm->arch.vgic.enabled))
> return NULL;
> @@ -204,8 +204,8 @@ static int vgic_irq_cmp(void *priv, struct list_head *a, struct list_head *b)
> goto out;
> }
>
> - penda = irqa->enabled && irqa->pending;
> - pendb = irqb->enabled && irqb->pending;
> + penda = irqa->enabled && irq_is_pending(irqa);
> + pendb = irqb->enabled && irq_is_pending(irqb);
>
> if (!penda || !pendb) {
> ret = (int)pendb - (int)penda;
> @@ -371,12 +371,10 @@ static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> return 0;
> }
>
> - if (irq->config == VGIC_CONFIG_LEVEL) {
> + if (irq->config == VGIC_CONFIG_LEVEL)
> irq->line_level = level;
> - irq->pending = level || irq->soft_pending;
> - } else {
> - irq->pending = true;
> - }
> + else
> + irq_set_pending_latch(irq, true);
>
> vgic_queue_irq_unlock(kvm, irq);
> vgic_put_irq(kvm, irq);
> @@ -689,7 +687,7 @@ int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
>
> list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) {
> spin_lock(&irq->irq_lock);
> - pending = irq->pending && irq->enabled;
> + pending = irq_is_pending(irq) && irq->enabled;
> spin_unlock(&irq->irq_lock);
>
> if (pending)
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 859f65c..70c7e40 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -30,6 +30,19 @@
>
> #define vgic_irq_is_sgi(intid) ((intid) < VGIC_NR_SGIS)
>
> +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;
> +}
> +
> +static inline void irq_set_pending_latch(struct vgic_irq *irq, bool val)
> +{
> + irq->pending_latch = val;
> +}
> +
> struct vgic_vmcr {
> u32 ctlr;
> u32 abpr;
>
More information about the linux-arm-kernel
mailing list