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;
Date:   Sat, 21 Aug 2021 11:56:46 +0100
Message-ID: <875yvzqd5d.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Raghavendra Rao Ananta <rananta@google.com>
Cc:     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>,
        Oliver Upton <oupton@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
Subject: Re: [PATCH] KVM: arm64: Ratelimit error log during guest debug exception
In-Reply-To: <CAJHc60wn7PP1zQ5EKOGQDFbZsf=d9codWTuWbtMT5AHegfbVHw@mail.gmail.com>
References: <20210819223406.1132426-1-rananta@google.com>
        <87sfz4qx9r.wl-maz@kernel.org>
        <CAJHc60wn7PP1zQ5EKOGQDFbZsf=d9codWTuWbtMT5AHegfbVHw@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Sat, 21 Aug 2021 00:01:24 +0100,
Raghavendra Rao Ananta <rananta@google.com> wrote:
> 
> [1  <text/plain; UTF-8 (7bit)>]
> On Fri, Aug 20, 2021 at 2:29 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 19 Aug 2021 23:34:06 +0100,
> > Raghavendra Rao Ananta <rananta@google.com> wrote:
> > >
> > > Potentially, the guests could trigger a debug exception that's
> > > outside the exception class range.
> >
> > How? All the exception classes that lead to this functions are already
> > handled in the switch/case statement.
> >
> I guess I didn't think this through. Landing into kvm_handle_guest_debug()
> itself is not possible :)

Exactly.

> > My take on this is that this code isn't reachable, and that it could
> > be better rewritten as:
> >
> > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> > index 6f48336b1d86..ae7ec086827b 100644
> > --- a/arch/arm64/kvm/handle_exit.c
> > +++ b/arch/arm64/kvm/handle_exit.c
> > @@ -119,28 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu
> *vcpu)
> >  {
> >         struct kvm_run *run = vcpu->run;
> >         u32 esr = kvm_vcpu_get_esr(vcpu);
> > -       int ret = 0;
> >
> >         run->exit_reason = KVM_EXIT_DEBUG;
> >         run->debug.arch.hsr = esr;
> >
> > -       switch (ESR_ELx_EC(esr)) {
> > -       case ESR_ELx_EC_WATCHPT_LOW:
> > +       if (ESR_ELx_EC(esr) ==  ESR_ELx_EC_WATCHPT_LOW)
> >                 run->debug.arch.far = vcpu->arch.fault.far_el2;
> > -               fallthrough;
> > -       case ESR_ELx_EC_SOFTSTP_LOW:
> > -       case ESR_ELx_EC_BREAKPT_LOW:
> > -       case ESR_ELx_EC_BKPT32:
> > -       case ESR_ELx_EC_BRK64:
> > -               break;
> > -       default:
> > -               kvm_err("%s: un-handled case esr: %#08x\n",
> > -                       __func__, (unsigned int) esr);
> > -               ret = -1;
> > -               break;
> > -       }
> >
> > -       return ret;
> > +       return 0;
> >  }
> >
> This looks better, but do you think we would be compromising on readability?

I don't think so. The exit handler table is, on its own, pretty
explicit about what we route to this handler, and the comment above
the function clearly states that we exit to userspace for all the
debug ECs.

	M.

-- 
Without deviation from the norm, progress is not possible.