diff mbox series

[v2,28/33] spapr: Remove SpaprIrq::init_kvm hook

Message ID 20190927055028.11493-29-david@gibson.dropbear.id.au
State New
Headers show
Series spapr: IRQ subsystem cleanup | expand

Commit Message

David Gibson Sept. 27, 2019, 5:50 a.m. UTC 
This hook is a bit odd.  The only caller is spapr_irq_init_kvm(), but
it explicitly takes an SpaprIrq *, so it's never really called through the
current SpaprIrq.  Essentially this is just a way of passing through a
function pointer so that spapr_irq_init_kvm() can handle some
configuration and error handling logic without duplicating it between the
xics and xive reset paths.

So, make it just take that function pointer.  Because of earlier reworks
to the KVM connect/disconnect code in the xics and xive backends we can
also eliminate some wrapper functions and streamline error handling a bit.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_irq.c         | 74 +++++++++++++-------------------------
 include/hw/ppc/spapr_irq.h |  1 -
 2 files changed, 25 insertions(+), 50 deletions(-)

Comments

Greg Kurz Sept. 27, 2019, 3:04 p.m. UTC | #1 
On Fri, 27 Sep 2019 15:50:23 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> This hook is a bit odd.  The only caller is spapr_irq_init_kvm(), but
> it explicitly takes an SpaprIrq *, so it's never really called through the
> current SpaprIrq.  Essentially this is just a way of passing through a
> function pointer so that spapr_irq_init_kvm() can handle some
> configuration and error handling logic without duplicating it between the
> xics and xive reset paths.
> 
> So, make it just take that function pointer.  Because of earlier reworks
> to the KVM connect/disconnect code in the xics and xive backends we can
> also eliminate some wrapper functions and streamline error handling a bit.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_irq.c         | 74 +++++++++++++-------------------------
>  include/hw/ppc/spapr_irq.h |  1 -
>  2 files changed, 25 insertions(+), 50 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 561bdbc4de..c6abebc4ef 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -65,33 +65,35 @@ void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num)
>      bitmap_clear(spapr->irq_map, irq - SPAPR_IRQ_MSI, num);
>  }
>  
> -static void spapr_irq_init_kvm(SpaprMachineState *spapr,
> -                                  SpaprIrq *irq, Error **errp)
> +static int spapr_irq_init_kvm(int (*fn)(SpaprInterruptController *, Error **),
> +                              SpaprInterruptController *intc,
> +                              Error **errp)
>  {
> -    MachineState *machine = MACHINE(spapr);
> +    MachineState *machine = MACHINE(qdev_get_machine());
>      Error *local_err = NULL;
>  
>      if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
> -        irq->init_kvm(spapr, &local_err);
> -        if (local_err && machine_kernel_irqchip_required(machine)) {
> -            error_prepend(&local_err,
> -                          "kernel_irqchip requested but unavailable: ");
> -            error_propagate(errp, local_err);
> -            return;
> -        }
> +        if (fn(intc, &local_err) < 0) {
> +            if (machine_kernel_irqchip_required(machine)) {
> +                error_prepend(&local_err,
> +                              "kernel_irqchip requested but unavailable: ");
> +                error_propagate(errp, local_err);
> +                return -1;
> +            }
>  
> -        if (!local_err) {
> -            return;
> +            /*
> +             * We failed to initialize the KVM device, fallback to
> +             * emulated mode
> +             */
> +            error_prepend(&local_err,
> +                          "kernel_irqchip allowed but unavailable: ");
> +            error_append_hint(&local_err,
> +                              "Falling back to kernel-irqchip=off\n");
> +            warn_report_err(local_err);
>          }
> -
> -        /*
> -         * We failed to initialize the KVM device, fallback to
> -         * emulated mode
> -         */
> -        error_prepend(&local_err, "kernel_irqchip allowed but unavailable: ");
> -        error_append_hint(&local_err, "Falling back to kernel-irqchip=off\n");
> -        warn_report_err(local_err);
>      }
> +
> +    return 0;
>  }
>  
>  /*
> @@ -112,20 +114,7 @@ static int spapr_irq_post_load_xics(SpaprMachineState *spapr, int version_id)
>  
>  static void spapr_irq_reset_xics(SpaprMachineState *spapr, Error **errp)
>  {
> -    Error *local_err = NULL;
> -
> -    spapr_irq_init_kvm(spapr, &spapr_irq_xics, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -}
> -
> -static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
> -{
> -    if (kvm_enabled()) {
> -        xics_kvm_connect(SPAPR_INTC(spapr->ics), errp);
> -    }
> +    spapr_irq_init_kvm(xics_kvm_connect, SPAPR_INTC(spapr->ics), errp);
>  }
>  
>  SpaprIrq spapr_irq_xics = {
> @@ -136,7 +125,6 @@ SpaprIrq spapr_irq_xics = {
>  
>      .post_load   = spapr_irq_post_load_xics,
>      .reset       = spapr_irq_reset_xics,
> -    .init_kvm    = spapr_irq_init_kvm_xics,
>  };
>  
>  /*
> @@ -151,7 +139,6 @@ static int spapr_irq_post_load_xive(SpaprMachineState *spapr, int version_id)
>  static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp)
>  {
>      CPUState *cs;
> -    Error *local_err = NULL;
>  
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -160,9 +147,8 @@ static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp)
>          spapr_xive_set_tctx_os_cam(spapr_cpu_state(cpu)->tctx);
>      }
>  
> -    spapr_irq_init_kvm(spapr, &spapr_irq_xive, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (spapr_irq_init_kvm(kvmppc_xive_connect,
> +                           SPAPR_INTC(spapr->xive), errp) < 0) {
>          return;
>      }
>  
> @@ -170,13 +156,6 @@ static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp)
>      spapr_xive_mmio_set_enabled(spapr->xive, true);
>  }
>  
> -static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp)
> -{
> -    if (kvm_enabled()) {
> -        kvmppc_xive_connect(SPAPR_INTC(spapr->xive), errp);
> -    }
> -}
> -
>  SpaprIrq spapr_irq_xive = {
>      .nr_xirqs    = SPAPR_NR_XIRQS,
>      .nr_msis     = SPAPR_NR_MSIS,
> @@ -185,7 +164,6 @@ SpaprIrq spapr_irq_xive = {
>  
>      .post_load   = spapr_irq_post_load_xive,
>      .reset       = spapr_irq_reset_xive,
> -    .init_kvm    = spapr_irq_init_kvm_xive,
>  };
>  
>  /*
> @@ -251,7 +229,6 @@ SpaprIrq spapr_irq_dual = {
>  
>      .post_load   = spapr_irq_post_load_dual,
>      .reset       = spapr_irq_reset_dual,
> -    .init_kvm    = NULL, /* should not be used */
>  };
>  
>  
> @@ -682,7 +659,6 @@ SpaprIrq spapr_irq_xics_legacy = {
>  
>      .post_load   = spapr_irq_post_load_xics,
>      .reset       = spapr_irq_reset_xics,
> -    .init_kvm    = spapr_irq_init_kvm_xics,
>  };
>  
>  static void spapr_irq_register_types(void)
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index c82724fc2b..c947f40571 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -85,7 +85,6 @@ typedef struct SpaprIrq {
>  
>      int (*post_load)(SpaprMachineState *spapr, int version_id);
>      void (*reset)(SpaprMachineState *spapr, Error **errp);
> -    void (*init_kvm)(SpaprMachineState *spapr, Error **errp);
>  } SpaprIrq;
>  
>  extern SpaprIrq spapr_irq_xics;
Cédric Le Goater Sept. 30, 2019, 5:55 a.m. UTC | #2 
On 27/09/2019 07:50, David Gibson wrote:
> This hook is a bit odd.  The only caller is spapr_irq_init_kvm(), but
> it explicitly takes an SpaprIrq *, so it's never really called through the
> current SpaprIrq.  Essentially this is just a way of passing through a
> function pointer so that spapr_irq_init_kvm() can handle some
> configuration and error handling logic without duplicating it between the
> xics and xive reset paths.

