* [PATCH] KVM: arm64: Ratelimit error log during guest debug exception @ 2021-08-19 22:34 Raghavendra Rao Ananta 2021-08-20 9:29 ` Marc Zyngier 0 siblings, 1 reply; 5+ messages in thread From: Raghavendra Rao Ananta @ 2021-08-19 22:34 UTC (permalink / raw) To: Marc Zyngier, Alexandru Elisei, Suzuki K Poulose Cc: Catalin Marinas, Will Deacon, Peter Shier, Ricardo Koller, Oliver Upton, Reiji Watanabe, Jing Zhang, Raghavendra Rao Anata, linux-arm-kernel, kvmarm, linux-kernel 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(-) 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; -- 2.33.0.rc2.250.ged5fa647cd-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: arm64: Ratelimit error log during guest debug exception 2021-08-19 22:34 [PATCH] KVM: arm64: Ratelimit error log during guest debug exception Raghavendra Rao Ananta @ 2021-08-20 9:29 ` Marc Zyngier [not found] ` <CAJHc60wn7PP1zQ5EKOGQDFbZsf=d9codWTuWbtMT5AHegfbVHw@mail.gmail.com> 0 siblings, 1 reply; 5+ messages in thread From: Marc Zyngier @ 2021-08-20 9:29 UTC (permalink / raw) To: Raghavendra Rao Ananta Cc: Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Peter Shier, Ricardo Koller, Oliver Upton, Reiji Watanabe, Jing Zhang, linux-arm-kernel, kvmarm, linux-kernel 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. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply related [flat|nested] 5+ messages in thread
[parent not found: <CAJHc60wn7PP1zQ5EKOGQDFbZsf=d9codWTuWbtMT5AHegfbVHw@mail.gmail.com>]
* Re: [PATCH] KVM: arm64: Ratelimit error log during guest debug exception [not found] ` <CAJHc60wn7PP1zQ5EKOGQDFbZsf=d9codWTuWbtMT5AHegfbVHw@mail.gmail.com> @ 2021-08-21 10:56 ` Marc Zyngier 2021-08-23 18:13 ` Raghavendra Rao Ananta 0 siblings, 1 reply; 5+ messages in thread From: Marc Zyngier @ 2021-08-21 10:56 UTC (permalink / raw) To: Raghavendra Rao Ananta Cc: Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Peter Shier, Ricardo Koller, Oliver Upton, Reiji Watanabe, Jing Zhang, linux-arm-kernel, kvmarm, linux-kernel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: arm64: Ratelimit error log during guest debug exception 2021-08-21 10:56 ` Marc Zyngier @ 2021-08-23 18:13 ` Raghavendra Rao Ananta 2021-08-23 21:51 ` Marc Zyngier 0 siblings, 1 reply; 5+ messages in thread From: Raghavendra Rao Ananta @ 2021-08-23 18:13 UTC (permalink / raw) To: Marc Zyngier Cc: Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Peter Shier, Ricardo Koller, Oliver Upton, Reiji Watanabe, Jing Zhang, linux-arm-kernel, kvmarm, linux-kernel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: arm64: Ratelimit error log during guest debug exception 2021-08-23 18:13 ` Raghavendra Rao Ananta @ 2021-08-23 21:51 ` Marc Zyngier 0 siblings, 0 replies; 5+ messages in thread From: Marc Zyngier @ 2021-08-23 21:51 UTC (permalink / raw) To: Raghavendra Rao Ananta Cc: Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon, Peter Shier, Ricardo Koller, Oliver Upton, Reiji Watanabe, Jing Zhang, linux-arm-kernel, kvmarm, linux-kernel 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. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-23 21:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-19 22:34 [PATCH] KVM: arm64: Ratelimit error log during guest debug exception Raghavendra Rao Ananta
2021-08-20 9:29 ` Marc Zyngier
[not found] ` <CAJHc60wn7PP1zQ5EKOGQDFbZsf=d9codWTuWbtMT5AHegfbVHw@mail.gmail.com>
2021-08-21 10:56 ` Marc Zyngier
2021-08-23 18:13 ` Raghavendra Rao Ananta
2021-08-23 21:51 ` Marc Zyngier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).