From: Cédric Le Goater <clg@kaod.org>

Introduce a new sPAPR IRQ handler to handle resend after migration
when the machine is using a KVM XICS interrupt controller model.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c             | 13 +++++--------
 hw/ppc/spapr_irq.c         | 27 +++++++++++++++++++++++++++
 include/hw/ppc/spapr_irq.h |  2 ++
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index dfb617e580..0b09a88753 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1730,14 +1730,6 @@ static int spapr_post_load(void *opaque, int version_id)
         return err;
     }
 
-    if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
-        CPUState *cs;
-        CPU_FOREACH(cs) {
-            PowerPCCPU *cpu = POWERPC_CPU(cs);
-            icp_resend(ICP(cpu->intc));
-        }
-    }
-
     /* In earlier versions, there was no separate qdev for the PAPR
      * RTC, so the RTC offset was stored directly in sPAPREnvironment.
      * So when migrating from those versions, poke the incoming offset
@@ -1758,6 +1750,11 @@ static int spapr_post_load(void *opaque, int version_id)
         }
     }
 
+    err = spapr_irq_post_load(spapr, version_id);
+    if (err) {
+        return err;
+    }
+
     return err;
 }
 
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index fdcc7795e4..292c448a15 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -197,6 +197,18 @@ static Object *spapr_irq_cpu_intc_create_xics(sPAPRMachineState *spapr,
     return icp_create(cpu, spapr->icp_type, XICS_FABRIC(spapr), errp);
 }
 
+static int spapr_irq_post_load_xics(sPAPRMachineState *spapr, int version_id)
+{
+    if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
+        CPUState *cs;
+        CPU_FOREACH(cs) {
+            PowerPCCPU *cpu = POWERPC_CPU(cs);
+            icp_resend(ICP(cpu->intc));
+        }
+    }
+    return 0;
+}
+
 #define SPAPR_IRQ_XICS_NR_IRQS     0x1000
 #define SPAPR_IRQ_XICS_NR_MSIS     \
     (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
@@ -212,6 +224,7 @@ sPAPRIrq spapr_irq_xics = {
     .print_info  = spapr_irq_print_info_xics,
     .dt_populate = spapr_dt_xics,
     .cpu_intc_create = spapr_irq_cpu_intc_create_xics,
+    .post_load   = spapr_irq_post_load_xics,
 };
 
 /*
@@ -295,6 +308,11 @@ static Object *spapr_irq_cpu_intc_create_xive(sPAPRMachineState *spapr,
     return xive_tctx_create(cpu, XIVE_ROUTER(spapr->xive), errp);
 }
 
+static int spapr_irq_post_load_xive(sPAPRMachineState *spapr, int version_id)
+{
+    return 0;
+}
+
 /*
  * XIVE uses the full IRQ number space. Set it to 8K to be compatible
  * with XICS.
@@ -314,6 +332,7 @@ sPAPRIrq spapr_irq_xive = {
     .print_info  = spapr_irq_print_info_xive,
     .dt_populate = spapr_dt_xive,
     .cpu_intc_create = spapr_irq_cpu_intc_create_xive,
+    .post_load   = spapr_irq_post_load_xive,
 };
 
 /*
@@ -352,6 +371,13 @@ qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
     return smc->irq->qirq(spapr, irq);
 }
 
+int spapr_irq_post_load(sPAPRMachineState *spapr, int version_id)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+
+    return smc->irq->post_load(spapr, version_id);
+}
+
 /*
  * XICS legacy routines - to deprecate one day
  */
@@ -420,4 +446,5 @@ sPAPRIrq spapr_irq_xics_legacy = {
     .print_info  = spapr_irq_print_info_xics,
     .dt_populate = spapr_dt_xics,
     .cpu_intc_create = spapr_irq_cpu_intc_create_xics,
+    .post_load   = spapr_irq_post_load_xics,
 };
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index 13db0428ab..84a25ffb6c 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -43,6 +43,7 @@ typedef struct sPAPRIrq {
                         void *fdt, uint32_t phandle);
     Object *(*cpu_intc_create)(sPAPRMachineState *spapr, Object *cpu,
                                Error **errp);
+    int (*post_load)(sPAPRMachineState *spapr, int version_id);
 } sPAPRIrq;
 
 extern sPAPRIrq spapr_irq_xics;