yes. There were a few iteration before reaching that state. 
 
> So, make it just take that function pointer.  Because of earlier reworks
> to the KVM connect/disconnect code in the xics and xive backends we can
> also eliminate some wrapper functions and streamline error handling a bit.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/spapr_irq.c         | 74 +++++++++++++-------------------------
>  include/hw/ppc/spapr_irq.h |  1 -
>  2 files changed, 25 insertions(+), 50 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 561bdbc4de..c6abebc4ef 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -65,33 +65,35 @@ void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num)
>      bitmap_clear(spapr->irq_map, irq - SPAPR_IRQ_MSI, num);
>  }
>  
> -static void spapr_irq_init_kvm(SpaprMachineState *spapr,
> -                                  SpaprIrq *irq, Error **errp)
> +static int spapr_irq_init_kvm(int (*fn)(SpaprInterruptController *, Error **),
> +                              SpaprInterruptController *intc,
> +                              Error **errp)
>  {
> -    MachineState *machine = MACHINE(spapr);
> +    MachineState *machine = MACHINE(qdev_get_machine());
>      Error *local_err = NULL;
>  
>      if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
> -        irq->init_kvm(spapr, &local_err);
> -        if (local_err && machine_kernel_irqchip_required(machine)) {
> -            error_prepend(&local_err,
> -                          "kernel_irqchip requested but unavailable: ");
> -            error_propagate(errp, local_err);
> -            return;
> -        }
> +        if (fn(intc, &local_err) < 0) {
> +            if (machine_kernel_irqchip_required(machine)) {
> +                error_prepend(&local_err,
> +                              "kernel_irqchip requested but unavailable: ");
> +                error_propagate(errp, local_err);
> +                return -1;
> +            }
>  
> -        if (!local_err) {
> -            return;
> +            /*
> +             * We failed to initialize the KVM device, fallback to
> +             * emulated mode
> +             */
> +            error_prepend(&local_err,
> +                          "kernel_irqchip allowed but unavailable: ");
> +            error_append_hint(&local_err,
> +                              "Falling back to kernel-irqchip=off\n");
> +            warn_report_err(local_err);
>          }
> -
> -        /*
> -         * We failed to initialize the KVM device, fallback to
> -         * emulated mode
> -         */
> -        error_prepend(&local_err, "kernel_irqchip allowed but unavailable: ");
> -        error_append_hint(&local_err, "Falling back to kernel-irqchip=off\n");
> -        warn_report_err(local_err);
>      }
> +
> +    return 0;
>  }
>  
>  /*
> @@ -112,20 +114,7 @@ static int spapr_irq_post_load_xics(SpaprMachineState *spapr, int version_id)
>  
>  static void spapr_irq_reset_xics(SpaprMachineState *spapr, Error **errp)
>  {
> -    Error *local_err = NULL;
> -
> -    spapr_irq_init_kvm(spapr, &spapr_irq_xics, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -}
> -
> -static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
> -{
> -    if (kvm_enabled()) {
> -        xics_kvm_connect(SPAPR_INTC(spapr->ics), errp);
> -    }
> +    spapr_irq_init_kvm(xics_kvm_connect, SPAPR_INTC(spapr->ics), errp);
>  }
>  
>  SpaprIrq spapr_irq_xics = {
> @@ -136,7 +125,6 @@ SpaprIrq spapr_irq_xics = {
>  
>      .post_load   = spapr_irq_post_load_xics,
>      .reset       = spapr_irq_reset_xics,
> -    .init_kvm    = spapr_irq_init_kvm_xics,
>  };
>  
>  /*
> @@ -151,7 +139,6 @@ static int spapr_irq_post_load_xive(SpaprMachineState *spapr, int version_id)
>  static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp)
>  {
>      CPUState *cs;
> -    Error *local_err = NULL;
>  
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -160,9 +147,8 @@ static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp)
>          spapr_xive_set_tctx_os_cam(spapr_cpu_state(cpu)->tctx);
>      }
>  
> -    spapr_irq_init_kvm(spapr, &spapr_irq_xive, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (spapr_irq_init_kvm(kvmppc_xive_connect,
> +                           SPAPR_INTC(spapr->xive), errp) < 0) {
>          return;
>      }
>  
> @@ -170,13 +156,6 @@ static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp)
>      spapr_xive_mmio_set_enabled(spapr->xive, true);
>  }
>  
> -static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp)
> -{
> -    if (kvm_enabled()) {
> -        kvmppc_xive_connect(SPAPR_INTC(spapr->xive), errp);
> -    }
> -}
> -
>  SpaprIrq spapr_irq_xive = {
>      .nr_xirqs    = SPAPR_NR_XIRQS,
>      .nr_msis     = SPAPR_NR_MSIS,
> @@ -185,7 +164,6 @@ SpaprIrq spapr_irq_xive = {
>  
>      .post_load   = spapr_irq_post_load_xive,
>      .reset       = spapr_irq_reset_xive,
> -    .init_kvm    = spapr_irq_init_kvm_xive,
>  };
>  
>  /*
> @@ -251,7 +229,6 @@ SpaprIrq spapr_irq_dual = {
>  
>      .post_load   = spapr_irq_post_load_dual,
>      .reset       = spapr_irq_reset_dual,
> -    .init_kvm    = NULL, /* should not be used */
>  };
>  
>  
> @@ -682,7 +659,6 @@ SpaprIrq spapr_irq_xics_legacy = {
>  
>      .post_load   = spapr_irq_post_load_xics,
>      .reset       = spapr_irq_reset_xics,
> -    .init_kvm    = spapr_irq_init_kvm_xics,
>  };
>  
>  static void spapr_irq_register_types(void)
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index c82724fc2b..c947f40571 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -85,7 +85,6 @@ typedef struct SpaprIrq {
>  
>      int (*post_load)(SpaprMachineState *spapr, int version_id);
>      void (*reset)(SpaprMachineState *spapr, Error **errp);
> -    void (*init_kvm)(SpaprMachineState *spapr, Error **errp);
>  } SpaprIrq;
>  
>  extern SpaprIrq spapr_irq_xics;
>
diff mbox series

