On Wed, Aug 08, 2018 at 02:14:59PM +0100, Marc Zyngier wrote:
> Although vgic-v3 now supports Group0 interrupts, it still doesn't
> deal with Group0 SGIs. As usually with the GIC, nothing is simple:
>
> - ICC_SGI1R can signal SGIs of both groups, since GICD_CTLR.DS==1
> with KVM (as per 8.1.10, Non-secure EL1 access)
>
> - ICC_SGI0R can only generate Group0 SGIs
>
> - ICC_ASGI1R sees its scope refocussed to generate only Group0
> SGIs (as per the note at the bottom of Table 8-14)
>
> One way to look at this is that an SGI can be generated if the
> group implied by the CPU interface is lower or equal to the
> group specified by the interrupt.
This sentence hurts my brain. Another way to look at it is that with
DS=1, only SGI1R allows signaling group1 interrupts. See below.
>
> We only support Group1 SGIs so far, so no material change.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
> arch/arm/kvm/coproc.c | 2 +-
> arch/arm64/kvm/sys_regs.c | 2 +-
> include/kvm/arm_vgic.h | 2 +-
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 16 +++++++++++++---
> 4 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index 3a02e76699a6..ec517992c12d 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -253,7 +253,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
> reg = (u64)*vcpu_reg(vcpu, p->Rt2) << 32;
> reg |= *vcpu_reg(vcpu, p->Rt1) ;
>
> - vgic_v3_dispatch_sgi(vcpu, reg);
> + vgic_v3_dispatch_sgi(vcpu, reg, 1);
>
> return true;
> }
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index e04aacb2a24c..a09139b97e81 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -255,7 +255,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
> if (!p->is_write)
> return read_from_write_only(vcpu, p, r);
>
> - vgic_v3_dispatch_sgi(vcpu, p->regval);
> + vgic_v3_dispatch_sgi(vcpu, p->regval, 1);
>
> return true;
> }
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index c134790be32c..06a25b11efa7 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -373,7 +373,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
> void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu);
> void kvm_vgic_reset_mapped_irq(struct kvm_vcpu *vcpu, u32 vintid);
>
> -void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
> +void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group);
>
> /**
> * kvm_vgic_get_max_vcpus - Get the maximum number of VCPUs allowed by HW
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 88e78b582139..11d321f7ad71 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -901,6 +901,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
> * vgic_v3_dispatch_sgi - handle SGI requests from VCPUs
> * @vcpu: The VCPU requesting a SGI
> * @reg: The value written into the ICC_SGI1R_EL1 register by that VCPU
> + * @group: Interrupt group requested by the sender
> *
> * With GICv3 (and ARE=1) CPUs trigger SGIs by writing to a system register.
> * This will trap in sys_regs.c and call this function.
> @@ -910,7 +911,7 @@ static int match_mpidr(u64 sgi_aff, u16 sgi_cpu_mask, struct kvm_vcpu *vcpu)
> * check for matching ones. If this bit is set, we signal all, but not the
> * calling VCPU.
> */
> -void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
> +void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, int group)
> {
> struct kvm *kvm = vcpu->kvm;
> struct kvm_vcpu *c_vcpu;
> @@ -959,9 +960,18 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
> irq = vgic_get_irq(vcpu->kvm, c_vcpu, sgi);
>
> spin_lock_irqsave(&irq->irq_lock, flags);
> - irq->pending_latch = true;
>
> - vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> + /*
> + * A Group0 access can only generate a Group0 SGI,
> + * while a Group1 access can generate either group.
> + */
nit: "Is a Group 0 access" a well-defined term?
I think it may be clearer if the parameter was: bool allow_group1
and the condition was:
if (irq->group && allow_group1) {
...
At least that would match the comment.
> + if (irq->group <= group) {
> + irq->pending_latch = true;
> + vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
> + } else {
> + spin_unlock_irqrestore(&irq->irq_lock, flags);
> + }
> +
> vgic_put_irq(vcpu->kvm, irq);
> }
> }
> --
> 2.18.0
>
Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
