Re: [Qemu-ppc] [PATCH v3 03/19] xics: Disintricate allocation and type setting of interrupts

[Greg Kurz]

On Fri, 18 Jan 2019 13:26:29 +0100
Cédric Le Goater <address@hidden> wrote:

> On 1/17/19 6:14 PM, Greg Kurz wrote:
> > The current code assumes that an interrupt is allocated as soon as its
> > type is set to MSI or LSI. PHB hotplug will require to be able to set
> > the type of an interrupt before actually allocating it.
> > 
> > Disintricate type setting from allocation by using another flag bit for
> > the latter. Introduce a new ics_claim_irq() function for allocation. The
> > behavior of aborting if the same irq gets allocated twice is kept.
> > 
> > ics_set_irq_type() now only sets the type to MSI or LSI. It doesn't bring
> > anything to abort if the type was already set before. Drop the assert
> > and XICS_FLAGS_IRQ_MASK on the way.
> > 
> > Older QEMUs don't know about XICS_FLAGS_IRQ_CLAIMED. In order to safely
> > handle incoming migration, we must fix the irq flags. This is done at
> > post load thanks to a compat property. We don't need to do anything for
> > backward migration since older machines only care for the irq type.
> > 
> > Signed-off-by: Greg Kurz <address@hidden>  
> 
>  
> Reviewed-by: Cédric Le Goater <address@hidden>
> 
> In case of a respin, may be transform macro ICS_IRQ_FREE() in a static 
> inline helper.
> 

Yeah, there will be a respin (see my other mail in this thread) and I'll
just do that.

