Re: [Qemu-devel] [RFC PATCH] spapr/irq: force XICS interrupt mode on non P9 machines
On Mon, 18 Mar 2019 07:27:59 +0100
Cédric Le Goater 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
> >
> > 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, _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(_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 = _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
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
>
> 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(_err, "forcing XICS interrupt controller");
>> +warn_report_err(local_err);
>> +spapr->irq = _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
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
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(_err, "forcing XICS interrupt controller");
> +warn_report_err(local_err);
> +spapr->irq = _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
signature.asc
Description: PGP signature
[Qemu-devel] [RFC PATCH] spapr/irq: force XICS interrupt mode on non P9 machines
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
---
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(_err, "forcing XICS interrupt controller");
+warn_report_err(local_err);
+spapr->irq = _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