@@ -53,6 +54,7 @@ void spapr_irq_init(sPAPRMachineState *spapr, Error **errp);
 int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp);
 void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
 qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
+int spapr_irq_post_load(sPAPRMachineState *spapr, int version_id);
 
 /*
  * XICS legacy routines
-- 
2.19.2

On Fri, 21 Dec 2018 at 05:46, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> From: Cédric Le Goater <clg@kaod.org>
>
> Introduce a new sPAPR IRQ handler to handle resend after migration
> when the machine is using a KVM XICS interrupt controller model.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

> @@ -1758,6 +1750,11 @@ static int spapr_post_load(void *opaque, int version_id)
>          }
>      }
>
> +    err = spapr_irq_post_load(spapr, version_id);
> +    if (err) {
> +        return err;
> +    }
> +

Hi; this change causes Coverity to complain (CID 1398591) that
we're now overwriting the setting of err in the earlier
 err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
without ever testing it. (We also do this in the existing
codepath that calls kvmppc_configure_v3_mmu().)

Should the call to spapr_rtc_import_offset() have its
own "if (err) do something" code, or should it simply
be ignoring its return value entirely, or something
more complicated ?

thanks
-- PMM

On 1/3/19 8:07 PM, Peter Maydell wrote:
> On Fri, 21 Dec 2018 at 05:46, David Gibson <david@gibson.dropbear.id.au> wrote:
>>
>> From: Cédric Le Goater <clg@kaod.org>
>>
>> Introduce a new sPAPR IRQ handler to handle resend after migration
>> when the machine is using a KVM XICS interrupt controller model.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
>> @@ -1758,6 +1750,11 @@ static int spapr_post_load(void *opaque, int version_id)
>>          }
>>      }
>>
>> +    err = spapr_irq_post_load(spapr, version_id);
>> +    if (err) {
>> +        return err;
>> +    }
>> +
> 
> Hi; this change causes Coverity to complain (CID 1398591) that
> we're now overwriting the setting of err in the earlier
>  err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
> without ever testing it. (We also do this in the existing
> codepath that calls kvmppc_configure_v3_mmu().)

yes. I suppose we have been missing something like :

@@ -1754,6 +1754,9 @@ static int spapr_post_load(void *opaque,
      * value into the RTC device */
     if (version_id < 3) {
         err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
+        if (err) {
+            return err;
+        }
     }
 
     if (kvm_enabled() && spapr->patb_entry) {


C. 

> 
> Should the call to spapr_rtc_import_offset() have its
> own "if (err) do something" code, or should it simply
> be ignoring its return value entirely, or something
> more complicated ?
> 
> thanks
> -- PMM
> 

On Thu, Jan 03, 2019 at 10:57:21PM +0100, Cédric Le Goater wrote:
> On 1/3/19 8:07 PM, Peter Maydell wrote:
> > On Fri, 21 Dec 2018 at 05:46, David Gibson <david@gibson.dropbear.id.au> wrote:
> >>
> >> From: Cédric Le Goater <clg@kaod.org>
> >>
> >> Introduce a new sPAPR IRQ handler to handle resend after migration
> >> when the machine is using a KVM XICS interrupt controller model.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> >> @@ -1758,6 +1750,11 @@ static int spapr_post_load(void *opaque, int version_id)
> >>          }
> >>      }
> >>
> >> +    err = spapr_irq_post_load(spapr, version_id);
> >> +    if (err) {
> >> +        return err;
> >> +    }
> >> +
> > 
> > Hi; this change causes Coverity to complain (CID 1398591) that
> > we're now overwriting the setting of err in the earlier
> >  err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
> > without ever testing it. (We also do this in the existing
> > codepath that calls kvmppc_configure_v3_mmu().)
> 
> yes. I suppose we have been missing something like :
> 
> @@ -1754,6 +1754,9 @@ static int spapr_post_load(void *opaque,
>       * value into the RTC device */
>      if (version_id < 3) {
>          err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
> +        if (err) {
> +            return err;
> +        }
>      }
>  
>      if (kvm_enabled() && spapr->patb_entry) {

Yeah, looks like it.  Can you send a patch please?


> 
> 
> C. 
> 
> > 
> > Should the call to spapr_rtc_import_offset() have its
> > own "if (err) do something" code, or should it simply
> > be ignoring its return value entirely, or something
> > more complicated ?
> > 
> > thanks
> > -- PMM
> > 
> 

-- 
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