> Thanks,
> 
> C.
> 
> > ---
> >  hw/core/machine.c     |    4 ++++
> >  hw/intc/xics.c        |   28 +++++++++++++++++++++++++---
> >  hw/ppc/pnv_psi.c      |    1 +
> >  hw/ppc/spapr_irq.c    |    4 +---
> >  include/hw/ppc/xics.h |    8 ++++++--
> >  5 files changed, 37 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 1a0a9ab1117a..536a34092367 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -25,6 +25,10 @@
> >  
> >  GlobalProperty hw_compat_3_1[] = {
> >      {
> > +        .driver   = "ics-base",
> > +        .property = "has-claimed-flag",
> > +        .value    = "off",
> > +    },{
> >          .driver   = "pcie-root-port",
> >          .property = "x-speed",
> >          .value    = "2_5",
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index 16e8ffa2aaf7..82cf04548907 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -75,7 +75,7 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon)
> >      for (i = 0; i < ics->nr_irqs; i++) {
> >          ICSIRQState *irq = ics->irqs + i;
> >  
> > -        if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) {
> > +        if (!(irq->flags & XICS_FLAGS_IRQ_CLAIMED)) {
> >              continue;
> >          }
> >          monitor_printf(mon, "  %4x %s %02x %02x\n",
> > @@ -662,6 +662,22 @@ static int ics_base_dispatch_post_load(void *opaque, 
> > int version_id)
> >      ICSState *ics = opaque;
> >      ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
> >  
> > +    if (!ics->has_claimed_flag) {
> > +        int i;
> > +
> > +        for (i = 0; i < ics->nr_irqs; i++) {
> > +            ICSIRQState *irq = ics->irqs + i;
> > +
> > +            /*
> > +             * For older machines, allocating the irq and setting its type 
> > is
> > +             * the same thing.
> > +             */
> > +            if (irq->flags & (XICS_FLAGS_IRQ_LSI| XICS_FLAGS_IRQ_MSI)) {
> > +                irq->flags |= XICS_FLAGS_IRQ_CLAIMED;
> > +            }
> > +        }
> > +    }
> > +
> >      if (info->post_load) {
> >          return info->post_load(ics, version_id);
> >      }
> > @@ -702,6 +718,7 @@ static const VMStateDescription vmstate_ics_base = {
> >  
> >  static Property ics_base_properties[] = {
> >      DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0),
> > +    DEFINE_PROP_BOOL("has-claimed-flag", ICSState, has_claimed_flag, true),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -741,10 +758,15 @@ ICPState *xics_icp_get(XICSFabric *xi, int server)
> >      return xic->icp_get(xi, server);
> >  }
> >  
> > -void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
> > +void ics_claim_irq(ICSState *ics, int srcno)
> >  {
> > -    assert(!(ics->irqs[srcno].flags & XICS_FLAGS_IRQ_MASK));
> > +    assert(ICS_IRQ_FREE(ics, srcno));
> > +
> > +    ics->irqs[srcno].flags |= XICS_FLAGS_IRQ_CLAIMED;
> > +}
> >  
> > +void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
> > +{
> >      ics->irqs[srcno].flags |=
> >          lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI;
> >  }
> > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> > index 8ced09506321..ced34e1119dc 100644
> > --- a/hw/ppc/pnv_psi.c
> > +++ b/hw/ppc/pnv_psi.c
> > @@ -488,6 +488,7 @@ static void pnv_psi_realize(DeviceState *dev, Error 
> > **errp)
> >  
> >      for (i = 0; i < ics->nr_irqs; i++) {
> >          ics_set_irq_type(ics, i, true);
> > +        ics_claim_irq(ics, i);
> >      }
> >  
> >      psi->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index 1da7a32348fc..86c712d15382 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -125,9 +125,6 @@ error:
> >      error_propagate(errp, local_err);
> >  }
> >  
> > -#define ICS_IRQ_FREE(ics, srcno)   \
> > -    (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_MASK)))
> > -
> >  static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq, bool 
> > lsi,
> >                                  Error **errp)
> >  {
> > @@ -146,6 +143,7 @@ static int spapr_irq_claim_xics(sPAPRMachineState 
> > *spapr, int irq, bool lsi,
> >      }
> >  
> >      ics_set_irq_type(ics, irq - ics->offset, lsi);
> > +    ics_claim_irq(ics, irq - ics->offset);
> >      return 0;
> >  }
> >  
> > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > index fad786e8b22d..e3be5cc663c5 100644
> > --- a/include/hw/ppc/xics.h
> > +++ b/include/hw/ppc/xics.h
> > @@ -131,6 +131,7 @@ struct ICSState {
> >      /*< public >*/
> >      uint32_t nr_irqs;
> >      uint32_t offset;
> > +    bool has_claimed_flag;
> >      ICSIRQState *irqs;
> >      XICSFabric *xics;
> >  };
> > @@ -153,13 +154,15 @@ struct ICSIRQState {
> >  #define XICS_STATUS_PRESENTED          0x10
> >  #define XICS_STATUS_QUEUED             0x20
> >      uint8_t status;
> > -/* (flags & XICS_FLAGS_IRQ_MASK) == 0 means the interrupt is not allocated 
> > */
> >  #define XICS_FLAGS_IRQ_LSI             0x1
> >  #define XICS_FLAGS_IRQ_MSI             0x2
> > -#define XICS_FLAGS_IRQ_MASK            0x3
> > +#define XICS_FLAGS_IRQ_CLAIMED         0x4
> >      uint8_t flags;
> >  };
> >  
> > +#define ICS_IRQ_FREE(ics, srcno)   \
> > +    (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_CLAIMED)))
> > +
> >  struct XICSFabric {
> >      Object parent;
> >  };
> > @@ -193,6 +196,7 @@ void ics_simple_write_xive(ICSState *ics, int nr, int 
> > server,
> >  void ics_simple_set_irq(void *opaque, int srcno, int val);
> >  void ics_kvm_set_irq(void *opaque, int srcno, int val);
> >  
> > +void ics_claim_irq(ICSState *ics, int srcno);
> >  void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
> >  void icp_pic_print_info(ICPState *icp, Monitor *mon);
> >  void ics_pic_print_info(ICSState *ics, Monitor *mon);
> >   
>