| Message ID | 20210819223406.1132426-1-rananta@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Ratelimit error log during guest debug exception | expand |
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. > This could lead to an excessive syslog flooding. Hence, ratelimit > the error message. > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > --- > arch/arm64/kvm/handle_exit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index 04ebab299aa4..c7cec7ffe93c 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -134,7 +134,7 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu) > case ESR_ELx_EC_BRK64: > break; > default: > - kvm_err("%s: un-handled case esr: %#08x\n", > + kvm_pr_unimpl("%s: un-handled case esr: %#08x\n", > __func__, (unsigned int) esr); > ret = -1; > break; 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; } static int kvm_handle_unknown_ec(struct kvm_vcpu *vcpu) Thanks, M.
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.
On Sat, Aug 21, 2021 at 3:56 AM Marc Zyngier <maz@kernel.org> wrote: > > 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. Sounds great. I'm happy to send out a patch with you as 'Suggested-by' , if you are okay with it. Regards, Raghavendra > > M. > > -- > Without deviation from the norm, progress is not possible.
On 2021-08-23 19:13, Raghavendra Rao Ananta wrote: > On Sat, Aug 21, 2021 at 3:56 AM Marc Zyngier <maz@kernel.org> wrote: >> >> 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. > > Sounds great. I'm happy to send out a patch with you as 'Suggested-by' > , if you > are okay with it. Fire away! M.
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index 04ebab299aa4..c7cec7ffe93c 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -134,7 +134,7 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu) case ESR_ELx_EC_BRK64: break; default: - kvm_err("%s: un-handled case esr: %#08x\n", + kvm_pr_unimpl("%s: un-handled case esr: %#08x\n", __func__, (unsigned int) esr); ret = -1; break;
Potentially, the guests could trigger a debug exception that's outside the exception class range. This could lead to an excessive syslog flooding. Hence, ratelimit the error message. Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> --- arch/arm64/kvm/handle_exit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)