Re: [Qemu-ppc] [RFC PATCH] spapr/irq: force XICS interrupt mode on non P9 machines

[Greg Kurz]

On Mon, 18 Mar 2019 07:27:59 +0100
Cédric Le Goater <address@hidden> wrote:

> On 3/18/19 2:52 AM, David Gibson wrote:
> > On Sun, Mar 17, 2019 at 09:33:42PM +0100, Cédric Le Goater wrote:  
> >> There is no need to propose the 'dual' interrupt mode interrupt device
> >> on POWER7/8 machines and the XIVE mode will not operate. Simply force
> >> XICS in this case.
> >>
> >> This makes the check in spapr_machine_init() redundant on XIVE-only
> >> machines.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>  
> > 
> > This is not my preferred approach.  If the user explicitly selects
> > xive or dual mode with a POWER8 cpu, we should hard error, rather than
> > forcing a different mode from the one requested.  
> 
> I though so.
> 
> > We do need to make sure we default to xics mode with POWER8, even on
> > new machine types.  
> 
> That's the problem. 
> 
> When parsing the options, in the machine instance_init(), we don't 
> know the CPU type. 
> 

Yes but you still can have spapr_irq_init() to report an error, and
we have:

    /* Set up Interrupt Controller before we create the VCPUs */
    spapr_irq_init(spapr, &error_fatal);

Some remarks below.

> C. 
> 
> >> ---
> >>  hw/ppc/spapr_irq.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> >> index f2ca1bb66c9d..d27ae68915a1 100644
> >> --- a/hw/ppc/spapr_irq.c
> >> +++ b/hw/ppc/spapr_irq.c
> >> @@ -16,6 +16,7 @@
> >>  #include "hw/ppc/spapr_xive.h"
> >>  #include "hw/ppc/xics.h"
> >>  #include "hw/ppc/xics_spapr.h"
> >> +#include "cpu-models.h"
> >>  #include "sysemu/kvm.h"
> >>  
> >>  #include "trace.h"
> >> @@ -655,6 +656,7 @@ SpaprIrq spapr_irq_dual = {
> >>  void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
> >>  {
> >>      MachineState *machine = MACHINE(spapr);
> >> +    Error *local_err = NULL;
> >>  
> >>      if (machine_kernel_irqchip_split(machine)) {
> >>          error_setg(errp, "kernel_irqchip split mode not supported on 
> >> pseries");
> >> @@ -667,6 +669,14 @@ void spapr_irq_init(SpaprMachineState *spapr, Error 
> >> **errp)
> >>          return;
> >>      }
> >>  
> >> +    /* Force XICS on non P9 machines */
> >> +    if (!ppc_type_check_compat(machine->cpu_type, 
> >> CPU_POWERPC_LOGICAL_3_00,
> >> +                              0, spapr->max_compat_pvr)) {

This also takes into account a POWER9 (or newer) CPU running in architected
power8 (or less) mode, which is a good thing. Maybe this should be mentioned
in the changelog for clarity.

> >> +        error_setg(&local_err, "forcing XICS interrupt controller");
> >> +        warn_report_err(local_err);

BTW, at this point, local_err is a dangling pointer. If some future change
causes the variable to be passed again to error_setg(), QEMU will abort.
A better practice would be to set local_err to NULL if you don't return
right away.

Also, instead of doing error_setg()+warn_report_err(), you could just
call warn_report() directly, without involving an error variable.

Anyway, in the present case you just need to error_setg(errp) and return.

> >> +        spapr->irq = &spapr_irq_xics;
> >> +    }
> >> +
> >>      /* Initialize the MSI IRQ allocator. */
> >>      if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
> >>          spapr_irq_msi_init(spapr, spapr->irq->nr_msis);  
> >   
>