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