On Sun, 10 Nov 2019 14:15:38 +0000
Marc Zyngier <maz@xxxxxxxxxx> wrote:
Hi Marc,
thanks for having a look!
> On Fri, 8 Nov 2019 17:49:50 +0000
> Andre Przywara <andre.przywara@xxxxxxx> wrote:
>
> > The GIC specification describes support for two distinct interrupt
> > groups, which can be enabled independently from each other. At the
> > moment our VGIC emulation does not really honour this, so using
> > Group0 interrupts with the GICv3 emulation does not work as expected
> > at the moment.
> >
> > Prepare the code for dealing with the *two* interrupt groups:
> > Allow to handle the two enable bits in the distributor, by providing
> > accessors. At the moment this still only honours group1, because we
> > need more code to properly differentiate the group enables.
> > Also provide a stub function to later implement the re-scanning of all
> > IRQs, should the group enable bit for a group change.
> >
> > This patch does not change the current behaviour yet, but just provides
> > the infrastructure bits separately, mostly for review purposes.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > ---
> > include/kvm/arm_vgic.h | 7 ++-
> > virt/kvm/arm/vgic/vgic-debug.c | 2 +-
> > virt/kvm/arm/vgic/vgic-mmio-v2.c | 23 ++++++----
> > virt/kvm/arm/vgic/vgic-mmio-v3.c | 22 +++++----
> > virt/kvm/arm/vgic/vgic.c | 76 +++++++++++++++++++++++++++++++-
> > virt/kvm/arm/vgic/vgic.h | 3 ++
> > 6 files changed, 110 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index 9d53f545a3d5..0f845430c732 100644
> > --- a/include/kvm/arm_vgic.h
> > +++ b/include/kvm/arm_vgic.h
> > @@ -29,6 +29,9 @@
> > #define VGIC_MIN_LPI 8192
> > #define KVM_IRQCHIP_NUM_PINS (1020 - 32)
> >
> > +#define GIC_GROUP0 0
> > +#define GIC_GROUP1 1
>
> Is there any reason why:
> - these are not bit masks (1 << 0, 1 << 1)
I needed them this way to do the shifting of the new enable bit later, in vgic_dist_enable_group(). But I see that this the only place, so I will try to use masks everywhere.
> - they are not using architectural constants (GICD_EnableGroup0,
> GICD_EnableGroup1)
>
> > +
> > #define irq_is_ppi(irq) ((irq) >= VGIC_NR_SGIS && (irq) < VGIC_NR_PRIVATE_IRQS)
> > #define irq_is_spi(irq) ((irq) >= VGIC_NR_PRIVATE_IRQS && \
> > (irq) <= VGIC_MAX_SPI)
> > @@ -227,8 +230,8 @@ struct vgic_dist {
> > struct list_head rd_regions;
> > };
> >
> > - /* distributor enabled */
> > - bool enabled;
> > + /* group0 and/or group1 IRQs enabled on the distributor level */
> > + u8 groups_enable;
>
> The comment is a bit misleading. This should be a bitmap of the enabled
> groups, limited to groups 0 and 1 (short of having more of the stuff).
Which it actually is, it's just not spelled out here.
Do you want to use an explicit C bitfield here or just have the comment amended?
> >
> > struct vgic_irq *spis;
> >
> > diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c
> > index cc12fe9b2df3..ab64e908024e 100644
> > --- a/virt/kvm/arm/vgic/vgic-debug.c
> > +++ b/virt/kvm/arm/vgic/vgic-debug.c
> > @@ -150,7 +150,7 @@ static void print_dist_state(struct seq_file *s, struct vgic_dist *dist)
> > seq_printf(s, "nr_spis:\t%d\n", dist->nr_spis);
> > if (v3)
> > seq_printf(s, "nr_lpis:\t%d\n", dist->lpi_list_count);
> > - seq_printf(s, "enabled:\t%d\n", dist->enabled);
> > + seq_printf(s, "groups enabled:\t%d\n", dist->groups_enable);
>
> You could actually print 0 and/or 1, instead of 0, 1, 2 or 3...
>
> > seq_printf(s, "\n");
> >
> > seq_printf(s, "P=pending_latch, L=line_level, A=active\n");
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > index 5945f062d749..fe8528bd5b55 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -26,11 +26,14 @@ static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
> > gpa_t addr, unsigned int len)
> > {
> > struct vgic_dist *vgic = &vcpu->kvm->arch.vgic;
> > - u32 value;
> > + u32 value = 0;
> >
> > switch (addr & 0x0c) {
> > case GIC_DIST_CTRL:
> > - value = vgic->enabled ? GICD_ENABLE : 0;
> > + if (vgic_dist_group_enabled(vcpu->kvm, GIC_GROUP0))
> > + value |= GICD_ENABLE;
> > + if (vgic_dist_group_enabled(vcpu->kvm, GIC_GROUP1))
> > + value |= BIT(1);
>
> Time to follow the naming in the spec, like on GICv3.
Which is a bit confusing for GICv2, since it uses bit 0 for group 1 interrupts in dual-security/non-secure mode, and the very same bit for group 0 interrupts in our single-security case. Shall I just keep the existing name for bit 0, and add an explicit GICD_ENABLE_GRP1 name for bit 1? GICD_ENABLE is also used in the irqchip driver.
> > break;
> > case GIC_DIST_CTR:
> > value = vgic->nr_spis + VGIC_NR_PRIVATE_IRQS;
> > @@ -42,8 +45,6 @@ static unsigned long vgic_mmio_read_v2_misc(struct kvm_vcpu *vcpu,
> > (vgic->implementation_rev << GICD_IIDR_REVISION_SHIFT) |
> > (IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
> > break;
> > - default:
> > - return 0;
> > }
> >
> > return value;
> > @@ -53,14 +54,18 @@ static void vgic_mmio_write_v2_misc(struct kvm_vcpu *vcpu,
> > gpa_t addr, unsigned int len,
> > unsigned long val)
> > {
> > - struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> > - bool was_enabled = dist->enabled;
> > + struct kvm *kvm = vcpu->kvm;
> > + int grp0_changed, grp1_changed;
> >
> > switch (addr & 0x0c) {
> > case GIC_DIST_CTRL:
> > - dist->enabled = val & GICD_ENABLE;
> > - if (!was_enabled && dist->enabled)
> > - vgic_kick_vcpus(vcpu->kvm);
> > + grp0_changed = vgic_dist_enable_group(kvm, GIC_GROUP0,
> > + val & GICD_ENABLE);
> > + grp1_changed = vgic_dist_enable_group(kvm, GIC_GROUP1,
> > + val & BIT(1));
> > + if (grp0_changed || grp1_changed)
> > + vgic_rescan_pending_irqs(kvm, grp0_changed > 0 ||
> > + grp1_changed > 0);
> > break;
> > case GIC_DIST_CTR:
> > case GIC_DIST_IIDR:
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > index 7dfd15dbb308..73e3410af332 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > @@ -66,7 +66,9 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
> >
> > switch (addr & 0x0c) {
> > case GICD_CTLR:
> > - if (vgic->enabled)
> > + if (vgic_dist_group_enabled(vcpu->kvm, GIC_GROUP0))
> > + value |= GICD_CTLR_ENABLE_SS_G0;
> > + if (vgic_dist_group_enabled(vcpu->kvm, GIC_GROUP1))
> > value |= GICD_CTLR_ENABLE_SS_G1;
> > value |= GICD_CTLR_ARE_NS | GICD_CTLR_DS;
> > break;
> > @@ -85,8 +87,6 @@ static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
> > (vgic->implementation_rev << GICD_IIDR_REVISION_SHIFT) |
> > (IMPLEMENTER_ARM << GICD_IIDR_IMPLEMENTER_SHIFT);
> > break;
> > - default:
> > - return 0;
> > }
> >
> > return value;
> > @@ -96,15 +96,19 @@ static void vgic_mmio_write_v3_misc(struct kvm_vcpu *vcpu,
> > gpa_t addr, unsigned int len,
> > unsigned long val)
> > {
> > - struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> > - bool was_enabled = dist->enabled;
> > + struct kvm *kvm = vcpu->kvm;
> > + int grp0_changed, grp1_changed;
> >
> > switch (addr & 0x0c) {
> > case GICD_CTLR:
> > - dist->enabled = val & GICD_CTLR_ENABLE_SS_G1;
> > -
> > - if (!was_enabled && dist->enabled)
> > - vgic_kick_vcpus(vcpu->kvm);
> > + grp0_changed = vgic_dist_enable_group(kvm, GIC_GROUP0,
> > + val & GICD_CTLR_ENABLE_SS_G0);
> > + grp1_changed = vgic_dist_enable_group(kvm, GIC_GROUP1,
> > + val & GICD_CTLR_ENABLE_SS_G1);
> > +
> > + if (grp0_changed || grp1_changed)
> > + vgic_rescan_pending_irqs(kvm, grp0_changed > 0 ||
> > + grp1_changed > 0);
>
> Aren't you losing some state here? If I have disabled G0 and enabled G1
> at the same time, the result is "enabled", which makes little sense
> when you have two groups. My hunch is that you have to rescan the all
> the pending interrupts and match them against the group.
Mmh, I think you are right, in this case the kick routine at the beginning of vgic_rescan_pending_irqs() wouldn't trigger.
I will probably just pass on both the old and new bitmasks and let the callee figure that out (or ignore it).
>
> > break;
> > case GICD_TYPER:
> > case GICD_IIDR:
> > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > index 99b02ca730a8..3b88e14d239f 100644
> > --- a/virt/kvm/arm/vgic/vgic.c
> > +++ b/virt/kvm/arm/vgic/vgic.c
> > @@ -201,6 +201,12 @@ void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active)
> > active));
> > }
> >
> > +static bool vgic_irq_is_grp_enabled(struct kvm *kvm, struct vgic_irq *irq)
> > +{
> > + /* Placeholder implementation until we properly support Group0. */
> > + return kvm->arch.vgic.groups_enable;
> > +}
> > +
> > /**
> > * kvm_vgic_target_oracle - compute the target vcpu for an irq
> > *
> > @@ -228,7 +234,8 @@ static struct kvm_vcpu *vgic_target_oracle(struct vgic_irq *irq)
> > */
> > if (irq->enabled && irq_is_pending(irq)) {
> > if (unlikely(irq->target_vcpu &&
> > - !irq->target_vcpu->kvm->arch.vgic.enabled))
> > + !vgic_irq_is_grp_enabled(irq->target_vcpu->kvm,
> > + irq)))
> > return NULL;
> >
> > return irq->target_vcpu;
> > @@ -303,6 +310,71 @@ static void vgic_sort_ap_list(struct kvm_vcpu *vcpu)
> > list_sort(NULL, &vgic_cpu->ap_list_head, vgic_irq_cmp);
> > }
> >
> > +int vgic_dist_enable_group(struct kvm *kvm, int group, bool status)
> > +{
> > + struct vgic_dist *dist = &kvm->arch.vgic;
> > + u32 group_mask = 1U << group;
> > + u32 new_bit = (unsigned)status << group;
> > + u8 was_enabled = dist->groups_enable & group_mask;
> > +
> > + if (new_bit == was_enabled)
> > + return 0;
> > +
> > + /* Group 0 on GICv3 and Group 1 on GICv2 are ignored for now. */
> > + if (kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> > + if (group == GIC_GROUP0)
> > + return 0;
> > + } else {
> > + if (group == GIC_GROUP1)
> > + return 0;
> > + }
> > +
> > + dist->groups_enable &= ~group_mask;
> > + dist->groups_enable |= new_bit;
> > + if (new_bit > was_enabled)
> > + return 1;
> > + else
> > + return -1;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * The group enable status of at least one of the groups has changed.
> > + * If enabled is true, at least one of the groups got enabled.
> > + * Adjust the forwarding state of every IRQ (on ap_list or not) accordingly.
> > + */
> > +void vgic_rescan_pending_irqs(struct kvm *kvm, bool enabled)
> > +{
> > + /*
> > + * TODO: actually scan *all* IRQs of the VM for pending IRQs.
> > + * If a pending IRQ's group is now enabled, add it to its ap_list.
> > + * If a pending IRQ's group is now disabled, kick the VCPU to
> > + * let it remove this IRQ from its ap_list. We have to let the
> > + * VCPU do it itself, because we can't know the exact state of an
> > + * IRQ pending on a running VCPU.
> > + */
> > +
> > + /* For now just kick all VCPUs, as the old code did. */
> > + vgic_kick_vcpus(kvm);
> > +}
> > +
> > +bool vgic_dist_group_enabled(struct kvm *kvm, int group)
> > +{
> > + struct vgic_dist *dist = &kvm->arch.vgic;
> > +
> > + /* Group 0 on GICv3 and Group 1 on GICv2 are ignored for now. */
> > + if (kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> > + if (group == GIC_GROUP0)
> > + return false;
> > + } else {
> > + if (group == GIC_GROUP1)
> > + return false;
> > + }
> > +
> > + return dist->groups_enable & (1U << group);
> > +}
> > +
> > /*
> > * Only valid injection if changing level for level-triggered IRQs or for a
> > * rising edge, and in-kernel connected IRQ lines can only be controlled by
> > @@ -949,7 +1021,7 @@ int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
> > unsigned long flags;
> > struct vgic_vmcr vmcr;
> >
> > - if (!vcpu->kvm->arch.vgic.enabled)
> > + if (!vcpu->kvm->arch.vgic.groups_enable)
> > return false;
> >
> > if (vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last)
> > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> > index c7fefd6b1c80..219eb23d580d 100644
> > --- a/virt/kvm/arm/vgic/vgic.h
> > +++ b/virt/kvm/arm/vgic/vgic.h
> > @@ -168,7 +168,10 @@ void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending);
> > void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active);
> > bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
> > unsigned long flags);
> > +bool vgic_dist_group_enabled(struct kvm *kvm, int group);
> > +int vgic_dist_enable_group(struct kvm *kvm, int group, bool status);
> > void vgic_kick_vcpus(struct kvm *kvm);
> > +void vgic_rescan_pending_irqs(struct kvm *kvm, bool enabled);
> >
> > int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
> > phys_addr_t addr, phys_addr_t alignment);
>
>
> Overall, the logic seems a bit unclear. Please start by cleaning this
> up so that we know what we're talking about, and get rid of the
> (extremely premature) optimizations. If group enables get changed in any
> way, just rescan the whole thing. Nobody changes their enables anyway.
Looking back at it it seems indeed pretty rubbish. I will see if using masks everywhere can make this cleaner, plus trying to avoid too many optimisations.
Cheers,
Andre.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
