On Fri, Aug 20, 2021 at 2:29 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Thu, 19 Aug 2021 23:34:06 +0100,
> Raghavendra Rao Ananta <rananta@xxxxxxxxxx> 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 :)
> > This could lead to an excessive syslog flooding. Hence, ratelimit
> > the error message.
> >
> > Signed-off-by: Raghavendra Rao Ananta <rananta@xxxxxxxxxx>
> > ---
> > 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;
> }
>
This looks better, but do you think we would be compromising on readability?
Regards,
Raghavendra
> static int kvm_handle_unknown_ec(struct kvm_vcpu *vcpu)
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
