spapr/irq: force XICS interrupt mode on non P9 machines

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

Posted by Cédric Le Goater 5 years, 1 month ago

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 <clg@kaod.org>
---
 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)) {
+        error_setg(&local_err, "forcing XICS interrupt controller");
+        warn_report_err(local_err);
+        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);
-- 
2.20.1

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

Posted by David Gibson 5 years, 1 month ago

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 <clg@kaod.org>

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.

We do need to make sure we default to xics mode with POWER8, even on
new machine types.

> ---
>  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)) {
> +        error_setg(&local_err, "forcing XICS interrupt controller");
> +        warn_report_err(local_err);
> +        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);

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

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

Posted by Cédric Le Goater 5 years, 1 month ago

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 <clg@kaod.org>
> 
> 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. 

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)) {
>> +        error_setg(&local_err, "forcing XICS interrupt controller");
>> +        warn_report_err(local_err);
>> +        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);
>

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

Posted by Greg Kurz 5 years, 1 month ago

On Mon, 18 Mar 2019 07:27:59 +0100
Cédric Le Goater <clg@kaod.org> 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 <clg@kaod.org>  
> > 
> > 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);  
> >   
>