Patch

diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 561bdbc4de..c6abebc4ef 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -65,33 +65,35 @@  void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num)
     bitmap_clear(spapr->irq_map, irq - SPAPR_IRQ_MSI, num);
 }
 
-static void spapr_irq_init_kvm(SpaprMachineState *spapr,
-                                  SpaprIrq *irq, Error **errp)
+static int spapr_irq_init_kvm(int (*fn)(SpaprInterruptController *, Error **),
+                              SpaprInterruptController *intc,
+                              Error **errp)
 {
-    MachineState *machine = MACHINE(spapr);
+    MachineState *machine = MACHINE(qdev_get_machine());
     Error *local_err = NULL;
 
     if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
-        irq->init_kvm(spapr, &local_err);
-        if (local_err && machine_kernel_irqchip_required(machine)) {
-            error_prepend(&local_err,
-                          "kernel_irqchip requested but unavailable: ");
-            error_propagate(errp, local_err);
-            return;
-        }
+        if (fn(intc, &local_err) < 0) {
+            if (machine_kernel_irqchip_required(machine)) {
+                error_prepend(&local_err,
+                              "kernel_irqchip requested but unavailable: ");
+                error_propagate(errp, local_err);
+                return -1;
+            }
 
-        if (!local_err) {
-            return;
+            /*
+             * We failed to initialize the KVM device, fallback to
+             * emulated mode
+             */
+            error_prepend(&local_err,
+                          "kernel_irqchip allowed but unavailable: ");
+            error_append_hint(&local_err,
+                              "Falling back to kernel-irqchip=off\n");
+            warn_report_err(local_err);
         }
-
-        /*
-         * We failed to initialize the KVM device, fallback to
-         * emulated mode
-         */
-        error_prepend(&local_err, "kernel_irqchip allowed but unavailable: ");
-        error_append_hint(&local_err, "Falling back to kernel-irqchip=off\n");
-        warn_report_err(local_err);
     }
+
+    return 0;
 }
 
 /*
@@ -112,20 +114,7 @@  static int spapr_irq_post_load_xics(SpaprMachineState *spapr, int version_id)
 
 static void spapr_irq_reset_xics(SpaprMachineState *spapr, Error **errp)
 {
-    Error *local_err = NULL;
-
-    spapr_irq_init_kvm(spapr, &spapr_irq_xics, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-}
-
-static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
-{
-    if (kvm_enabled()) {
-        xics_kvm_connect(SPAPR_INTC(spapr->ics), errp);
-    }
+    spapr_irq_init_kvm(xics_kvm_connect, SPAPR_INTC(spapr->ics), errp);
 }
 
 SpaprIrq spapr_irq_xics = {
@@ -136,7 +125,6 @@  SpaprIrq spapr_irq_xics = {
 
     .post_load   = spapr_irq_post_load_xics,
     .reset       = spapr_irq_reset_xics,
-    .init_kvm    = spapr_irq_init_kvm_xics,
 };
 
 /*
@@ -151,7 +139,6 @@  static int spapr_irq_post_load_xive(SpaprMachineState *spapr, int version_id)
 static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp)
 {
     CPUState *cs;
-    Error *local_err = NULL;
 
     CPU_FOREACH(cs) {
         PowerPCCPU *cpu = POWERPC_CPU(cs);
@@ -160,9 +147,8 @@  static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp)
         spapr_xive_set_tctx_os_cam(spapr_cpu_state(cpu)->tctx);
     }
 
-    spapr_irq_init_kvm(spapr, &spapr_irq_xive, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (spapr_irq_init_kvm(kvmppc_xive_connect,
+                           SPAPR_INTC(spapr->xive), errp) < 0) {
         return;
     }
 
@@ -170,13 +156,6 @@  static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp)
     spapr_xive_mmio_set_enabled(spapr->xive, true);
 }
 
-static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp)
-{
-    if (kvm_enabled()) {
-        kvmppc_xive_connect(SPAPR_INTC(spapr->xive), errp);
-    }
-}
-
 SpaprIrq spapr_irq_xive = {
     .nr_xirqs    = SPAPR_NR_XIRQS,
     .nr_msis     = SPAPR_NR_MSIS,
@@ -185,7 +164,6 @@  SpaprIrq spapr_irq_xive = {
 
     .post_load   = spapr_irq_post_load_xive,
     .reset       = spapr_irq_reset_xive,
-    .init_kvm    = spapr_irq_init_kvm_xive,
 };
 
 /*
@@ -251,7 +229,6 @@  SpaprIrq spapr_irq_dual = {
 
     .post_load   = spapr_irq_post_load_dual,
     .reset       = spapr_irq_reset_dual,
-    .init_kvm    = NULL, /* should not be used */
 };
 
 
@@ -682,7 +659,6 @@  SpaprIrq spapr_irq_xics_legacy = {
 
     .post_load   = spapr_irq_post_load_xics,
     .reset       = spapr_irq_reset_xics,
-    .init_kvm    = spapr_irq_init_kvm_xics,
 };
 
 static void spapr_irq_register_types(void)
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index c82724fc2b..c947f40571 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -85,7 +85,6 @@  typedef struct SpaprIrq {
 
     int (*post_load)(SpaprMachineState *spapr, int version_id);
     void (*reset)(SpaprMachineState *spapr, Error **errp);
-    void (*init_kvm)(SpaprMachineState *spapr, Error **errp);
 } SpaprIrq;
 
 extern SpaprIrq spapr_irq_xics;