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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]

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?

> 
> Based-on: <156217454083.559957.7359208229523652842.stgit@bahia.lan>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> 
> 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) {
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]