Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210909013818.1191270-1-rananta@google.com> <20210909013818.1191270-12-rananta@google.com>
 <YTmZPSEm3Fj6l1PN@google.com>
In-Reply-To: <YTmZPSEm3Fj6l1PN@google.com>
From:   Raghavendra Rao Ananta <rananta@google.com>
Date:   Thu, 9 Sep 2021 09:38:56 -0700
Message-ID: <CAJHc60x_r46W+81=A76zC=zW_3xqmvMWf3CspiQKVVnPA0TtTA@mail.gmail.com>
Subject: Re: [PATCH v4 11/18] KVM: arm64: selftests: Add basic GICv3 support
To:     Oliver Upton <oupton@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, Marc Zyngier <maz@kernel.org>,
        Andrew Jones <drjones@redhat.com>,
        James Morse <james.morse@arm.com>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>, Peter Shier <pshier@google.com>,
        Ricardo Koller <ricarkol@google.com>,
        Reiji Watanabe <reijiw@google.com>,
        Jing Zhang <jingzhangos@google.com>,
        linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Sep 8, 2021 at 10:18 PM Oliver Upton <oupton@google.com> wrote:
>
> On Thu, Sep 09, 2021 at 01:38:11AM +0000, Raghavendra Rao Ananta wrote:
> > Add basic support for ARM Generic Interrupt Controller v3.
> > The support provides guests to setup interrupts.
> >
> > The work is inspired from kvm-unit-tests and the kernel's
> > GIC driver (drivers/irqchip/irq-gic-v3.c).
> >
> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  tools/testing/selftests/kvm/Makefile          |   2 +-
> >  .../selftests/kvm/include/aarch64/gic.h       |  21 ++
> >  tools/testing/selftests/kvm/lib/aarch64/gic.c |  93 +++++++
> >  .../selftests/kvm/lib/aarch64/gic_private.h   |  21 ++
> >  .../selftests/kvm/lib/aarch64/gic_v3.c        | 240 ++++++++++++++++++
> >  .../selftests/kvm/lib/aarch64/gic_v3.h        |  70 +++++
> >  6 files changed, 446 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/kvm/include/aarch64/gic.h
> >  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic.c
> >  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_private.h
> >  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_v3.c
> >  create mode 100644 tools/testing/selftests/kvm/lib/aarch64/gic_v3.h
> >
>
> [...]
>
> > +static void
> > +gic_dist_init(enum gic_type type, unsigned int nr_cpus, void *dist_base)
> > +{
> > +     const struct gic_common_ops *gic_ops;
>
> does this need to be initialized? I haven't tried compiling, but it
> seems it should trigger a compiler warning as it is only initialized if
> type == GIC_V3.
>
Huh, I thought I had a default case covering this (must have gone lost
during code reorg).
Nice catch though! Surprisingly, the compiler never warned. I'm not
sure if its smart
enough to figure out that the caller of this function had
GUEST_ASSERT(type < GIC_TYPE_MAX);
Anyway, I'll clean it up.

Regards,
Raghavendra

> > +     spin_lock(&gic_lock);
> > +
> > +     /* Distributor initialization is needed only once per VM */
> > +     if (gic_common_ops) {
> > +             spin_unlock(&gic_lock);
> > +             return;
> > +     }
> > +
> > +     if (type == GIC_V3)
> > +             gic_ops = &gicv3_ops;
> > +
> > +     gic_ops->gic_init(nr_cpus, dist_base);
> > +     gic_common_ops = gic_ops;
> > +
> > +     /* Make sure that the initialized data is visible to all the vCPUs */
> > +     dsb(sy);
> > +
> > +     spin_unlock(&gic_lock);
> > +}
>