linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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	[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	[flat|nested] 5+ messages in thread

* 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).