On Wed, May 24, 2017 at 10:13:21PM +0200, Eric Auger wrote:
> Virtual interrupts directly mapped to physical interrupts require
> some special care. Their pending and active state must be observed
> at distributor level and not in the list register.
This is not entirely true. There's a dependency, but there is also
separate virtual vs. physical state, see below.
>
> Also a level sensitive interrupt's level is not toggled down by any
> maintenance IRQ handler as the EOI is not trapped.
>
> This patch adds an host_irq field in vgic_irq struct to easily
> get the irqchip state of the host irq. We also handle the
> physical IRQ case in vgic_validate_injection and add helpers to
> get the line level and active state.
>
> Signed-off-by: Eric Auger <[email protected]>
> ---
> include/kvm/arm_vgic.h | 4 +++-
> virt/kvm/arm/arch_timer.c | 3 ++-
> virt/kvm/arm/vgic/vgic.c | 44 ++++++++++++++++++++++++++++++++++++++------
> virt/kvm/arm/vgic/vgic.h | 9 ++++++++-
> 4 files changed, 51 insertions(+), 9 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index ef71858..695ebc7 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -112,6 +112,7 @@ struct vgic_irq {
> bool hw; /* Tied to HW IRQ */
> struct kref refcount; /* Used for LPIs */
> u32 hwintid; /* HW INTID number */
> + unsigned int host_irq; /* linux irq corresponding to hwintid */
> union {
> u8 targets; /* GICv2 target VCPUs mask */
> u32 mpidr; /* GICv3 target VCPU */
> @@ -301,7 +302,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> bool level);
> int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> bool level);
> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> + u32 virt_irq, u32 phys_irq);
> int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
> bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 5976609..45f4779 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -651,7 +651,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
> * Tell the VGIC that the virtual interrupt is tied to a
> * physical interrupt. We do that once per VCPU.
> */
> - ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
> + ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq,
> + vtimer->irq.irq, phys_irq);
> if (ret)
> return ret;
>
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index 83b24d2..aa0618c 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -137,6 +137,28 @@ 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_unshared_mapped(irq)))
> + WARN_ON(irq_get_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_PENDING,
> + &line_level));
> + return line_level;
> +}
This really looks fishy. When do we need this exactly?
I feel like we should treat this more like everything else and set the
line_level on the irq even for forwarded interrupts, and then you don't
need changes to validate injection.
The challenge, then, is how to re-sample the line and lower the
line_level field when necessary. Can't we simply do this in
vgic_fold_lr_state(), and if you have a forwarded interrupt which is
level triggered and the level is high, then notify the one who injected
this and tell it to adjust its line level (lower it if it changed).
That would follow our existing path very closely.
Am I missing something?
> +
> +bool irq_is_active(struct vgic_irq *irq)
> +{
> + bool is_active = irq->active;
> +
> + if (unlikely(is_unshared_mapped(irq)))
> + WARN_ON(irq_get_irqchip_state(irq->host_irq,
> + IRQCHIP_STATE_ACTIVE,
> + &is_active));
> + return is_active;
> +}
Why do we need this?
The active state of a virtual IRQ is independent from the underlying
physical state, as I see it.
For example, when the interrupt is initially injected to the VGIC, it
will be ACTIVE on the physical distributor, but PENDING on the VGIC.
Thanks,
-Christoffer
> +
> /**
> * kvm_vgic_target_oracle - compute the target vcpu for an irq
> *
> @@ -153,7 +175,7 @@ static struct kvm_vcpu *vgic_target_oracle(struct vgic_irq *irq)
> DEBUG_SPINLOCK_BUG_ON(!spin_is_locked(&irq->irq_lock));
>
> /* If the interrupt is active, it must stay on the current vcpu */
> - if (irq->active)
> + if (irq_is_active(irq))
> return irq->vcpu ? : irq->target_vcpu;
>
> /*
> @@ -195,14 +217,18 @@ static int vgic_irq_cmp(void *priv, struct list_head *a, struct list_head *b)
> {
> struct vgic_irq *irqa = container_of(a, struct vgic_irq, ap_list);
> struct vgic_irq *irqb = container_of(b, struct vgic_irq, ap_list);
> + bool activea, activeb;
> bool penda, pendb;
> int ret;
>
> spin_lock(&irqa->irq_lock);
> spin_lock_nested(&irqb->irq_lock, SINGLE_DEPTH_NESTING);
>
> - if (irqa->active || irqb->active) {
> - ret = (int)irqb->active - (int)irqa->active;
> + activea = irq_is_active(irqa);
> + activeb = irq_is_active(irqb);
> +
> + if (activea || activeb) {
> + ret = (int)activeb - (int)activea;
> goto out;
> }
>
> @@ -234,13 +260,17 @@ 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.
> */
> static bool vgic_validate_injection(struct vgic_irq *irq, bool level)
> {
> switch (irq->config) {
> case VGIC_CONFIG_LEVEL:
> - return irq->line_level != level;
> + if (unlikely(is_unshared_mapped(irq)))
> + return true;
> + else
> + return irq->line_level != level;
> case VGIC_CONFIG_EDGE:
> return level;
> }
> @@ -392,7 +422,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> return 0;
> }
>
> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> + u32 virt_irq, u32 phys_irq)
> {
> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq);
>
> @@ -402,6 +433,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
>
> irq->hw = true;
> irq->hwintid = phys_irq;
> + irq->host_irq = host_irq;
>
> spin_unlock(&irq->irq_lock);
> vgic_put_irq(vcpu->kvm, irq);
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index da83e4c..dc4972b 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -17,6 +17,7 @@
> #define __KVM_ARM_VGIC_NEW_H__
>
> #include <linux/irqchip/arm-gic-common.h>
> +#include <linux/interrupt.h>
>
> #define PRODUCT_ID_KVM 0x4b /* ASCII code K */
> #define IMPLEMENTER_ARM 0x43b
> @@ -96,14 +97,20 @@
> /* 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);
> +bool irq_is_active(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_unshared_mapped(i) \
> +((i)->hw && (i)->intid >= VGIC_NR_PRIVATE_IRQS && (i)->intid < 1020)
> +
> /*
> * 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
>
On 02/06/17 14:33, Christoffer Dall wrote:
> On Wed, May 24, 2017 at 10:13:21PM +0200, Eric Auger wrote:
>> Virtual interrupts directly mapped to physical interrupts require
>> some special care. Their pending and active state must be observed
>> at distributor level and not in the list register.
>
> This is not entirely true. There's a dependency, but there is also
> separate virtual vs. physical state, see below.
I think this stems for the usual confusion about the "pending and active
state" vs "pending and active states". Yes, the GIC spec is rubbish. Can
I state this again?
>>
>> Also a level sensitive interrupt's level is not toggled down by any
>> maintenance IRQ handler as the EOI is not trapped.
>>
>> This patch adds an host_irq field in vgic_irq struct to easily
>> get the irqchip state of the host irq. We also handle the
>> physical IRQ case in vgic_validate_injection and add helpers to
>> get the line level and active state.
>>
>> Signed-off-by: Eric Auger <[email protected]>
>> ---
>> include/kvm/arm_vgic.h | 4 +++-
>> virt/kvm/arm/arch_timer.c | 3 ++-
>> virt/kvm/arm/vgic/vgic.c | 44 ++++++++++++++++++++++++++++++++++++++------
>> virt/kvm/arm/vgic/vgic.h | 9 ++++++++-
>> 4 files changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index ef71858..695ebc7 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -112,6 +112,7 @@ struct vgic_irq {
>> bool hw; /* Tied to HW IRQ */
>> struct kref refcount; /* Used for LPIs */
>> u32 hwintid; /* HW INTID number */
>> + unsigned int host_irq; /* linux irq corresponding to hwintid */
>> union {
>> u8 targets; /* GICv2 target VCPUs mask */
>> u32 mpidr; /* GICv3 target VCPU */
>> @@ -301,7 +302,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>> bool level);
>> int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>> bool level);
>> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
>> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
>> + u32 virt_irq, u32 phys_irq);
>> int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>> bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>>
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index 5976609..45f4779 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -651,7 +651,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>> * Tell the VGIC that the virtual interrupt is tied to a
>> * physical interrupt. We do that once per VCPU.
>> */
>> - ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
>> + ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq,
>> + vtimer->irq.irq, phys_irq);
>> if (ret)
>> return ret;
>>
>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>> index 83b24d2..aa0618c 100644
>> --- a/virt/kvm/arm/vgic/vgic.c
>> +++ b/virt/kvm/arm/vgic/vgic.c
>> @@ -137,6 +137,28 @@ 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_unshared_mapped(irq)))
>> + WARN_ON(irq_get_irqchip_state(irq->host_irq,
>> + IRQCHIP_STATE_PENDING,
>> + &line_level));
>> + return line_level;
>> +}
>
> This really looks fishy. When do we need this exactly?
>
> I feel like we should treat this more like everything else and set the
> line_level on the irq even for forwarded interrupts, and then you don't
> need changes to validate injection.
>
> The challenge, then, is how to re-sample the line and lower the
> line_level field when necessary. Can't we simply do this in
> vgic_fold_lr_state(), and if you have a forwarded interrupt which is
> level triggered and the level is high, then notify the one who injected
> this and tell it to adjust its line level (lower it if it changed).
>
> That would follow our existing path very closely.
>
> Am I missing something?
I don't think you are. I think Eric got confused because of the above.
But the flow is a bit a brainfsck :-(
- Physical interrupt fires, activated, injected in the vgic
- Injecting the interrupt has a very different flow from what we
currently have, and follow the same pattern as an Edge interrupt
(because the Pending state is kept at the physical distributor, so we
cannot preserve it in the emulation).
- Normal life cycle of the interrupt
- The fact that the Pending bit is kept at the distributor level ensures
that if it becomes pending again in the emulation, that's because the
guest has deactivated the physical interrupt by doing an EOI.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On Fri, Jun 02, 2017 at 03:10:23PM +0100, Marc Zyngier wrote:
> On 02/06/17 14:33, Christoffer Dall wrote:
> > On Wed, May 24, 2017 at 10:13:21PM +0200, Eric Auger wrote:
> >> Virtual interrupts directly mapped to physical interrupts require
> >> some special care. Their pending and active state must be observed
> >> at distributor level and not in the list register.
> >
> > This is not entirely true. There's a dependency, but there is also
> > separate virtual vs. physical state, see below.
>
> I think this stems for the usual confusion about the "pending and active
> state" vs "pending and active states". Yes, the GIC spec is rubbish. Can
> I state this again?
>
> >>
> >> Also a level sensitive interrupt's level is not toggled down by any
> >> maintenance IRQ handler as the EOI is not trapped.
> >>
> >> This patch adds an host_irq field in vgic_irq struct to easily
> >> get the irqchip state of the host irq. We also handle the
> >> physical IRQ case in vgic_validate_injection and add helpers to
> >> get the line level and active state.
> >>
> >> Signed-off-by: Eric Auger <[email protected]>
> >> ---
> >> include/kvm/arm_vgic.h | 4 +++-
> >> virt/kvm/arm/arch_timer.c | 3 ++-
> >> virt/kvm/arm/vgic/vgic.c | 44 ++++++++++++++++++++++++++++++++++++++------
> >> virt/kvm/arm/vgic/vgic.h | 9 ++++++++-
> >> 4 files changed, 51 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> >> index ef71858..695ebc7 100644
> >> --- a/include/kvm/arm_vgic.h
> >> +++ b/include/kvm/arm_vgic.h
> >> @@ -112,6 +112,7 @@ struct vgic_irq {
> >> bool hw; /* Tied to HW IRQ */
> >> struct kref refcount; /* Used for LPIs */
> >> u32 hwintid; /* HW INTID number */
> >> + unsigned int host_irq; /* linux irq corresponding to hwintid */
> >> union {
> >> u8 targets; /* GICv2 target VCPUs mask */
> >> u32 mpidr; /* GICv3 target VCPU */
> >> @@ -301,7 +302,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >> bool level);
> >> int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >> bool level);
> >> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
> >> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> >> + u32 virt_irq, u32 phys_irq);
> >> int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
> >> bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
> >>
> >> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >> index 5976609..45f4779 100644
> >> --- a/virt/kvm/arm/arch_timer.c
> >> +++ b/virt/kvm/arm/arch_timer.c
> >> @@ -651,7 +651,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
> >> * Tell the VGIC that the virtual interrupt is tied to a
> >> * physical interrupt. We do that once per VCPU.
> >> */
> >> - ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
> >> + ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq,
> >> + vtimer->irq.irq, phys_irq);
> >> if (ret)
> >> return ret;
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> >> index 83b24d2..aa0618c 100644
> >> --- a/virt/kvm/arm/vgic/vgic.c
> >> +++ b/virt/kvm/arm/vgic/vgic.c
> >> @@ -137,6 +137,28 @@ 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_unshared_mapped(irq)))
> >> + WARN_ON(irq_get_irqchip_state(irq->host_irq,
> >> + IRQCHIP_STATE_PENDING,
> >> + &line_level));
> >> + return line_level;
> >> +}
> >
> > This really looks fishy. When do we need this exactly?
> >
> > I feel like we should treat this more like everything else and set the
> > line_level on the irq even for forwarded interrupts, and then you don't
> > need changes to validate injection.
> >
> > The challenge, then, is how to re-sample the line and lower the
> > line_level field when necessary. Can't we simply do this in
> > vgic_fold_lr_state(), and if you have a forwarded interrupt which is
> > level triggered and the level is high, then notify the one who injected
> > this and tell it to adjust its line level (lower it if it changed).
> >
> > That would follow our existing path very closely.
> >
> > Am I missing something?
>
> I don't think you are. I think Eric got confused because of the above.
> But the flow is a bit a brainfsck :-(
>
> - Physical interrupt fires, activated, injected in the vgic
> - Injecting the interrupt has a very different flow from what we
> currently have, and follow the same pattern as an Edge interrupt
> (because the Pending state is kept at the physical distributor, so we
> cannot preserve it in the emulation).
> - Normal life cycle of the interrupt
> - The fact that the Pending bit is kept at the distributor level ensures
> that if it becomes pending again in the emulation, that's because the
> guest has deactivated the physical interrupt by doing an EOI.
>
I think there's a choice between how we choose to support this. We can
either do the edge-like injection, or we can model the line_level to the
best of our ability (we just have to lower the line after the guest
exits after deactivation if it's not still pending at the physical
distributor).
One question with doing this edge-like, can you ahve this scenario:
1. VM runs with active virtual interrupt linked to physical
interrupt.
2. VM deactivates virtual+physical interrupt
3. Physical interrupt fires again on the host
4. The host injects the virtual interrupt as pending to the VGIC (and
IPIs the VCPU etc.)
5. The device lowers the physical line (another VPCU programs the
device, there's some delay, or whatever)
6. The VCPU now sees a pending interrupt, which is no longer pending.
Not sure if the line-like approach really solves this, though, or if
getting a spurius interrupt is something we care about.
Perhaps we need to try to implement both and see how it looks like?
Thanks,
-Christoffer
On Fri, Jun 02 2017 at 6:29:44 pm BST, Christoffer Dall <[email protected]> wrote:
> On Fri, Jun 02, 2017 at 03:10:23PM +0100, Marc Zyngier wrote:
>> On 02/06/17 14:33, Christoffer Dall wrote:
>> > On Wed, May 24, 2017 at 10:13:21PM +0200, Eric Auger wrote:
>> >> Virtual interrupts directly mapped to physical interrupts require
>> >> some special care. Their pending and active state must be observed
>> >> at distributor level and not in the list register.
>> >
>> > This is not entirely true. There's a dependency, but there is also
>> > separate virtual vs. physical state, see below.
>>
>> I think this stems for the usual confusion about the "pending and active
>> state" vs "pending and active states". Yes, the GIC spec is rubbish. Can
>> I state this again?
>>
>> >>
>> >> Also a level sensitive interrupt's level is not toggled down by any
>> >> maintenance IRQ handler as the EOI is not trapped.
>> >>
>> >> This patch adds an host_irq field in vgic_irq struct to easily
>> >> get the irqchip state of the host irq. We also handle the
>> >> physical IRQ case in vgic_validate_injection and add helpers to
>> >> get the line level and active state.
>> >>
>> >> Signed-off-by: Eric Auger <[email protected]>
>> >> ---
>> >> include/kvm/arm_vgic.h | 4 +++-
>> >> virt/kvm/arm/arch_timer.c | 3 ++-
>> >> virt/kvm/arm/vgic/vgic.c | 44 ++++++++++++++++++++++++++++++++++++++------
>> >> virt/kvm/arm/vgic/vgic.h | 9 ++++++++-
>> >> 4 files changed, 51 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> >> index ef71858..695ebc7 100644
>> >> --- a/include/kvm/arm_vgic.h
>> >> +++ b/include/kvm/arm_vgic.h
>> >> @@ -112,6 +112,7 @@ struct vgic_irq {
>> >> bool hw; /* Tied to HW IRQ */
>> >> struct kref refcount; /* Used for LPIs */
>> >> u32 hwintid; /* HW INTID number */
>> >> + unsigned int host_irq; /* linux irq corresponding to hwintid */
>> >> union {
>> >> u8 targets; /* GICv2 target VCPUs mask */
>> >> u32 mpidr; /* GICv3 target VCPU */
>> >> @@ -301,7 +302,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>> >> bool level);
>> >> int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>> >> bool level);
>> >> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
>> >> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
>> >> + u32 virt_irq, u32 phys_irq);
>> >> int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>> >> bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>> >>
>> >> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> >> index 5976609..45f4779 100644
>> >> --- a/virt/kvm/arm/arch_timer.c
>> >> +++ b/virt/kvm/arm/arch_timer.c
>> >> @@ -651,7 +651,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>> >> * Tell the VGIC that the virtual interrupt is tied to a
>> >> * physical interrupt. We do that once per VCPU.
>> >> */
>> >> - ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
>> >> + ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq,
>> >> + vtimer->irq.irq, phys_irq);
>> >> if (ret)
>> >> return ret;
>> >>
>> >> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>> >> index 83b24d2..aa0618c 100644
>> >> --- a/virt/kvm/arm/vgic/vgic.c
>> >> +++ b/virt/kvm/arm/vgic/vgic.c
>> >> @@ -137,6 +137,28 @@ 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_unshared_mapped(irq)))
>> >> + WARN_ON(irq_get_irqchip_state(irq->host_irq,
>> >> + IRQCHIP_STATE_PENDING,
>> >> + &line_level));
>> >> + return line_level;
>> >> +}
>> >
>> > This really looks fishy. When do we need this exactly?
>> >
>> > I feel like we should treat this more like everything else and set the
>> > line_level on the irq even for forwarded interrupts, and then you don't
>> > need changes to validate injection.
>> >
>> > The challenge, then, is how to re-sample the line and lower the
>> > line_level field when necessary. Can't we simply do this in
>> > vgic_fold_lr_state(), and if you have a forwarded interrupt which is
>> > level triggered and the level is high, then notify the one who injected
>> > this and tell it to adjust its line level (lower it if it changed).
>> >
>> > That would follow our existing path very closely.
>> >
>> > Am I missing something?
>>
>> I don't think you are. I think Eric got confused because of the above.
>> But the flow is a bit a brainfsck :-(
>>
>> - Physical interrupt fires, activated, injected in the vgic
>> - Injecting the interrupt has a very different flow from what we
>> currently have, and follow the same pattern as an Edge interrupt
>> (because the Pending state is kept at the physical distributor, so we
>> cannot preserve it in the emulation).
>> - Normal life cycle of the interrupt
>> - The fact that the Pending bit is kept at the distributor level ensures
>> that if it becomes pending again in the emulation, that's because the
>> guest has deactivated the physical interrupt by doing an EOI.
>>
>
> I think there's a choice between how we choose to support this. We can
> either do the edge-like injection, or we can model the line_level to the
> best of our ability (we just have to lower the line after the guest
> exits after deactivation if it's not still pending at the physical
> distributor).
>
> One question with doing this edge-like, can you ahve this scenario:
> 1. VM runs with active virtual interrupt linked to physical
> interrupt.
> 2. VM deactivates virtual+physical interrupt
> 3. Physical interrupt fires again on the host
> 4. The host injects the virtual interrupt as pending to the VGIC (and
> IPIs the VCPU etc.)
> 5. The device lowers the physical line (another VPCU programs the
> device, there's some delay, or whatever)
> 6. The VCPU now sees a pending interrupt, which is no longer pending.
>
> Not sure if the line-like approach really solves this, though, or if
> getting a spurius interrupt is something we care about.
That would be a spurious interrupt indeed, but I'm not sure that's
something the line level sampling you suggest would avoid either. There
is a fundamental disconnect between the injection and the physical line,
and it can only be modelled to some level of accuracy (/me curse the
architecture again).
> Perhaps we need to try to implement both and see how it looks like?
There is definitely room for experiment, but I feel Eric should focus on
one of them (whichever it is). Happy to help prototyping the other one
though.
Thanks,
M.
--
Jazz is not dead, it just smell funny.
On Thu, Jun 08, 2017 at 09:23:16AM +0100, Marc Zyngier wrote:
> On Fri, Jun 02 2017 at 6:29:44 pm BST, Christoffer Dall <[email protected]> wrote:
> > On Fri, Jun 02, 2017 at 03:10:23PM +0100, Marc Zyngier wrote:
> >> On 02/06/17 14:33, Christoffer Dall wrote:
> >> > On Wed, May 24, 2017 at 10:13:21PM +0200, Eric Auger wrote:
> >> >> Virtual interrupts directly mapped to physical interrupts require
> >> >> some special care. Their pending and active state must be observed
> >> >> at distributor level and not in the list register.
> >> >
> >> > This is not entirely true. There's a dependency, but there is also
> >> > separate virtual vs. physical state, see below.
> >>
> >> I think this stems for the usual confusion about the "pending and active
> >> state" vs "pending and active states". Yes, the GIC spec is rubbish. Can
> >> I state this again?
> >>
> >> >>
> >> >> Also a level sensitive interrupt's level is not toggled down by any
> >> >> maintenance IRQ handler as the EOI is not trapped.
> >> >>
> >> >> This patch adds an host_irq field in vgic_irq struct to easily
> >> >> get the irqchip state of the host irq. We also handle the
> >> >> physical IRQ case in vgic_validate_injection and add helpers to
> >> >> get the line level and active state.
> >> >>
> >> >> Signed-off-by: Eric Auger <[email protected]>
> >> >> ---
> >> >> include/kvm/arm_vgic.h | 4 +++-
> >> >> virt/kvm/arm/arch_timer.c | 3 ++-
> >> >> virt/kvm/arm/vgic/vgic.c | 44 ++++++++++++++++++++++++++++++++++++++------
> >> >> virt/kvm/arm/vgic/vgic.h | 9 ++++++++-
> >> >> 4 files changed, 51 insertions(+), 9 deletions(-)
> >> >>
> >> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> >> >> index ef71858..695ebc7 100644
> >> >> --- a/include/kvm/arm_vgic.h
> >> >> +++ b/include/kvm/arm_vgic.h
> >> >> @@ -112,6 +112,7 @@ struct vgic_irq {
> >> >> bool hw; /* Tied to HW IRQ */
> >> >> struct kref refcount; /* Used for LPIs */
> >> >> u32 hwintid; /* HW INTID number */
> >> >> + unsigned int host_irq; /* linux irq corresponding to hwintid */
> >> >> union {
> >> >> u8 targets; /* GICv2 target VCPUs mask */
> >> >> u32 mpidr; /* GICv3 target VCPU */
> >> >> @@ -301,7 +302,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >> >> bool level);
> >> >> int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >> >> bool level);
> >> >> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
> >> >> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> >> >> + u32 virt_irq, u32 phys_irq);
> >> >> int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
> >> >> bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
> >> >>
> >> >> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >> >> index 5976609..45f4779 100644
> >> >> --- a/virt/kvm/arm/arch_timer.c
> >> >> +++ b/virt/kvm/arm/arch_timer.c
> >> >> @@ -651,7 +651,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
> >> >> * Tell the VGIC that the virtual interrupt is tied to a
> >> >> * physical interrupt. We do that once per VCPU.
> >> >> */
> >> >> - ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
> >> >> + ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq,
> >> >> + vtimer->irq.irq, phys_irq);
> >> >> if (ret)
> >> >> return ret;
> >> >>
> >> >> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> >> >> index 83b24d2..aa0618c 100644
> >> >> --- a/virt/kvm/arm/vgic/vgic.c
> >> >> +++ b/virt/kvm/arm/vgic/vgic.c
> >> >> @@ -137,6 +137,28 @@ 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_unshared_mapped(irq)))
> >> >> + WARN_ON(irq_get_irqchip_state(irq->host_irq,
> >> >> + IRQCHIP_STATE_PENDING,
> >> >> + &line_level));
> >> >> + return line_level;
> >> >> +}
> >> >
> >> > This really looks fishy. When do we need this exactly?
> >> >
> >> > I feel like we should treat this more like everything else and set the
> >> > line_level on the irq even for forwarded interrupts, and then you don't
> >> > need changes to validate injection.
> >> >
> >> > The challenge, then, is how to re-sample the line and lower the
> >> > line_level field when necessary. Can't we simply do this in
> >> > vgic_fold_lr_state(), and if you have a forwarded interrupt which is
> >> > level triggered and the level is high, then notify the one who injected
> >> > this and tell it to adjust its line level (lower it if it changed).
> >> >
> >> > That would follow our existing path very closely.
> >> >
> >> > Am I missing something?
> >>
> >> I don't think you are. I think Eric got confused because of the above.
> >> But the flow is a bit a brainfsck :-(
> >>
> >> - Physical interrupt fires, activated, injected in the vgic
> >> - Injecting the interrupt has a very different flow from what we
> >> currently have, and follow the same pattern as an Edge interrupt
> >> (because the Pending state is kept at the physical distributor, so we
> >> cannot preserve it in the emulation).
> >> - Normal life cycle of the interrupt
> >> - The fact that the Pending bit is kept at the distributor level ensures
> >> that if it becomes pending again in the emulation, that's because the
> >> guest has deactivated the physical interrupt by doing an EOI.
> >>
> >
> > I think there's a choice between how we choose to support this. We can
> > either do the edge-like injection, or we can model the line_level to the
> > best of our ability (we just have to lower the line after the guest
> > exits after deactivation if it's not still pending at the physical
> > distributor).
> >
> > One question with doing this edge-like, can you ahve this scenario:
> > 1. VM runs with active virtual interrupt linked to physical
> > interrupt.
> > 2. VM deactivates virtual+physical interrupt
> > 3. Physical interrupt fires again on the host
> > 4. The host injects the virtual interrupt as pending to the VGIC (and
> > IPIs the VCPU etc.)
> > 5. The device lowers the physical line (another VPCU programs the
> > device, there's some delay, or whatever)
> > 6. The VCPU now sees a pending interrupt, which is no longer pending.
> >
> > Not sure if the line-like approach really solves this, though, or if
> > getting a spurius interrupt is something we care about.
>
> That would be a spurious interrupt indeed, but I'm not sure that's
> something the line level sampling you suggest would avoid either. There
> is a fundamental disconnect between the injection and the physical line,
> and it can only be modelled to some level of accuracy (/me curse the
> architecture again).
>
> > Perhaps we need to try to implement both and see how it looks like?
>
> There is definitely room for experiment, but I feel Eric should focus on
> one of them (whichever it is). Happy to help prototyping the other one
> though.
>
That's fair. I'm just worried about the whole "emulate level triggered
interrupts as edge triggered" thing, but as you said, the architecture
doesn't allow us to model it more accurately.
Thanks,
-Christoffer
Hi Christoffer,
On 02/06/2017 15:33, Christoffer Dall wrote:
> On Wed, May 24, 2017 at 10:13:21PM +0200, Eric Auger wrote:
>> Virtual interrupts directly mapped to physical interrupts require
>> some special care. Their pending and active state must be observed
>> at distributor level and not in the list register.
>
> This is not entirely true. There's a dependency, but there is also
> separate virtual vs. physical state, see below.
>
>>
>> Also a level sensitive interrupt's level is not toggled down by any
>> maintenance IRQ handler as the EOI is not trapped.
>>
>> This patch adds an host_irq field in vgic_irq struct to easily
>> get the irqchip state of the host irq. We also handle the
>> physical IRQ case in vgic_validate_injection and add helpers to
>> get the line level and active state.
>>
>> Signed-off-by: Eric Auger <[email protected]>
>> ---
>> include/kvm/arm_vgic.h | 4 +++-
>> virt/kvm/arm/arch_timer.c | 3 ++-
>> virt/kvm/arm/vgic/vgic.c | 44 ++++++++++++++++++++++++++++++++++++++------
>> virt/kvm/arm/vgic/vgic.h | 9 ++++++++-
>> 4 files changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index ef71858..695ebc7 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -112,6 +112,7 @@ struct vgic_irq {
>> bool hw; /* Tied to HW IRQ */
>> struct kref refcount; /* Used for LPIs */
>> u32 hwintid; /* HW INTID number */
>> + unsigned int host_irq; /* linux irq corresponding to hwintid */
>> union {
>> u8 targets; /* GICv2 target VCPUs mask */
>> u32 mpidr; /* GICv3 target VCPU */
>> @@ -301,7 +302,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>> bool level);
>> int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>> bool level);
>> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
>> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
>> + u32 virt_irq, u32 phys_irq);
>> int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>> bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>>
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index 5976609..45f4779 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -651,7 +651,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>> * Tell the VGIC that the virtual interrupt is tied to a
>> * physical interrupt. We do that once per VCPU.
>> */
>> - ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
>> + ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq,
>> + vtimer->irq.irq, phys_irq);
>> if (ret)
>> return ret;
>>
>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>> index 83b24d2..aa0618c 100644
>> --- a/virt/kvm/arm/vgic/vgic.c
>> +++ b/virt/kvm/arm/vgic/vgic.c
>> @@ -137,6 +137,28 @@ 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_unshared_mapped(irq)))
>> + WARN_ON(irq_get_irqchip_state(irq->host_irq,
>> + IRQCHIP_STATE_PENDING,
>> + &line_level));
>> + return line_level;
>> +}
>
> This really looks fishy. When do we need this exactly?
>
> I feel like we should treat this more like everything else and set the
> line_level on the irq even for forwarded interrupts, and then you don't
> need changes to validate injection.
>
> The challenge, then, is how to re-sample the line and lower the
> line_level field when necessary. Can't we simply do this in
> vgic_fold_lr_state(), and if you have a forwarded interrupt which is
> level triggered and the level is high,
Didn't you mean level is low? When the level is low you would trigger
the resamplefd? Doesn't it bring a useless overhead?
So yes I was confused by the spec and I thought the pending and active
bit*s* were not reliable in the LRs and instead stared at the
distributor state.
That being said, as you mention, the line level is difficult to model as
the resamplefd is not called so why not directly looking at the physical
distributor state (pending bit?). The impact on the SM is reduced to
irq_line_level() and validate_injection() which is always true for
forwarded interrupts.
then notify the one who injected
> this and tell it to adjust its line level (lower it if it changed).
>
> That would follow our existing path very closely.
>
> Am I missing something?
>
>> +
>> +bool irq_is_active(struct vgic_irq *irq)
>> +{
>> + bool is_active = irq->active;
>> +
>> + if (unlikely(is_unshared_mapped(irq)))
>> + WARN_ON(irq_get_irqchip_state(irq->host_irq,
>> + IRQCHIP_STATE_ACTIVE,
>> + &is_active));
>> + return is_active;
>> +}
>
> Why do we need this?
>
> The active state of a virtual IRQ is independent from the underlying
> physical state, as I see it.
>
> For example, when the interrupt is initially injected to the VGIC, it
> will be ACTIVE on the physical distributor, but PENDING on the VGIC.
as I can use the active bit of the LR, that code is not needed then.
Thanks
Eric
>
>
> Thanks,
> -Christoffer
>
>> +
>> /**
>> * kvm_vgic_target_oracle - compute the target vcpu for an irq
>> *
>> @@ -153,7 +175,7 @@ static struct kvm_vcpu *vgic_target_oracle(struct vgic_irq *irq)
>> DEBUG_SPINLOCK_BUG_ON(!spin_is_locked(&irq->irq_lock));
>>
>> /* If the interrupt is active, it must stay on the current vcpu */
>> - if (irq->active)
>> + if (irq_is_active(irq))
>> return irq->vcpu ? : irq->target_vcpu;
>>
>> /*
>> @@ -195,14 +217,18 @@ static int vgic_irq_cmp(void *priv, struct list_head *a, struct list_head *b)
>> {
>> struct vgic_irq *irqa = container_of(a, struct vgic_irq, ap_list);
>> struct vgic_irq *irqb = container_of(b, struct vgic_irq, ap_list);
>> + bool activea, activeb;
>> bool penda, pendb;
>> int ret;
>>
>> spin_lock(&irqa->irq_lock);
>> spin_lock_nested(&irqb->irq_lock, SINGLE_DEPTH_NESTING);
>>
>> - if (irqa->active || irqb->active) {
>> - ret = (int)irqb->active - (int)irqa->active;
>> + activea = irq_is_active(irqa);
>> + activeb = irq_is_active(irqb);
>> +
>> + if (activea || activeb) {
>> + ret = (int)activeb - (int)activea;
>> goto out;
>> }
>>
>> @@ -234,13 +260,17 @@ 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.
>> */
>> static bool vgic_validate_injection(struct vgic_irq *irq, bool level)
>> {
>> switch (irq->config) {
>> case VGIC_CONFIG_LEVEL:
>> - return irq->line_level != level;
>> + if (unlikely(is_unshared_mapped(irq)))
>> + return true;
>> + else
>> + return irq->line_level != level;
>> case VGIC_CONFIG_EDGE:
>> return level;
>> }
>> @@ -392,7 +422,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>> return 0;
>> }
>>
>> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
>> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
>> + u32 virt_irq, u32 phys_irq)
>> {
>> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq);
>>
>> @@ -402,6 +433,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
>>
>> irq->hw = true;
>> irq->hwintid = phys_irq;
>> + irq->host_irq = host_irq;
>>
>> spin_unlock(&irq->irq_lock);
>> vgic_put_irq(vcpu->kvm, irq);
>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>> index da83e4c..dc4972b 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -17,6 +17,7 @@
>> #define __KVM_ARM_VGIC_NEW_H__
>>
>> #include <linux/irqchip/arm-gic-common.h>
>> +#include <linux/interrupt.h>
>>
>> #define PRODUCT_ID_KVM 0x4b /* ASCII code K */
>> #define IMPLEMENTER_ARM 0x43b
>> @@ -96,14 +97,20 @@
>> /* 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);
>> +bool irq_is_active(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_unshared_mapped(i) \
>> +((i)->hw && (i)->intid >= VGIC_NR_PRIVATE_IRQS && (i)->intid < 1020)
>> +
>> /*
>> * 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
>>
Hi Christoffer, Marc,
On 08/06/2017 10:34, Christoffer Dall wrote:
> On Thu, Jun 08, 2017 at 09:23:16AM +0100, Marc Zyngier wrote:
>> On Fri, Jun 02 2017 at 6:29:44 pm BST, Christoffer Dall <[email protected]> wrote:
>>> On Fri, Jun 02, 2017 at 03:10:23PM +0100, Marc Zyngier wrote:
>>>> On 02/06/17 14:33, Christoffer Dall wrote:
>>>>> On Wed, May 24, 2017 at 10:13:21PM +0200, Eric Auger wrote:
>>>>>> Virtual interrupts directly mapped to physical interrupts require
>>>>>> some special care. Their pending and active state must be observed
>>>>>> at distributor level and not in the list register.
>>>>>
>>>>> This is not entirely true. There's a dependency, but there is also
>>>>> separate virtual vs. physical state, see below.
>>>>
>>>> I think this stems for the usual confusion about the "pending and active
>>>> state" vs "pending and active states". Yes, the GIC spec is rubbish. Can
>>>> I state this again?
>>>>
>>>>>>
>>>>>> Also a level sensitive interrupt's level is not toggled down by any
>>>>>> maintenance IRQ handler as the EOI is not trapped.
>>>>>>
>>>>>> This patch adds an host_irq field in vgic_irq struct to easily
>>>>>> get the irqchip state of the host irq. We also handle the
>>>>>> physical IRQ case in vgic_validate_injection and add helpers to
>>>>>> get the line level and active state.
>>>>>>
>>>>>> Signed-off-by: Eric Auger <[email protected]>
>>>>>> ---
>>>>>> include/kvm/arm_vgic.h | 4 +++-
>>>>>> virt/kvm/arm/arch_timer.c | 3 ++-
>>>>>> virt/kvm/arm/vgic/vgic.c | 44 ++++++++++++++++++++++++++++++++++++++------
>>>>>> virt/kvm/arm/vgic/vgic.h | 9 ++++++++-
>>>>>> 4 files changed, 51 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>>>>> index ef71858..695ebc7 100644
>>>>>> --- a/include/kvm/arm_vgic.h
>>>>>> +++ b/include/kvm/arm_vgic.h
>>>>>> @@ -112,6 +112,7 @@ struct vgic_irq {
>>>>>> bool hw; /* Tied to HW IRQ */
>>>>>> struct kref refcount; /* Used for LPIs */
>>>>>> u32 hwintid; /* HW INTID number */
>>>>>> + unsigned int host_irq; /* linux irq corresponding to hwintid */
>>>>>> union {
>>>>>> u8 targets; /* GICv2 target VCPUs mask */
>>>>>> u32 mpidr; /* GICv3 target VCPU */
>>>>>> @@ -301,7 +302,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>>>>>> bool level);
>>>>>> int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>>>>>> bool level);
>>>>>> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
>>>>>> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
>>>>>> + u32 virt_irq, u32 phys_irq);
>>>>>> int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>>>>>> bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>>>>>>
>>>>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>>>>>> index 5976609..45f4779 100644
>>>>>> --- a/virt/kvm/arm/arch_timer.c
>>>>>> +++ b/virt/kvm/arm/arch_timer.c
>>>>>> @@ -651,7 +651,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>>>>>> * Tell the VGIC that the virtual interrupt is tied to a
>>>>>> * physical interrupt. We do that once per VCPU.
>>>>>> */
>>>>>> - ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
>>>>>> + ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq,
>>>>>> + vtimer->irq.irq, phys_irq);
>>>>>> if (ret)
>>>>>> return ret;
>>>>>>
>>>>>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>>>>>> index 83b24d2..aa0618c 100644
>>>>>> --- a/virt/kvm/arm/vgic/vgic.c
>>>>>> +++ b/virt/kvm/arm/vgic/vgic.c
>>>>>> @@ -137,6 +137,28 @@ 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_unshared_mapped(irq)))
>>>>>> + WARN_ON(irq_get_irqchip_state(irq->host_irq,
>>>>>> + IRQCHIP_STATE_PENDING,
>>>>>> + &line_level));
>>>>>> + return line_level;
>>>>>> +}
>>>>>
>>>>> This really looks fishy. When do we need this exactly?
>>>>>
>>>>> I feel like we should treat this more like everything else and set the
>>>>> line_level on the irq even for forwarded interrupts, and then you don't
>>>>> need changes to validate injection.
>>>>>
>>>>> The challenge, then, is how to re-sample the line and lower the
>>>>> line_level field when necessary. Can't we simply do this in
>>>>> vgic_fold_lr_state(), and if you have a forwarded interrupt which is
>>>>> level triggered and the level is high, then notify the one who injected
>>>>> this and tell it to adjust its line level (lower it if it changed).
>>>>>
>>>>> That would follow our existing path very closely.
>>>>>
>>>>> Am I missing something?
>>>>
>>>> I don't think you are. I think Eric got confused because of the above.
>>>> But the flow is a bit a brainfsck :-(
>>>>
>>>> - Physical interrupt fires, activated, injected in the vgic
>>>> - Injecting the interrupt has a very different flow from what we
>>>> currently have, and follow the same pattern as an Edge interrupt
>>>> (because the Pending state is kept at the physical distributor, so we
>>>> cannot preserve it in the emulation).
>>>> - Normal life cycle of the interrupt
>>>> - The fact that the Pending bit is kept at the distributor level ensures
>>>> that if it becomes pending again in the emulation, that's because the
>>>> guest has deactivated the physical interrupt by doing an EOI.
>>>>
>>>
>>> I think there's a choice between how we choose to support this. We can
>>> either do the edge-like injection, or we can model the line_level to the
>>> best of our ability (we just have to lower the line after the guest
>>> exits after deactivation if it's not still pending at the physical
>>> distributor).
>>>
>>> One question with doing this edge-like, can you ahve this scenario:
>>> 1. VM runs with active virtual interrupt linked to physical
>>> interrupt.
>>> 2. VM deactivates virtual+physical interrupt
>>> 3. Physical interrupt fires again on the host
>>> 4. The host injects the virtual interrupt as pending to the VGIC (and
>>> IPIs the VCPU etc.)
>>> 5. The device lowers the physical line (another VPCU programs the
>>> device, there's some delay, or whatever)
>>> 6. The VCPU now sees a pending interrupt, which is no longer pending.
>>>
>>> Not sure if the line-like approach really solves this, though, or if
>>> getting a spurius interrupt is something we care about.
>>
>> That would be a spurious interrupt indeed, but I'm not sure that's
>> something the line level sampling you suggest would avoid either. There
>> is a fundamental disconnect between the injection and the physical line,
>> and it can only be modelled to some level of accuracy (/me curse the
>> architecture again).
>>
>>> Perhaps we need to try to implement both and see how it looks like?
>>
>> There is definitely room for experiment, but I feel Eric should focus on
>> one of them (whichever it is). Happy to help prototyping the other one
>> though.
>>
> That's fair. I'm just worried about the whole "emulate level triggered
> interrupts as edge triggered" thing, but as you said, the architecture
> doesn't allow us to model it more accurately.
if the line level is modeled using the physical distributor pending
state, don't you fix that case?
Thanks
Eric
>
> Thanks,
> -Christoffer
>
On Thu, Jun 08, 2017 at 10:49:12AM +0200, Auger Eric wrote:
> Hi Christoffer,
>
> On 02/06/2017 15:33, Christoffer Dall wrote:
> > On Wed, May 24, 2017 at 10:13:21PM +0200, Eric Auger wrote:
> >> Virtual interrupts directly mapped to physical interrupts require
> >> some special care. Their pending and active state must be observed
> >> at distributor level and not in the list register.
> >
> > This is not entirely true. There's a dependency, but there is also
> > separate virtual vs. physical state, see below.
> >
> >>
> >> Also a level sensitive interrupt's level is not toggled down by any
> >> maintenance IRQ handler as the EOI is not trapped.
> >>
> >> This patch adds an host_irq field in vgic_irq struct to easily
> >> get the irqchip state of the host irq. We also handle the
> >> physical IRQ case in vgic_validate_injection and add helpers to
> >> get the line level and active state.
> >>
> >> Signed-off-by: Eric Auger <[email protected]>
> >> ---
> >> include/kvm/arm_vgic.h | 4 +++-
> >> virt/kvm/arm/arch_timer.c | 3 ++-
> >> virt/kvm/arm/vgic/vgic.c | 44 ++++++++++++++++++++++++++++++++++++++------
> >> virt/kvm/arm/vgic/vgic.h | 9 ++++++++-
> >> 4 files changed, 51 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> >> index ef71858..695ebc7 100644
> >> --- a/include/kvm/arm_vgic.h
> >> +++ b/include/kvm/arm_vgic.h
> >> @@ -112,6 +112,7 @@ struct vgic_irq {
> >> bool hw; /* Tied to HW IRQ */
> >> struct kref refcount; /* Used for LPIs */
> >> u32 hwintid; /* HW INTID number */
> >> + unsigned int host_irq; /* linux irq corresponding to hwintid */
> >> union {
> >> u8 targets; /* GICv2 target VCPUs mask */
> >> u32 mpidr; /* GICv3 target VCPU */
> >> @@ -301,7 +302,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >> bool level);
> >> int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >> bool level);
> >> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
> >> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> >> + u32 virt_irq, u32 phys_irq);
> >> int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
> >> bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
> >>
> >> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >> index 5976609..45f4779 100644
> >> --- a/virt/kvm/arm/arch_timer.c
> >> +++ b/virt/kvm/arm/arch_timer.c
> >> @@ -651,7 +651,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
> >> * Tell the VGIC that the virtual interrupt is tied to a
> >> * physical interrupt. We do that once per VCPU.
> >> */
> >> - ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
> >> + ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq,
> >> + vtimer->irq.irq, phys_irq);
> >> if (ret)
> >> return ret;
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> >> index 83b24d2..aa0618c 100644
> >> --- a/virt/kvm/arm/vgic/vgic.c
> >> +++ b/virt/kvm/arm/vgic/vgic.c
> >> @@ -137,6 +137,28 @@ 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_unshared_mapped(irq)))
> >> + WARN_ON(irq_get_irqchip_state(irq->host_irq,
> >> + IRQCHIP_STATE_PENDING,
> >> + &line_level));
> >> + return line_level;
> >> +}
> >
> > This really looks fishy. When do we need this exactly?
> >
> > I feel like we should treat this more like everything else and set the
> > line_level on the irq even for forwarded interrupts, and then you don't
> > need changes to validate injection.
> >
> > The challenge, then, is how to re-sample the line and lower the
> > line_level field when necessary. Can't we simply do this in
> > vgic_fold_lr_state(), and if you have a forwarded interrupt which is
> > level triggered and the level is high,
> Didn't you mean level is low? When the level is low you would trigger
> the resamplefd? Doesn't it bring a useless overhead?
No, I meant high.
So the idea would be that (for example) VFIO gets notified when the
level goes up, and can forward this information to KVM at that time, but
neither VFIO nor KVM get notified when the level lowers again. The idea
would then be that we only care about this change when we're done with
the virtual interrupt, so if the line_level is already high, we should
do the resample trick to get VFIO to lower the line - sort of the
opposite from the non-forwarding case.
But I'm now realizing that this doesn't work as easily as I thought,
because then you would have to change the injection path to IPI the VM
when setting the line_level to high when it's already high, and the
interrupt is a forwarded one.
>
> So yes I was confused by the spec and I thought the pending and active
> bit*s* were not reliable in the LRs and instead stared at the
> distributor state.
Been there done that.
>
> That being said, as you mention, the line level is difficult to model as
> the resamplefd is not called so why not directly looking at the physical
> distributor state (pending bit?). The impact on the SM is reduced to
> irq_line_level() and validate_injection() which is always true for
> forwarded interrupts.
>
Because the virtual state is in fact decoupled from the physical state,
so it feels wrong to me to start peeking into the physical GIC for our
emulated model.
Thanks,
-Christoffer
On Thu, Jun 08, 2017 at 10:55:29AM +0200, Auger Eric wrote:
> Hi Christoffer, Marc,
>
> On 08/06/2017 10:34, Christoffer Dall wrote:
> > On Thu, Jun 08, 2017 at 09:23:16AM +0100, Marc Zyngier wrote:
> >> On Fri, Jun 02 2017 at 6:29:44 pm BST, Christoffer Dall <[email protected]> wrote:
> >>> On Fri, Jun 02, 2017 at 03:10:23PM +0100, Marc Zyngier wrote:
> >>>> On 02/06/17 14:33, Christoffer Dall wrote:
> >>>>> On Wed, May 24, 2017 at 10:13:21PM +0200, Eric Auger wrote:
> >>>>>> Virtual interrupts directly mapped to physical interrupts require
> >>>>>> some special care. Their pending and active state must be observed
> >>>>>> at distributor level and not in the list register.
> >>>>>
> >>>>> This is not entirely true. There's a dependency, but there is also
> >>>>> separate virtual vs. physical state, see below.
> >>>>
> >>>> I think this stems for the usual confusion about the "pending and active
> >>>> state" vs "pending and active states". Yes, the GIC spec is rubbish. Can
> >>>> I state this again?
> >>>>
> >>>>>>
> >>>>>> Also a level sensitive interrupt's level is not toggled down by any
> >>>>>> maintenance IRQ handler as the EOI is not trapped.
> >>>>>>
> >>>>>> This patch adds an host_irq field in vgic_irq struct to easily
> >>>>>> get the irqchip state of the host irq. We also handle the
> >>>>>> physical IRQ case in vgic_validate_injection and add helpers to
> >>>>>> get the line level and active state.
> >>>>>>
> >>>>>> Signed-off-by: Eric Auger <[email protected]>
> >>>>>> ---
> >>>>>> include/kvm/arm_vgic.h | 4 +++-
> >>>>>> virt/kvm/arm/arch_timer.c | 3 ++-
> >>>>>> virt/kvm/arm/vgic/vgic.c | 44 ++++++++++++++++++++++++++++++++++++++------
> >>>>>> virt/kvm/arm/vgic/vgic.h | 9 ++++++++-
> >>>>>> 4 files changed, 51 insertions(+), 9 deletions(-)
> >>>>>>
> >>>>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> >>>>>> index ef71858..695ebc7 100644
> >>>>>> --- a/include/kvm/arm_vgic.h
> >>>>>> +++ b/include/kvm/arm_vgic.h
> >>>>>> @@ -112,6 +112,7 @@ struct vgic_irq {
> >>>>>> bool hw; /* Tied to HW IRQ */
> >>>>>> struct kref refcount; /* Used for LPIs */
> >>>>>> u32 hwintid; /* HW INTID number */
> >>>>>> + unsigned int host_irq; /* linux irq corresponding to hwintid */
> >>>>>> union {
> >>>>>> u8 targets; /* GICv2 target VCPUs mask */
> >>>>>> u32 mpidr; /* GICv3 target VCPU */
> >>>>>> @@ -301,7 +302,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >>>>>> bool level);
> >>>>>> int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >>>>>> bool level);
> >>>>>> -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
> >>>>>> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> >>>>>> + u32 virt_irq, u32 phys_irq);
> >>>>>> int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
> >>>>>> bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
> >>>>>>
> >>>>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >>>>>> index 5976609..45f4779 100644
> >>>>>> --- a/virt/kvm/arm/arch_timer.c
> >>>>>> +++ b/virt/kvm/arm/arch_timer.c
> >>>>>> @@ -651,7 +651,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
> >>>>>> * Tell the VGIC that the virtual interrupt is tied to a
> >>>>>> * physical interrupt. We do that once per VCPU.
> >>>>>> */
> >>>>>> - ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
> >>>>>> + ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq,
> >>>>>> + vtimer->irq.irq, phys_irq);
> >>>>>> if (ret)
> >>>>>> return ret;
> >>>>>>
> >>>>>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> >>>>>> index 83b24d2..aa0618c 100644
> >>>>>> --- a/virt/kvm/arm/vgic/vgic.c
> >>>>>> +++ b/virt/kvm/arm/vgic/vgic.c
> >>>>>> @@ -137,6 +137,28 @@ 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_unshared_mapped(irq)))
> >>>>>> + WARN_ON(irq_get_irqchip_state(irq->host_irq,
> >>>>>> + IRQCHIP_STATE_PENDING,
> >>>>>> + &line_level));
> >>>>>> + return line_level;
> >>>>>> +}
> >>>>>
> >>>>> This really looks fishy. When do we need this exactly?
> >>>>>
> >>>>> I feel like we should treat this more like everything else and set the
> >>>>> line_level on the irq even for forwarded interrupts, and then you don't
> >>>>> need changes to validate injection.
> >>>>>
> >>>>> The challenge, then, is how to re-sample the line and lower the
> >>>>> line_level field when necessary. Can't we simply do this in
> >>>>> vgic_fold_lr_state(), and if you have a forwarded interrupt which is
> >>>>> level triggered and the level is high, then notify the one who injected
> >>>>> this and tell it to adjust its line level (lower it if it changed).
> >>>>>
> >>>>> That would follow our existing path very closely.
> >>>>>
> >>>>> Am I missing something?
> >>>>
> >>>> I don't think you are. I think Eric got confused because of the above.
> >>>> But the flow is a bit a brainfsck :-(
> >>>>
> >>>> - Physical interrupt fires, activated, injected in the vgic
> >>>> - Injecting the interrupt has a very different flow from what we
> >>>> currently have, and follow the same pattern as an Edge interrupt
> >>>> (because the Pending state is kept at the physical distributor, so we
> >>>> cannot preserve it in the emulation).
> >>>> - Normal life cycle of the interrupt
> >>>> - The fact that the Pending bit is kept at the distributor level ensures
> >>>> that if it becomes pending again in the emulation, that's because the
> >>>> guest has deactivated the physical interrupt by doing an EOI.
> >>>>
> >>>
> >>> I think there's a choice between how we choose to support this. We can
> >>> either do the edge-like injection, or we can model the line_level to the
> >>> best of our ability (we just have to lower the line after the guest
> >>> exits after deactivation if it's not still pending at the physical
> >>> distributor).
> >>>
> >>> One question with doing this edge-like, can you ahve this scenario:
> >>> 1. VM runs with active virtual interrupt linked to physical
> >>> interrupt.
> >>> 2. VM deactivates virtual+physical interrupt
> >>> 3. Physical interrupt fires again on the host
> >>> 4. The host injects the virtual interrupt as pending to the VGIC (and
> >>> IPIs the VCPU etc.)
> >>> 5. The device lowers the physical line (another VPCU programs the
> >>> device, there's some delay, or whatever)
> >>> 6. The VCPU now sees a pending interrupt, which is no longer pending.
> >>>
> >>> Not sure if the line-like approach really solves this, though, or if
> >>> getting a spurius interrupt is something we care about.
> >>
> >> That would be a spurious interrupt indeed, but I'm not sure that's
> >> something the line level sampling you suggest would avoid either. There
> >> is a fundamental disconnect between the injection and the physical line,
> >> and it can only be modelled to some level of accuracy (/me curse the
> >> architecture again).
> >>
> >>> Perhaps we need to try to implement both and see how it looks like?
> >>
> >> There is definitely room for experiment, but I feel Eric should focus on
> >> one of them (whichever it is). Happy to help prototyping the other one
> >> though.
> >>
> > That's fair. I'm just worried about the whole "emulate level triggered
> > interrupts as edge triggered" thing, but as you said, the architecture
> > doesn't allow us to model it more accurately.
> if the line level is modeled using the physical distributor pending
> state, don't you fix that case?
>
I see what you mean. Perhaps. So that would mean that we move
line_level() to the physical distributor for forwarded interrupts, but
the active state is managed virtually in the GIC?
That contradicts my argument on the other mail, but if it's a more
accurate emulation, then that's definitely worth considering.
Thanks,
-Christoffer