| Message ID | 155023083585.1011724.2868047424353921455.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xics: Get rid of KVM specific classes | expand |
On 2/15/19 12:40 PM, Greg Kurz wrote: > The "simple" ICS class knows how to interract with KVM. Adapt sPAPR to use > it instead of the ICS KVM class. You are changing the type name. What about migration ? Can't we move the xics_kvm_init() and xics_spapr_init() call under spapr_ics_create() ? It would simplify a lot the routine I think if these were done before creating the ICSState. C. > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/ppc/spapr_irq.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index 9f43b7b3bf16..4aa8165307c7 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -67,13 +67,12 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr) > */ > > static ICSState *spapr_ics_create(sPAPRMachineState *spapr, > - const char *type_ics, > int nr_irqs, Error **errp) > { > Error *local_err = NULL; > Object *obj; > > - obj = object_new(type_ics); > + obj = object_new(TYPE_ICS_SIMPLE); > object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); > object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), > &error_abort); > @@ -98,14 +97,14 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs, > { > MachineState *machine = MACHINE(spapr); > Error *local_err = NULL; > + bool xics_kvm = false; > > if (kvm_enabled()) { > if (machine_kernel_irqchip_allowed(machine) && > !xics_kvm_init(spapr, &local_err)) { > - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, > - &local_err); > + xics_kvm = true; > } > - if (machine_kernel_irqchip_required(machine) && !spapr->ics) { > + if (machine_kernel_irqchip_required(machine) && !xics_kvm) { > error_prepend(&local_err, > "kernel_irqchip requested but unavailable: "); > goto error; > @@ -114,12 +113,12 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs, > local_err = NULL; > } > > - if (!spapr->ics) { > + if (!xics_kvm) { > xics_spapr_init(spapr); > - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, > - &local_err); > } > > + spapr->ics = spapr_ics_create(spapr, nr_irqs, &local_err); > + > error: > error_propagate(errp, local_err); > } >
On Fri, 15 Feb 2019 14:02:16 +0100 Cédric Le Goater <clg@kaod.org> wrote: > On 2/15/19 12:40 PM, Greg Kurz wrote: > > The "simple" ICS class knows how to interract with KVM. Adapt sPAPR to use > > it instead of the ICS KVM class. > > You are changing the type name. What about migration ? > Huh ?!? I don't see how the type name would relate to migration. AFAICT they aren't being referred to in the vmstate descriptors in xics.c. > Can't we move the xics_kvm_init() and xics_spapr_init() call under > spapr_ics_create() ? It would simplify a lot the routine I think > if these were done before creating the ICSState. > I'll look into that if there's a v2 or else in a followup patch. > C. > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/ppc/spapr_irq.c | 15 +++++++-------- > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > > index 9f43b7b3bf16..4aa8165307c7 100644 > > --- a/hw/ppc/spapr_irq.c > > +++ b/hw/ppc/spapr_irq.c > > @@ -67,13 +67,12 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr) > > */ > > > > static ICSState *spapr_ics_create(sPAPRMachineState *spapr, > > - const char *type_ics, > > int nr_irqs, Error **errp) > > { > > Error *local_err = NULL; > > Object *obj; > > > > - obj = object_new(type_ics); > > + obj = object_new(TYPE_ICS_SIMPLE); > > object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); > > object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), > > &error_abort); > > @@ -98,14 +97,14 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs, > > { > > MachineState *machine = MACHINE(spapr); > > Error *local_err = NULL; > > + bool xics_kvm = false; > > > > if (kvm_enabled()) { > > if (machine_kernel_irqchip_allowed(machine) && > > !xics_kvm_init(spapr, &local_err)) { > > - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, > > - &local_err); > > + xics_kvm = true; > > } > > - if (machine_kernel_irqchip_required(machine) && !spapr->ics) { > > + if (machine_kernel_irqchip_required(machine) && !xics_kvm) { > > error_prepend(&local_err, > > "kernel_irqchip requested but unavailable: "); > > goto error; > > @@ -114,12 +113,12 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs, > > local_err = NULL; > > } > > > > - if (!spapr->ics) { > > + if (!xics_kvm) { > > xics_spapr_init(spapr); > > - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, > > - &local_err); > > } > > > > + spapr->ics = spapr_ics_create(spapr, nr_irqs, &local_err); > > + > > error: > > error_propagate(errp, local_err); > > } > > >
On 2/15/19 2:32 PM, Greg Kurz wrote: > On Fri, 15 Feb 2019 14:02:16 +0100 > Cédric Le Goater <clg@kaod.org> wrote: > >> On 2/15/19 12:40 PM, Greg Kurz wrote: >>> The "simple" ICS class knows how to interract with KVM. Adapt sPAPR to use >>> it instead of the ICS KVM class. >> >> You are changing the type name. What about migration ? >> > > Huh ?!? I don't see how the type name would relate to migration. AFAICT they > aren't being referred to in the vmstate descriptors in xics.c. In that case all is fine. C. >> Can't we move the xics_kvm_init() and xics_spapr_init() call under >> spapr_ics_create() ? It would simplify a lot the routine I think >> if these were done before creating the ICSState. >> > > I'll look into that if there's a v2 or else in a followup patch. > >> C. >> >>> Signed-off-by: Greg Kurz <groug@kaod.org> >>> --- >>> hw/ppc/spapr_irq.c | 15 +++++++-------- >>> 1 file changed, 7 insertions(+), 8 deletions(-) >>> >>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c >>> index 9f43b7b3bf16..4aa8165307c7 100644 >>> --- a/hw/ppc/spapr_irq.c >>> +++ b/hw/ppc/spapr_irq.c >>> @@ -67,13 +67,12 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr) >>> */ >>> >>> static ICSState *spapr_ics_create(sPAPRMachineState *spapr, >>> - const char *type_ics, >>> int nr_irqs, Error **errp) >>> { >>> Error *local_err = NULL; >>> Object *obj; >>> >>> - obj = object_new(type_ics); >>> + obj = object_new(TYPE_ICS_SIMPLE); >>> object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); >>> object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), >>> &error_abort); >>> @@ -98,14 +97,14 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs, >>> { >>> MachineState *machine = MACHINE(spapr); >>> Error *local_err = NULL; >>> + bool xics_kvm = false; >>> >>> if (kvm_enabled()) { >>> if (machine_kernel_irqchip_allowed(machine) && >>> !xics_kvm_init(spapr, &local_err)) { >>> - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, >>> - &local_err); >>> + xics_kvm = true; >>> } >>> - if (machine_kernel_irqchip_required(machine) && !spapr->ics) { >>> + if (machine_kernel_irqchip_required(machine) && !xics_kvm) { >>> error_prepend(&local_err, >>> "kernel_irqchip requested but unavailable: "); >>> goto error; >>> @@ -114,12 +113,12 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs, >>> local_err = NULL; >>> } >>> >>> - if (!spapr->ics) { >>> + if (!xics_kvm) { >>> xics_spapr_init(spapr); >>> - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, >>> - &local_err); >>> } >>> >>> + spapr->ics = spapr_ics_create(spapr, nr_irqs, &local_err); >>> + >>> error: >>> error_propagate(errp, local_err); >>> } >>> >> >
On Fri, Feb 15, 2019 at 02:02:16PM +0100, Cédric Le Goater wrote: > On 2/15/19 12:40 PM, Greg Kurz wrote: > > The "simple" ICS class knows how to interract with KVM. Adapt sPAPR to use > > it instead of the ICS KVM class. > > You are changing the type name. What about migration ? As with ICP this would be a problem if the ics-kvm class had any extra migration state, but it doesn't.
On Fri, Feb 15, 2019 at 12:40:35PM +0100, Greg Kurz wrote: > The "simple" ICS class knows how to interract with KVM. Adapt sPAPR to use > it instead of the ICS KVM class. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/ppc/spapr_irq.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index 9f43b7b3bf16..4aa8165307c7 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -67,13 +67,12 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr) > */ > > static ICSState *spapr_ics_create(sPAPRMachineState *spapr, > - const char *type_ics, > int nr_irqs, Error **errp) > { > Error *local_err = NULL; > Object *obj; > > - obj = object_new(type_ics); > + obj = object_new(TYPE_ICS_SIMPLE); > object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); > object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), > &error_abort); > @@ -98,14 +97,14 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs, > { > MachineState *machine = MACHINE(spapr); > Error *local_err = NULL; > + bool xics_kvm = false; > > if (kvm_enabled()) { > if (machine_kernel_irqchip_allowed(machine) && > !xics_kvm_init(spapr, &local_err)) { > - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, > - &local_err); > + xics_kvm = true; > } > - if (machine_kernel_irqchip_required(machine) && !spapr->ics) { > + if (machine_kernel_irqchip_required(machine) && !xics_kvm) { > error_prepend(&local_err, > "kernel_irqchip requested but unavailable: "); > goto error; > @@ -114,12 +113,12 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs, > local_err = NULL; > } > > - if (!spapr->ics) { > + if (!xics_kvm) { > xics_spapr_init(spapr); > - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, > - &local_err); As Cédric suggested, I think this would be nicer if xics_kvm_init() and xics_spapr_init() were folded together with the KVM fallback inside. That can be a follow up patch, though, Applied. > } > > + spapr->ics = spapr_ics_create(spapr, nr_irqs, &local_err); > + > error: > error_propagate(errp, local_err); > } >
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c index 9f43b7b3bf16..4aa8165307c7 100644 --- a/hw/ppc/spapr_irq.c +++ b/hw/ppc/spapr_irq.c @@ -67,13 +67,12 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr) */ static ICSState *spapr_ics_create(sPAPRMachineState *spapr, - const char *type_ics, int nr_irqs, Error **errp) { Error *local_err = NULL; Object *obj; - obj = object_new(type_ics); + obj = object_new(TYPE_ICS_SIMPLE); object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort); object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr), &error_abort); @@ -98,14 +97,14 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs, { MachineState *machine = MACHINE(spapr); Error *local_err = NULL; + bool xics_kvm = false; if (kvm_enabled()) { if (machine_kernel_irqchip_allowed(machine) && !xics_kvm_init(spapr, &local_err)) { - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, - &local_err); + xics_kvm = true; } - if (machine_kernel_irqchip_required(machine) && !spapr->ics) { + if (machine_kernel_irqchip_required(machine) && !xics_kvm) { error_prepend(&local_err, "kernel_irqchip requested but unavailable: "); goto error; @@ -114,12 +113,12 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs, local_err = NULL; } - if (!spapr->ics) { + if (!xics_kvm) { xics_spapr_init(spapr); - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, - &local_err); } + spapr->ics = spapr_ics_create(spapr, nr_irqs, &local_err); + error: error_propagate(errp, local_err); }
The "simple" ICS class knows how to interract with KVM. Adapt sPAPR to use it instead of the ICS KVM class. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr_irq.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)