Re: [Qemu-devel] [PATCH for-4.2] xics/kvm: Convert assert() to error_setg()

[Greg Kurz]

On Thu, 4 Jul 2019 10:23:57 +1000
David Gibson <address@hidden> wrote:

> On Wed, Jul 03, 2019 at 07:50:12PM +0200, Greg Kurz wrote:
> > ics_set_kvm_state_one() is called either during reset, in which case
> > both 'saved priority' and 'current priority' are equal to 0xff, or
> > during migration. In the latter case, 'saved priority' may differ
> > from 'current priority' only if the interrupt had been masked with
> > the ibm,int-off RTAS call. Instead of aborting QEMU, print out an
> > error and exit.
> 
> What's the rationale for this?  Doesn't hitting this indicate an error
> in the qemu code, for which an abort is the usual response?
> 

This error can be hit by the destination during migration if the
incoming stream is corrupted. Aborting in this case would mislead
the user into suspecting a bug in the destination QEMU, which isn't
the case.

Appart from that, when the in-kernel XICS is in use, only two functions
manipulate the ICS state: ics_set_kvm_state_one() and ics_get_kvm_state().
The code is trivial enough that I don't see a great value in the assert
in the first place... BTW, it comes from the commit:

commit 11ad93f68195f68cc94d988f2aa50b4d190ee52a
Author: David Gibson <address@hidden>
Date:   Thu Sep 26 16:18:44 2013 +1000

    xics-kvm: Support for in-kernel XICS interrupt controller

Maybe you remember some context that justified the assert at the time ?

> > 
> > Based-on: <address@hidden>
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> > 
> > This isn't a bugfix, hence targetting 4.2, but it depends on an actual
> > fix for 4.1, as mentionned in the Based-on tag.
> > ---
> >  hw/intc/xics_kvm.c |   17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> > index 2df1f3e92c7e..f8758b928250 100644
> > --- a/hw/intc/xics_kvm.c
> > +++ b/hw/intc/xics_kvm.c
> > @@ -255,8 +255,21 @@ int ics_set_kvm_state_one(ICSState *ics, int srcno, 
> > Error **errp)
> >      state = irq->server;
> >      state |= (uint64_t)(irq->saved_priority & KVM_XICS_PRIORITY_MASK)
> >          << KVM_XICS_PRIORITY_SHIFT;
> > -    if (irq->priority != irq->saved_priority) {
> > -        assert(irq->priority == 0xff);
> > +
> > +    /*
> > +     * An interrupt can be masked either because the ICS is resetting, in
> > +     * which case we expect 'current priority' and 'saved priority' to be
> > +     * equal to 0xff, or because the guest has called the ibm,int-off RTAS
> > +     * call, in which case we we have recorded the priority the interrupt
> > +     * had before it was masked in 'saved priority'. If the interrupt isn't
> > +     * masked, 'saved priority' and 'current priority' are equal (see
> > +     * ics_get_kvm_state()). Make sure we restore a sane state, otherwise
> > +     * fail migration.
> > +     */
> > +    if (irq->priority != irq->saved_priority && irq->priority != 0xff) {
> > +        error_setg(errp, "Corrupted state detected for interrupt source 
> > %d",
> > +                   srcno);
> > +        return -EINVAL;
> >      }
> >  
> >      if (irq->priority == 0xff) {
> > 
>