Re: [Qemu-devel] [PATCH v2 08/22] ppc/xics: use the QOM interface to resend irqs

[David Gibson]

On Thu, Feb 16, 2017 at 02:47:31PM +0100, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <address@hidden>
> ---
>  hw/intc/xics.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 0ffdf09c5304..2decb921e4e3 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -229,16 +229,15 @@ static void icp_check_ipi(ICPState *ss)
>      qemu_irq_raise(ss->output);
>  }
>  
> -static void icp_resend(ICPState *ss)
> +static void icp_resend(XICSInterface *xi, ICPState *ss)
>  {
> -    ICSState *ics;
> +    XICSInterfaceClass *xic = XICS_INTERFACE_GET_CLASS(xi);
>  
>      if (ss->mfrr < CPPR(ss)) {
>          icp_check_ipi(ss);
>      }
> -    QLIST_FOREACH(ics, &ss->xics->ics, list) {
> -        ics_resend(ics);
> -    }
> +
> +    xic->ics_resend(xi);
>  }
>  
>  void icp_set_cppr(ICPState *ss, uint8_t cppr)
> @@ -262,7 +261,7 @@ void icp_set_cppr(ICPState *ss, uint8_t cppr)
>          }
>      } else {
>          if (!XISR(ss)) {
> -            icp_resend(ss);
> +            icp_resend(XICS_INTERFACE(qdev_get_machine()), ss);

Here you're assuming that the machine is the implementor of the xics
interface, which is kinda ugly.  The ICP should have a pointer to the
xics interface, which will eventually replace the pointer to the
overall xics object it has now.

But I haven't read the rest of the series yet, maybe this is just
transitional.

>          }
>      }
>  }
> @@ -299,6 +298,8 @@ uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr)
>  
>  void icp_eoi(ICPState *ss, uint32_t xirr)
>  {
> +    XICSInterface *xi = XICS_INTERFACE(qdev_get_machine());
> +    XICSInterfaceClass *xic = XICS_INTERFACE_GET_CLASS(xi);
>      ICSState *ics;
>      uint32_t irq;
>  
> @@ -306,13 +307,13 @@ void icp_eoi(ICPState *ss, uint32_t xirr)
>      ss->xirr = (ss->xirr & ~CPPR_MASK) | (xirr & CPPR_MASK);
>      trace_xics_icp_eoi(ss->cs->cpu_index, xirr, ss->xirr);
>      irq = xirr & XISR_MASK;
> -    QLIST_FOREACH(ics, &ss->xics->ics, list) {
> -        if (ics_valid_irq(ics, irq)) {
> -            ics_eoi(ics, irq);
> -        }
> +
> +    ics = xic->ics_get(xi, irq);
> +    if (ics) {
> +        ics_eoi(ics, irq);
>      }
>      if (!XISR(ss)) {
> -        icp_resend(ss);
> +        icp_resend(xi, ss);
>      }
>  }
>  
> @@ -592,10 +593,11 @@ static void ics_simple_reset(DeviceState *dev)
>  
>  static int ics_simple_post_load(ICSState *ics, int version_id)
>  {
> +    XICSInterface *xi = XICS_INTERFACE(qdev_get_machine());
>      int i;
>  
>      for (i = 0; i < ics->xics->nr_servers; i++) {
> -        icp_resend(&ics->xics->ss[i]);
> +        icp_resend(xi, &ics->xics->ss[i]);
>      }

This resend triggering needs to get moved to the xics interface
implementor - i.e. the machine.  It's actually already broken right
now, since it incorrectly relies on the ordering of the ics and icp
restore during migration.

>      return 0;

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