On 09/08/18 08:40, Christoffer Dall wrote:
> 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.
Fair enough.
>>
>> 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.
Indeed, that's better. Let me respin that.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
